geniusjun opened a new pull request, #10335:
URL: https://github.com/apache/gravitino/pull/10335
### What changes were proposed in this pull request?
This PR updates `SessionUtils` to ensure transaction cleanup also runs when
callbacks throw non-`Exception` throwables.
The changes include:
- broaden the failure handling in `doWithCommit` from `Exception` to
`Throwable`
- apply the same pattern to `doWithCommitAndFetchResult`
- apply the same pattern to `doMultipleWithCommit`
- add unit tests to verify rollback happens when callbacks throw
`AssertionError`
With this change, `SqlSessions.rollbackAndCloseSqlSession()` is invoked
consistently for callback failures before the throwable is rethrown.
### Why are the changes needed?
`SessionUtils` previously caught only `Exception` in commit-managed helper
methods.
If a mapper callback threw a non-`Exception` throwable such as
`AssertionError`, the rollback path was skipped, which could leave the SQL
session or transaction state unclosed.
This PR fixes that bug by making rollback/close run for all throwables in
the callback path.
Fix: #10177
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added unit tests in `TestSessionUtils` to verify:
- `doWithCommit` rolls back and does not commit on `AssertionError`
- `doWithCommitAndFetchResult` rolls back and does not commit on
`AssertionError`
- `doMultipleWithCommit` rolls back and does not commit on `AssertionError`
Verified with:
```bash
env
JAVA_HOME=/opt/homebrew/Cellar/openjdk@17/17.0.16/libexec/openjdk.jdk/Contents/Home
\
./gradlew :core:test --tests
org.apache.gravitino.storage.relational.utils.TestSessionUtils -PskipITs
--
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]
