kitoha commented on PR #8210:
URL: https://github.com/apache/gravitino/pull/8210#issuecomment-3226638813
hi @yuqi1129 .
Thanks for the quick reviews.
The changes related to this PR’s issue have been merged and the problem
appears resolved. However, I believe the following behavior still needs
attention.
When `doMultipleWithCommit` wraps multiple calls to `doWithoutCommit` (or
`getWithoutCommit`), if `getWithoutCommit` closes the session and clears the
`ThreadLocal`, the batch no longer runs under a single owner/session.
Concretely:
```java
SessionUtils.doMultipleWithCommit(
// A ThreadLocal-bound session is created here (owner:
doMultipleWithCommit)
() -> SessionUtils.doWithoutCommit(
// Uses the session created above, then closes it and clears the
ThreadLocal
TopicMetaMapper.class, mapper ->
mapper.softDeleteTopicMetasByTopicId(topicId)),
() -> SessionUtils.doWithoutCommit(
// Because the previous call cleared the ThreadLocal, a new
session/ThreadLocal is created
OwnerMetaMapper.class,
mapper -> mapper.softDeleteOwnerRelByMetadataObjectIdAndType(
topicId, MetadataObject.Type.TOPIC.name())),
() -> SessionUtils.doWithoutCommit(
SecurableObjectMapper.class,
mapper -> mapper.softDeleteObjectRelsByMetadataObject(
topicId, MetadataObject.Type.TOPIC.name())),
() -> SessionUtils.doWithoutCommit(
TagMetadataObjectRelMapper.class,
mapper -> mapper.softDeleteTagMetadataObjectRelsByMetadataObject(
topicId, MetadataObject.Type.TOPIC.name())),
() -> SessionUtils.doWithoutCommit(
StatisticMetaMapper.class,
mapper -> mapper.softDeleteStatisticsByEntityId(topicId)),
() -> SessionUtils.doWithoutCommit(
PolicyMetadataObjectRelMapper.class,
mapper -> mapper.softDeletePolicyMetadataObjectRelsByMetadataObject(
topicId, MetadataObject.Type.TOPIC.name()))
);
```
In this flow:
`doMultipleWithCommit` creates/binds the session in a `ThreadLocal`
(becoming the lifecycle owner).
The first `doWithoutCommit`/`getWithoutCommit` uses that session but then
closes it and clears the `ThreadLocal`.
Subsequent lambdas run with new sessions/`ThreadLocals`.
The final commit/close in `doMultipleWithCommit` no longer owns the original
session for the earlier operations, which defeats the expectation that the
whole batch is executed atomically under one session/transaction.
On the other hand, if we change `getWithoutCommit` so that it doesn’t
close/clear when a session is already bound, then standalone calls to
`doWithoutCommit`/`getWithoutCommit` risk leaving the `ThreadLocal` uncleared,
which is also problematic.
What’s your preference here? If you think no further changes are necessary,
I’m fine with closing this PR as-is. Otherwise, I’m happy to adjust the
lifecycle rules (e.g., make `doMultipleWithCommit` the only lifecycle owner and
ensure getWithoutCommit `is no-op for close/clear when a session is already
bound, and handle standalone calls accordingly). Thanks!
--
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]
