Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Core persistence refactor phase 3 (diffbase on phase 2) - Add AtomicO…
…perationMetaStoreManager which passes all tests (#1139) * Initial sketch of how shared code can be lifted from PolarisMetaStoreManagerImpl into BaseMetaStoreManager * Make all TransactionalPersistence methods explicit in terms of whether they are one-shot atomic or if they are intended to be run as part of a caller-managed transaction, in which case the action will not be flushed/durable until the transaction is committed. Delegate all the BasePersistence methods to subclass variations of *InCurrentTxn so that the TransactionalPersistence implementations also happen to fully implement the real semantics of BasePersistence. * Invert the method naming semantics; "doFooAtomically" is now the set of "new" methods defined in `BasePersistence` while the original "doFoo" will mean that it expects to be run in a current transaction. Removed all the InCurrentTxn suffixes. * Copy/paste PolarisMetaStoreManagerImpl.java to a new AtomicOperationMetaStoreManager as a diffbase. * Add AtomicOperationMetaStoreManager class which starts as just a full copy/paste of PolarisMetaStoreManagerImpl, but remove all its usage of runInTransaction instead only using the "doFooAtomically" BasePersistence methods. Implement the intended compare-and-swap semantics in writeEntity/writeEntities in AbstractTransactionalPersistence so that the TreeMap and EclipseLink impls can both actually successfully be used in "single-durable-operation" mode as plain BasePersistence implementations (e.g. without using their runInTransaction capabilities at all). * Update all concurrency TODO comments with more thorough explanation of implications of failure modes in the absence of additional bulk methods,. Reorder the internals of dropEntity to drop the entity first, instead of grants, so that the drop attempt can't put the entity into a partially-broken but still existing state; instead, server failures just might cause garbage grant records which are safe/expected but need to be cleaned up in a background task somewhere. Fix the atomicity of name-collision-checks in createCatalog and createPrincipal. With these changes, the behavior of AtomicOperationMetaStoreManager should be fully "safe"/correct, with only the following suboptimal scenarios: -If the server fails during CreateCatalog, it's possible for a partially-initialized catalog (e.g. lacking catalog_admin and grants) to exist; this should just be deleted and the creation should be retried in such a case. -If grants on parents of an entity (e.g. namespaces, catalogs) are changed in the middle of an operation on the child entity, it's possible for happens-before semantics to be violated up to the duration of a single in-flight request if grants are not cached, and up to the duration of cache expiration in the worse case if the server crashes after updating grants but before updating grantRecordVersions on affected entities. -If a parent entity (e.g. namespace, catalog) is deleted in the middle of creating or updating a child entity, the create/update may still return "success" despite the parent being deleted, which effectively deletes the child entity as well. -If a new entity is being created under an empty namespace/catalog at the same time the parent namespace/catalog is being deleted, despite the semantic of preventing deletion of non-empty containers, the creation may succeed *and* the deletion of the parent may succeed, effectively deleting the child entity as well; this can only happen within the concurrency window of a single request (e.g. happening within milliseconds of each other). -In general, in the face of server failures or concurrency collisions, grantRecords may be stale up to the lifetime of EntityCache elements. -In general, whether or not there are server failures or concurrency collisions, the grantsOnSecurable and grantsToGrantee of a given entity may not be evaluated at the same effective SNAPSHOT_READ point in time, but this shouldn't be a problem because the securable side of grants can already be modified independently from the grantee side of grants. * Step 1 of inverting the semantics again so that we only define "InCurrentTxn" versions of methods in TransactionalPersistence. The fact that this unit compiles before we've renamed doFooAtomically back to doFoo helps prove that we didn't leave anything accidentally behind to call the atomic versions inside a transaction, because as of this moment there is not unqualified method name doFoo anymore; only either doFooInCurrentTxn or doFooAtomically. Committing this as a separate unit to have the intermediate proof-of-correctness. * Step 2 of inverting the method names; remove the "Atomically" suffix from all method names; now all the unqualified methods in BasePersistence are implicitly supposed to behave "atomically". All the methods that expect to be run inside a transaction that's managed by the outer caller will have the "InCurrentTxn" suffix. * Update javadoc for writeEntities per suggestion from offline discussion * From offline discussion, make PolarisCallContext just return a BasePersistence instead of a TransactionalPersistence. Just use blind casts for now in PolarisMetaStoreManagerImpl; refactoring can be done later to better define how the Persistence impl is injected in a more typesafe way for compatible PolarisMetaStoreManager impls. * Restore application[-it].properties to default in-memory impl for now instead of using the atomic one.
- Loading branch information
