-
Notifications
You must be signed in to change notification settings - Fork 40
Fix exception handling in the data loader importing to handle the unkown exceptions properly #3183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix exception handling in the data loader importing to handle the unkown exceptions properly #3183
Conversation
…own exceptions properly
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively addresses a critical issue in the data loader's exception handling by catching unexpected exceptions during transaction processing, ensuring transactions are properly aborted, and improving error logging. The changes are logical and the inclusion of a unit test to verify the new behavior is commendable. I have a few suggestions to further enhance the code's robustness and maintainability.
...r/core/src/main/java/com/scalar/db/dataloader/core/dataimport/processor/ImportProcessor.java
Show resolved
Hide resolved
...re/src/test/java/com/scalar/db/dataloader/core/dataimport/processor/ImportProcessorTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances exception handling in the data loader's transaction processing to properly catch and handle unexpected exceptions (non-TransactionException) that can occur with misconfigured transaction managers. The changes ensure transactions are properly aborted and errors are logged with appropriate context even when unexpected exceptions occur.
Key Changes:
- Added a generic
catch (Exception e)block to handle unexpected exceptions during transaction processing - Enhanced error logging to include batch ID and data chunk ID for better debugging context
- Added unit test to verify graceful handling of unexpected exceptions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/processor/ImportProcessor.java |
Added generic exception handler with enhanced logging and proper transaction abort logic for unexpected exceptions |
data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/processor/ImportProcessorTest.java |
Added unit test process_withUnexpectedExceptionInTransaction_shouldHandleGracefully() to verify proper handling of unexpected exceptions in transaction mode |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...re/src/test/java/com/scalar/db/dataloader/core/dataimport/processor/ImportProcessorTest.java
Show resolved
Hide resolved
...r/core/src/main/java/com/scalar/db/dataloader/core/dataimport/processor/ImportProcessor.java
Show resolved
Hide resolved
brfrn169
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
ypeckstadt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you.
inv-jishnu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thank you!
Torch3333
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
...r/core/src/main/java/com/scalar/db/dataloader/core/dataimport/processor/ImportProcessor.java
Outdated
Show resolved
Hide resolved
… fix/data-loader/handle-unkown-exception-when-importing
komamitsu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
...r/core/src/main/java/com/scalar/db/dataloader/core/dataimport/processor/ImportProcessor.java
Show resolved
Hide resolved
feeblefakie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
...r/core/src/main/java/com/scalar/db/dataloader/core/dataimport/processor/ImportProcessor.java
Show resolved
Hide resolved
…own exceptions properly (#3183)
…own exceptions properly (#3183)
Description
This PR addresses the issue of unexpected exceptions (non-
TransactionException) are not properly caught when importing data in transaction mode with misconfigured transaction managers, causing the import process to fail ungracefully.For example, in the
STORAGEmode of importing:single-crud-operationtransaction manager throwsUnsupportedOperationExceptionwhen starting a transactiongrpc.StatusRuntimeException: UNIMPLEMENTEDon the client side (using scalardb-cluster-data-loader)These exceptions bypass the existing
TransactionExceptionhandler, leaving transactions unaborted and errors unlogged with proper context.Related issues and/or PRs
N/A
Changes made
catch (Exception e)block inImportProcessor.processTransactionBatch()to handle unexpected exceptions"Unexpected error: <ExceptionClass> - <message>"process_withUnexpectedExceptionInTransaction_shouldHandleGracefully()to verify the new exception handlingChecklist
Additional notes (optional)
NA
Release notes
Fixed data-loader to properly handle unexpected exceptions during transaction processing for importing, ensuring transactions are aborted and errors are logged with proper context.