dennishuo commented on code in PR #1135:
URL: https://github.com/apache/polaris/pull/1135#discussion_r1985938607
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalPersistence.java:
##########
@@ -88,9 +100,214 @@ void runActionInReadTransaction(
* @return the list of entityActiveKeys for the specified lookup operation
*/
@Nonnull
- List<EntityNameLookupRecord> lookupEntityActiveBatch(
+ List<EntityNameLookupRecord> lookupEntityActiveBatchInCurrentTxn(
@Nonnull PolarisCallContext callCtx, List<PolarisEntitiesActiveKey>
entityActiveKeys);
/** Rollback the current transaction */
void rollback();
+
+ //
+ // Every method of BasePersistence will have a related *InCurrentTxn method
here; the semantics
+ // being that transactional implementations of a PolarisMetaStoreManager may
choose to
+ // self-manage outer transactions to perform all the persistence calls
within that provided
+ // transaction, while the basic implementation will only use the "durable in
a single-shot"
+ // methods from BasePersistence.
Review Comment:
Oh another nice benefit I noticed when doing this was even for methods that
aren't 1:1 from `BasePersistence`, I found it *much* easier to reason about
what was happening by still following the naming convention, e.g.
`writeToEntitiesChangeTrackingInCurrentTxn` so that it's easy to see at a
glance whether all the method calls indeed abide by being "in a current
transaction". This is also something that will benefit from actually providing
an actual Transaction state object for these variations (or in that case, it's
not as strictly necessary for the methods to be named as `InCurrentTxn`, but
instead having `(Transaction txn...` in the method signature would serve the
same purpose of disambiguation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
