Remove PolarisCallContext.getMetaStore #2229
Conversation
XN137
changed the title
XN137
force-pushed
the
Remove-PolarisCallContext.getMetaStore
branch
2 times, most recently
from
5cd5875 to
407ccb8
Compare
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
|
|
||
| BasePersistence ms = polarisCallContext.getMetaStore(); | ||
| // hackish way to inspect internal metastore | ||
| BasePersistence metaStore = ((BaseMetaStoreManager<?>) polarisMetaStoreManager).getMetaStore(); |
There was a problem hiding this comment.
| BasePersistence metaStore = ((BaseMetaStoreManager<?>) polarisMetaStoreManager).getMetaStore(); | |
| BasePersistence metaStore = getBasePersistence(); |
plus the implementation
protected BasePersistence getBasePersistence() {
return ((BaseMetaStoreManager<?>) polarisMetaStoreManager).getMetaStore();
}
| return PolarisStorageConfigurationInfo.deserialize(storageConfigInfoStr); | ||
| } | ||
|
|
||
| private final Supplier<MS> metaStoreSupplier; |
XN137
force-pushed
the
Remove-PolarisCallContext.getMetaStore
branch
from
407ccb8 to
5bb113c
Compare
| private final Supplier<MS> metaStoreSupplier; | ||
|
|
||
| public BaseMetaStoreManager(Supplier<MS> metaStoreSupplier) { | ||
| this.metaStoreSupplier = Suppliers.memoize(metaStoreSupplier::get); |
There was a problem hiding this comment.
it is still unclear what the actual lifetime scope of the BasePersistence should be here.
on main its lifetime is bound to the PolarisCallContext.
when we use memoize here, we'd have a single instance per realm.
when we dont use memoize here, we would be creating more instances than before, because for example AtomicOperationMetaStoreManager#loadResolvedEntityByName calls AtomicOperationMetaStoreManager#createEntityIfNotExists internally, both of which call getMetaStore().
i am wondering if the subclasses of BaseMetaStoreManager would need to tell the base class how to use the Supplier, depending on whether their specific BasePersistence impl instance has state or not.
theoretically we could also let BaseMetaStoreManager have a cache keyed by PolarisCallContext to keep exactly the existing "instance per callcontext" semantics though it would look a bit hacky
There was a problem hiding this comment.
FYI A lot of the machinery behind the MetaStoreManager/BasePersistence interactions/factories exists precisely to enforce the contract that BasePersistence instances are bound to a PolarisCallContext lifetime and are RequestScoped.
If the request scoping wasn't necessary, indeed none of the indirection would be necessary. Similarly, machinery for making MetaStoreManager realm-scoped goes beyond vanilla @ApplicationScoped annotations in order to define/enforce realm-scoping.
There was a problem hiding this comment.
as per my comment this is still WIP and i am figuring out the actual lifetime of the BasePersistence object.
if what you are saying is true and the currently suggested change would break an existing contract, how is it that no test in CI is failing?
can you help adding a test that covers this contract explicitly?
or at least describe how such a test would need to look like?
that would help a lot!
|
Hm, |
XN137
force-pushed
the
Remove-PolarisCallContext.getMetaStore
branch
4 times, most recently
from
79c4b42 to
3d4cd6d
Compare
the functionality no longer needs to be publically available as it has become a private implementation detail of the `MetaStoreManagerFactory`
XN137
force-pushed
the
Remove-PolarisCallContext.getMetaStore
branch
from
3d4cd6d to
ddfa332
Compare
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
closing as this will be replaced by #2555 |
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
4 participants
unfinished WIP