yuqi1129 commented on PR #8210:
URL: https://github.com/apache/gravitino/pull/8210#issuecomment-3226662929
> @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.
>
> ```java
> public static <T, R> R getWithoutCommit(Class<T> mapperClazz,
Function<T, R> func) {
> try {
> T mapper = SqlSessions.getMapper(mapperClazz);
> return func.apply(mapper);
> } finally {
> // This will decrement the counter, the session is closed only when
the counter is 0.
> SqlSessions.closeSqlSession(); // close session and remove
threadlocal
> }
> }
> ```
>
> 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 for `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 and 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. Thanks!
`doWithoutCommit` is warpped by `doWithoutCommit`, since we have use
reference count mechanims to manage the sql session, only when the count is 0
then we will close the sql session.
```
public static void doMultipleWithCommit(Runnable... operations) {
// This method acts as the outermost transaction boundary.
// It increments the session count once.
SqlSessions.getSqlSession(); // reference count = 1
try {
Arrays.stream(operations).forEach(Runnable::run); // refernce count
then increase and decrease
SqlSessions.commitAndCloseSqlSession(); // refernce count is 1 here,
then we commit or rollback
} catch (Exception e) {
SqlSessions.rollbackAndCloseSqlSession();
throw e;
}
}
}
```
So the whole logic seems to be okay.
--
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]
