-
Notifications
You must be signed in to change notification settings - Fork 40
Improve logging in Consensus Commit #2915
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
Conversation
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 improves observability and debugging capabilities in the Consensus Commit module by enhancing logging throughout the codebase. The changes add detailed transaction context to error and warning messages, providing better debugging information when issues occur.
Key changes:
- Enhanced error messages to include specific transaction details like Transaction IDs, table names, and partition/clustering keys
- Improved exception handling with more detailed error messages and better context propagation
- Refactored NoMutationException to include mutation details for better error reporting
Reviewed Changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| RecoveryHandlerTest.java | Updated test setup to mock additional transaction metadata components |
| RecoveryExecutorTest.java | Fixed test to use proper transaction ID in coordinator state mocking |
| TwoPhaseConsensusCommitManager.java | Added transaction ID to rollback failure warning logs |
| TwoPhaseConsensusCommit.java | Enhanced error messages with exception details and replaced snapshot ID calls with direct getId() |
| Snapshot.java | Added transaction ID to scanner closure warning logs |
| RecoveryHandler.java | Extensively improved logging with detailed table, key, and transaction information |
| RecoveryExecutor.java | Enhanced error messages and added detailed uncommitted record exception context |
| ParallelExecutor.java | Improved task naming and added exception details to parallel execution logs |
| CrudHandler.java | Enhanced error handling and scanner closure logging with transaction context |
| Coordinator.java | Improved retry logic with better exception handling and detailed logging |
| ConsensusCommitMutationOperationChecker.java | Refactored to use utility method for table metadata retrieval |
| ConsensusCommitManager.java | Added transaction ID to rollback failure warning logs |
| ConsensusCommit.java | Enhanced error messages and replaced snapshot ID calls with direct getId() |
| CommitMutationComposer.java | Added assertion to validate transaction state |
| CommitHandler.java | Comprehensive error message improvements with exception details |
| JdbcDatabase.java | Updated NoMutationException constructor calls to include mutation details |
| Storage handlers | Updated NoMutationException usage across DynamoDB, Cosmos, and Cassandra handlers |
| NoMutationException.java | Major refactor to include mutation details and enhanced error messaging |
| CoreError.java | Updated error message templates to include placeholders for detailed information |
Comments suppressed due to low confidence (2)
core/src/main/java/com/scalar/db/transaction/consensuscommit/Coordinator.java:326
- Missing closing brace for the while loop. The code structure shows an incomplete while loop that needs proper closure.
}
core/src/main/java/com/scalar/db/transaction/consensuscommit/Coordinator.java:372
- Missing closing brace for the while loop. The code structure shows an incomplete while loop that needs proper closure.
}
core/src/test/java/com/scalar/db/transaction/consensuscommit/RecoveryExecutorTest.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.
Summary of Changes
Hello @brfrn169, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the logging and error reporting capabilities within the Consensus Commit transaction framework. The primary goal is to improve observability and debugging by providing more detailed and contextual information in error messages and log outputs. This includes enriching exception details, adding transaction-specific identifiers to logs, and refining the handling of internal exceptions to offer clearer insights into transaction states and failures.
Highlights
- Enhanced Error Messages: Many
CoreErrormessages related to Consensus Commit have been updated to include%splaceholders, allowing for dynamic insertion of specific details. Notably, theCONSENSUS_COMMIT_READ_UNCOMMITTED_RECORDerror now provides comprehensive information including the table, partition key, clustering key, and the transaction ID that wrote the record, significantly aiding debugging. - Improved NoMutationException Details: The
NoMutationExceptionclass has been refactored to accept and store the list ofMutationobjects that failed to apply. This enhancement allows the exception message to include detailed information about the specific mutations (type, table, and keys), making it much easier to diagnose issues when conditional mutations do not satisfy their conditions. - Detailed Logging Across Components: Log messages throughout various Consensus Commit components, including
CommitHandler,ConsensusCommit,CrudHandler,Coordinator,ParallelExecutor, andRecoveryHandler, have been enriched. They now consistently include transaction IDs and more specific error details, greatly improving the observability and traceability of transaction flows. - Refined Exception Handling and Retries: Exception handling in the
CoordinatorandCrudHandlerhas been improved to aggregate suppressed exceptions, providing a more complete picture of the error chain during retries or recovery processes. This helps in understanding the root cause of transient failures. - Adjusted Logging Levels for Expected Flows: Some
logger.warncalls that previously indicated expected or non-critical scenarios (such as certain lazy recovery operations) have been changed tologger.info. This adjustment reduces noise in the logs, allowing developers to focus on genuinely problematic warnings while still retaining visibility into normal operational flows.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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 significantly improves logging and observability within the Consensus Commit protocol. Key changes include:
- Enhancing error messages in
CoreErrorto include detailed, context-specific information. - Refactoring
NoMutationExceptionto include a list of mutations that failed, providing crucial debugging information. - Improving retry logic in the
Coordinatorwith better exception handling and more informative logging. - Adding transaction IDs to numerous log messages for easier tracing.
- Refactoring code for better organization, such as moving utility methods to
ConsensusCommitUtils.
The changes are well-implemented and will be very beneficial for debugging and monitoring. I have a couple of minor suggestions for improving code style and consistency.
core/src/main/java/com/scalar/db/exception/storage/NoMutationException.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/exception/storage/NoMutationException.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandler.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandler.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/RecoveryHandler.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/exception/storage/NoMutationException.java
Show resolved
Hide resolved
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!
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!
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!
Description
This PR improves logging in Consensus Commit to improve observability and debugging.
Related issues and/or PRs
N/A
Changes made
Checklist
Additional notes (optional)
N/A
Release notes
N/A