dennishuo commented on code in PR #1135:
URL: https://github.com/apache/polaris/pull/1135#discussion_r1985932490
##########
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:
The old behavior was already effectively that, except that the semantics
were just implied in javadocs rather than an interface; do you mean you'd want
`AtomicPersistence` to basically be just an indicator interface that doesn't
necessarily add new methods?
public interface AtomicPersitence extends BasePersistence {}
I had tried to make it work initially without renaming the methods, but I
kept running into a few issues
1. If anything is calling things through the `BasePersistence` interface,
such as `BaseMetaStoreManager` we don't know if the method call is intended to
be inside an outer transaction or not, since the helpers in
BaseMetaStoreManager could also be used by atomic or transactional impls of
PolarisMetaStoreManager
2. Even within a "transactional" implementation, in particular when
inheriting shared methods from `BaseMetaStoreManager`, we have a situation
where we want to use atomic methods sometimes, and "InCurrentTxn" other times
with the same instance of a `TransactionalPersistence`. To achieve this the
other way we'd need a composition/delegator pattern where an
`AtomicPersistence` impementation implements all the "atomic" versions by
explicit runInTransaction just like the `AbstractTransactionalPersistence` does
here, but would have to provide the handle to the underlying raw
`TransactionalPersistence` anyways when a caller wants to call in a transaction.
With this PR's approach, we ensure very explicit semantics for each method
definition, regardless of whether the methods were from the base interface or
the TransactionalPersistence interface.
Also importantly, this approach allowed all current TransactionalPersistence
impls to be able to function as a `AtomicPersistence` as you describe, but
without any ambiguity, which made it easy to test.
Another thing to consider is that even for the transactional
implementations, the "silent `runInTransaction` approach is pretty suboptimal,
because it's not possible to manage transaction state explicitly, and depends
on a ThreadLocal which isn't great (if someone tries to get cute and
parallelize some validation fan-out requests against the EclipseLink impl right
now, transactions break).
This refactoring could be a step towards the more idiomatic approach of
adding a `Transaction` argument to the *InCurrentTxn versions of the method
calls, where the Session/Transaction intended to be used in a given method call
is unambiguous.
It looks like our EclipseLink impl actually *could've* been done that way,
since it has an explicit `EntityManager session` which represents the live
transaction, but only had to use ThreadLocal because the interfaces were not
well-suited for "runInTransaction" semantics.
--
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]
