Add atomic multi-entity and grant-record commit SPI#5035
Conversation
github-project-automation
Bot
moved this from PRs In Progress
to Done
in Basic Kanban Board
github-project-automation
Bot
moved this from Done
to PRs In Progress
in Basic Kanban Board
…I change As per review feedback, this replaces the broad BasePersistence SPI change (writeEntitiesAndGrantRecords) with a narrower JDBC-internal fix: - Remove writeEntitiesAndGrantRecords from BasePersistence (revert SPI change) - Add BasePersistence.flush() as a default no-op method - In JdbcBasePersistenceImpl, batch consecutive write operations in a ThreadLocal transaction; read operations and flush() commit it - AtomicOperationMetaStoreManager calls ms.flush() after sequences of writes that should be atomic (grant/revoke, createCatalog, dropEntity) This achieves the same atomicity for JDBC without adding RBAC-specific methods to the generic persistence SPI. NoSQL/transactional backends are unaffected since flush() is a no-op for them. Fixes apache#5035
0e34b06 to
0a79a4c
Compare
|
@flyingImer thanks for checking, but I've reworked per community feedback...apologies, writeEntities default-method suggestion is no longer needed since that method is gone.. FYI : https://lists.apache.org/thread/hp0y4j30ooztc01n3qz4rm7roczozror from @snazy |
| * <p>Callers that perform a sequence of write operations that should be atomic should invoke this | ||
| * method after the last write in the sequence. | ||
| */ | ||
| default void flush() { |
There was a problem hiding this comment.
The documented contract of BasePersistence says:
Each method in this interface must be atomic, meaning that write operations must either fully succeed with all changes applied, or fail entirely without partial updates.
With that, there must be no "pending changes" left, and no need for some "commit" or "flush."
There was a problem hiding this comment.
@snazy ahh..I get it! flush() was a mistake.. i've reworked to remove flush() and instead add commitChangeSet() to BasePersistence. this new method groups all mutations (entity creates/updates/deletes + grant creates/deletes) into a single atomic call, so each SPI method remains self-contained and atomic. default throws UnsupportedOperationException, so existing backends are unaffected..
Grant operations in AtomicOperationMetaStoreManager now build a ChangeSet internally and use commitChangeSet() when the backend supports it (JDBC), falling back to individual operations otherwise. no more ThreadLocal, no more flush :) thank you for checking this!
|
Thanks for looking into this area of the codebase, @iprithv ! From my POV we need a more holistic change to the Persistence SPI and that will probably need a Starting with individual PRs seems like introducing more complexity by trying to address the multi-object change project in narrow areas of the SPI surface. I believe we need to simplity the Persistence SPI by reducing the interface surface and introducing higher level concepts for grouping multiple objects into the same atomic change. Cf. #4939 (comment) |
…commits This redesigns the approach to making grant/catalog operations atomic: - Removed BasePersistence.flush() which violated the per-call atomicity contract - Added BasePersistence.commitChangeSet() for atomic mixed entity+grant commits - Added MetaStoreChangeSet, EntityMutation, GrantMutation records - Added PolarisMetaStoreManager.commitTransactionBatch() as primary mutation path - Refactored AtomicOperationMetaStoreManager grant ops to use commitChangeSet - Removed ThreadLocal<Connection> hack from JdbcBasePersistenceImpl - Each BasePersistence call is now self-contained and atomic
|
@dimas-b Thanks, I think this revised version aligns with what you were asking for. this is still like first step... it doesn't yet refactor all single-entity methods to ride on commitTransactionBatch, and it doesn't unify AtomicOperationMetaStoreManager with TransactionalMetaStoreManagerImpl. but it establishes the API foundation and makes grant operations atomic on JDBC without adding RBAC-specific methods to the SPI. I also posted a design mail to dev@ to gather broader feedback: https://lists.apache.org/thread/mslr6n7194qsotrlv26wqooo2wggxc8n WDYT, does this direction work? |
|
Thanks for pushing Polaris in this direction, @iprithv ! I support the general idea behind this change. However, I think it might be time to have a discussion about this kind of change on How critical is this change for your use cases? |
4 participants
AtomicOperationMetaStoreManager grant, revoke, createCatalog, and dropEntity previously performed multiple independent writeEntity / writeToGrantRecords / deleteEntity calls. For JDBC, each call was its own transaction, so a failure mid-sequence could leave partial state (entity without grants, grant without version bumps, catalog without admin role, etc.).