You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "dennishuo (via GitHub)" <gi...@apache.org> on 2025/03/07 09:37:13 UTC
[PR] Core persistence refactor phase 2 - Distinguish "InCurrentTxn" variations of all BasePersistence methods in TransactionalPersistence [polaris]
dennishuo opened a new pull request, #1135:
URL: https://github.com/apache/polaris/pull/1135
Fully differentiating between e.g. `writeEntity` an `writeEntityInCurrentTxn` allows all subclasses of AbstractTransactionalPersistence now to automatically inherit the ability to also behave as one-shot durable `BasePersistence` implementations.
This also allows now incrementally pulling shared logic out of `PolarisMetaStoreManagerImpl` into `BaseMetaStoreManager` which can be reused by other non-transactional implementations of `PolarisMetaStoreManager`; the `BaseMetaStoreManager` will only ever interact with `BasePersistence` while `PolarisMetaStoreManagerImpl` will interact with `TransactionalPersistence`.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Core persistence refactor phase 2 - Distinguish "InCurrentTxn" variations of all BasePersistence methods in TransactionalPersistence [polaris]
Posted by "dennishuo (via GitHub)" <gi...@apache.org>.
dennishuo commented on code in PR #1135:
URL: https://github.com/apache/polaris/pull/1135#discussion_r1985957696
##########
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:
If we want to reduce the diffs on the existing `PolarisMetaStoreManagerImpl` class we could instead invert the semantics, as in unqualified `writeEntity` could already always mean "in current transaction" since the current implementations all have that semantic, and instead, we introduce the new methods in `AtomicPersistence` to have `writeEntityAtomically`, `dropEntityAtomically`, etc.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Core persistence refactor phase 2 - Distinguish "InCurrentTxn" variations of all BasePersistence methods in TransactionalPersistence [polaris]
Posted by "dennishuo (via GitHub)" <gi...@apache.org>.
dennishuo commented on PR #1135:
URL: https://github.com/apache/polaris/pull/1135#issuecomment-2712083268
Discussed offline; since this is subsumed by https://github.com/apache/polaris/pull/1139 "phase 3" where we introduce the whole working `AtomicOperationMetaStoreManager` I'll just close this PR and we can use the phase 3 as the combined update.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Core persistence refactor phase 2 - Distinguish "InCurrentTxn" variations of all BasePersistence methods in TransactionalPersistence [polaris]
Posted by "dennishuo (via GitHub)" <gi...@apache.org>.
dennishuo commented on PR #1135:
URL: https://github.com/apache/polaris/pull/1135#issuecomment-2708017784
@dimas-b Okay, I inverted the method naming so that `BasePersistence` defines all-new `doFooAtomically` methods, while `TransactionalPersistence` keeps the existing `doFoo` method names. This way there is no change in which method needs to be called from `PolarisMetaStoreManagerImpl` and indeed no changes to the TreeMap/EclipseLink implementations.
The nice thing here is the `AbstractTransactionalPersistence` just provides an adapter layer to make all those existing implementations also able to provide all the `Atomically` methods without them having to change at all.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Core persistence refactor phase 2 - Distinguish "InCurrentTxn" variations of all BasePersistence methods in TransactionalPersistence [polaris]
Posted by "dennishuo (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Core persistence refactor phase 2 - Distinguish "InCurrentTxn" variations of all BasePersistence methods in TransactionalPersistence [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1135:
URL: https://github.com/apache/polaris/pull/1135#discussion_r1985855340
##########
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 new methods seem to match methods in `BasePersistence` 1:1, which seems like a redundant copy.
How about we use `BasePersistence` to define API methods, parameters and version check expectations, but not attach any atomicity or durability guarantees to them?
Then, `AtomicPersistence extends BasePersistence` <- here we define that each method call is durable and atomic.
`TransactionPersistence extends BasePersistence` <- allows explicitly managing Tx boundaries and defines that changes are durable on Tx commit. WDYT?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Core persistence refactor phase 2 - Distinguish "InCurrentTxn" variations of all BasePersistence methods in TransactionalPersistence [polaris]
Posted by "dennishuo (via GitHub)" <gi...@apache.org>.
dennishuo commented on PR #1135:
URL: https://github.com/apache/polaris/pull/1135#issuecomment-2708694552
If it helps to reason about how the the method renaming structure fits in and why things like `writeEntityAtomically` will actually diverge more, see the next phase that diffbases on top of this one: https://github.com/apache/polaris/pull/1139
If it's preferred we could just close this PR and try to merge the "phase 3" one all as one shot instead.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Core persistence refactor phase 2 - Distinguish "InCurrentTxn" variations of all BasePersistence methods in TransactionalPersistence [polaris]
Posted by "dennishuo (via GitHub)" <gi...@apache.org>.
dennishuo closed pull request #1135: Core persistence refactor phase 2 - Distinguish "InCurrentTxn" variations of all BasePersistence methods in TransactionalPersistence
URL: https://github.com/apache/polaris/pull/1135
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Core persistence refactor phase 2 - Distinguish "InCurrentTxn" variations of all BasePersistence methods in TransactionalPersistence [polaris]
Posted by "dennishuo (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Core persistence refactor phase 2 - Distinguish "InCurrentTxn" variations of all BasePersistence methods in TransactionalPersistence [polaris]
Posted by "dennishuo (via GitHub)" <gi...@apache.org>.
dennishuo commented on code in PR #1135:
URL: https://github.com/apache/polaris/pull/1135#discussion_r1985957696
##########
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:
If we want to reduce the diffs on the existing `PolarisMetaStoreManagerImpl` class we could instead invert the semantics, as in unqualified `writeEntity` could already always mean "in current transaction" since the current implementations all have that semantic, and instead, we introduce the new methods in `AtomicPersistence` to have `writeEntityAtomically`, `dropEntityAtomically`, etc.
The problem with doing it inverted like that though is that the `BasePersistence` methods are automatically the "InCurrentTxn" variants, so a subclass that only implements AtomicPersistence would I suppose just throw `UnsupportedOperationException` for all the methods from `BasePersistence` which kind of defeats the purpose.
Alternatively, the `doFooAtomically` methods could be the methods declared in `BasePersistence`.
In general it makes sense to have the "Atomic" operations in the base interface and "Transaction" ones in the subinterface, because as [AbstractTransactionalPersistence.java](https://github.com/apache/polaris/pull/1135/files#diff-718b00bacadc702326318cb44c43f49659c96f1c6ae86b1267fa6300973e82ae) shows, all Transactional implementations can also achieve the semantics of "Atomic Persistence", but not all implementations of Atomic Persistence" can support transactional styles.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org