-
Notifications
You must be signed in to change notification settings - Fork 260
add checksum failure metrics (#1549) #1555
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
base: branch-3.0.x
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @singhravidutt, 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 enhances the monitoring capabilities of the GCS Connector by introducing a new metric to specifically count checksum failures during write operations. This provides crucial visibility into potential data corruption or integrity issues, allowing operators to quickly identify and address problems related to data persistence. The change integrates seamlessly with the existing statistics and event bus infrastructure. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
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 introduces a new metric for tracking write checksum failures. The implementation correctly uses the existing event bus mechanism by adding a new GCSChecksumFailureEvent and a subscriber to update the WRITE_CHECKSUM_FAILURE_COUNT statistic. The changes are logical and include a new test case to verify the functionality. I have a couple of minor suggestions to enhance code clarity and robustness.
| } | ||
|
|
||
| @Subscribe | ||
| private void onGcsChecksumFailure(GCSChecksumFailureEvent exception) { |
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.
The parameter name exception is a bit misleading here. Although GCSChecksumFailureEvent extends Exception, in this context, it's functioning as an event rather than a handled exception. Renaming it to event would improve code clarity and better reflect its purpose within the event bus system.
| private void onGcsChecksumFailure(GCSChecksumFailureEvent exception) { | |
| private void onGcsChecksumFailure(GCSChecksumFailureEvent event) { |
| package com.google.cloud.hadoop.util; | ||
|
|
||
| /** A thin class to emit checksum failure event for EventBus notification. */ | ||
| public class GCSChecksumFailureEvent extends Exception {} |
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.
To improve code robustness and clearly state its intent, this class should be declared as final. Since it serves as a simple marker event and is not designed for extension, making it final prevents subclassing.
| public class GCSChecksumFailureEvent extends Exception {} | |
| public final class GCSChecksumFailureEvent extends Exception {} |
|
/gcbrun |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## branch-3.0.x #1555 +/- ##
==================================================
+ Coverage 82.03% 82.61% +0.58%
- Complexity 1972 2181 +209
==================================================
Files 114 120 +6
Lines 8615 9428 +813
Branches 1004 1130 +126
==================================================
+ Hits 7067 7789 +722
- Misses 1121 1173 +52
- Partials 427 466 +39
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dheerajsngh
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.
Added checksum failure metric. This will capture the number of failures per FS instance.