[#7744] improvement(core): allow nested SqlSession call#8242
[#7744] improvement(core): allow nested SqlSession call#8242mchades merged 3 commits intoapache:mainfrom
Conversation
|
@yuqi1129 could you please help to review this PR? thanks! |
| sessionCount.remove(); | ||
| } | ||
| } else if (count < 0) { | ||
| // This should not happen if the session management is correct. |
There was a problem hiding this comment.
This should not happen, unless an unknown bug is introduced.
core/src/main/java/org/apache/gravitino/storage/relational/service/UserMetaService.java
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/storage/relational/session/SqlSessions.java
Outdated
Show resolved
Hide resolved
|
Can the PR also fix the one #8210 that tries to? |
yes, the new commit I pushed can fixed that |
| sessions.set(sqlSession); | ||
| return sqlSession; | ||
| } | ||
| sessionCount.set(sessionCount.get() + 1); |
There was a problem hiding this comment.
This is not atomic, you'd better use AtomicInteger.
There was a problem hiding this comment.
Thanks for the suggestion. While the ThreadLocal already ensures thread safety here, I agree that using AtomicInteger makes the intent of a counter more explicit and the code cleaner. I've made the change.
| sessions.remove(); | ||
| } | ||
| } | ||
| handleSessionClose(true, false); |
There was a problem hiding this comment.
Can you change all the code style to handleSessionClose(true /** commit */, false /** rollback */); for better review.
|
@yuqi1129 can you please take another review? |
I don't have any more valuable comments. |
…#8242) ### What changes were proposed in this pull request? - add sessionCount for lifecycle management ### Why are the changes needed? Fix: apache#7744 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? tests added
…#8242) ### What changes were proposed in this pull request? - add sessionCount for lifecycle management ### Why are the changes needed? Fix: apache#7744 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? tests added
…#8242) ### What changes were proposed in this pull request? - add sessionCount for lifecycle management ### Why are the changes needed? Fix: apache#7744 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? tests added
3 participants
What changes were proposed in this pull request?
Why are the changes needed?
Fix: #7744
Does this PR introduce any user-facing change?
no
How was this patch tested?
tests added