-
Notifications
You must be signed in to change notification settings - Fork 199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core persistence refactor phase 3 (diffbase on phase 2) - Add AtomicOperationMetaStoreManager which passes all tests #1139
Open
dennishuo
wants to merge
7
commits into
apache:main
Choose a base branch
from
dennishuo:dhuo-persistence-phase3-add-atomic-ops-metastore-manager
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+3,054
−302
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ManagerImpl into BaseMetaStoreManager
…r 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.
…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.
…etaStoreManager as a diffbase.
… 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).
…f 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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).
Add PolarisTreeMapAtomicOperationMetaStoreManagerTest which covers this new class
in polaris-core:test, and add InMemoryAtomicOperationMetaStoreManagerFactory
which allows making all integration tests and the default
gradle run
use the new atomicoperations implementation if
polaris.persistence.type=in-memory-atomic
.Add TODO comments for all methods that still require additional thought to achieve "strict"
correctness in the face of concurrency, but which aren't easily exercised by existing test cases.
With this initial implementation, all current tests pass, and the main four core mutation methods
are indeed "strictly" correct with compare-and-swap mechanics:
For all other methods, generally the baseline functionality is still "correct" in the face of
concurrency, but certain happens-before relationships are violated when grants interact with
entity mutations, until we fix the noted TODOs.
Additionally, some multi-entity methods (e.g. CreateCatalog which also creates a CatalogRole and
assigns grants), currently can leave a partial-creation state, until the TODOs are implemented to
change the ordering of operations appropriately.
The following is what I believe to be a comprehensive list of ways the current
AtomicOperationMetaStoreManager
deviates from the transactional
PolarisMetaStoreManagerImpl
in the face of concurrency andserver failures, none of which seem to violate typical user expectations beyond borderline nuisance 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 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,
if the server fails after updating grantRecords but before updating
grantRecordVersions
-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.