Add close for storage, transaction in data loader core #2699
Conversation
ypeckstadt
requested review from
a team,
Torch3333,
brfrn169,
Copilot,
feeblefakie,
komamitsu and
ypeckstadt
and removed request for
a team
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that the distributed storage and distributed transaction manager objects are properly closed after the import process completes. Key changes include:
- Adding the closeResources() method to handle resource closure.
- Wrapping listener notifications and resource closure in try-catch blocks with exception suppression.
- Improving exception handling to propagate any errors encountered during the process.
| /** {@inheritDoc} Forwards the event to all registered listeners. */ | ||
| @Override | ||
| public void onAllDataChunksCompleted() { | ||
| Throwable firstException = null; |
There was a problem hiding this comment.
Catching Throwable may inadvertently capture critical errors (e.g., OutOfMemoryError). Consider catching Exception to handle only recoverable errors.
|
|
||
| /** Close resources properly once the process is completed */ | ||
| public void closeResources() { | ||
| try { |
There was a problem hiding this comment.
Catching Throwable in the closeResources method might hide serious errors. Consider catching Exception instead to avoid masking unrecoverable issues.
brfrn169
left a comment
brfrn169
left a comment
There was a problem hiding this comment.
@inv-jishnu Don't we need to update the unit tests?
|
@brfrn169 san, |
Co-authored-by: Peckstadt Yves <[email protected]>
Co-authored-by: Peckstadt Yves <[email protected]>
Co-authored-by: Peckstadt Yves <[email protected]>
Co-authored-by: Peckstadt Yves <[email protected]>
Co-authored-by: Peckstadt Yves <[email protected]>
5 participants
Description
This PR ensures that the distributed storage and distributed transaction manager objects are properly closed once the import process is completed.
These changes address the feedback provided in the following comments from PR #2618:
Discussion 1
Discussion 2
Related issues and/or PRs
NA
Changes made
Checklist
Additional notes (optional)
NA
Release notes
NA