-
Notifications
You must be signed in to change notification settings - Fork 509
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
HDDS-10206. Expose jmx metrics for snapshot cache size on the ozone manager. #6138
Conversation
@swamirishi Can you please review when you have time? |
@aswinshakil Can you please take a look when you can? |
@ceekay47 please take a look at compile error:
https://github.com/ceekay47/ozone/actions/runs/7883419762/job/21510351002#step:6:2382 |
@adoroszlai Thanks for taking a look. Fixed and updated the PR. |
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMetrics.java
Outdated
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.
Thanks @ceekay47 for the patch.
I don't think it is a good idea to update metrics like this. It would be better if you increment and decrement the snapshotCacheSize metric whenever you open a new rocksDB instance and close it respectively inside the SnapshotCache
.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
Outdated
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.
@ceekay47 Thanks for the patch I had some nitpicky comments otherwise the patch looks good.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMetrics.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMetrics.java
Show resolved
Hide resolved
@hemantk-12 @swamirishi Thanks for the review. I have addressed the review comments. Can you take a look? Thanks. |
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.
Thanks @ceekay47 for addressing the comments.
Left a comment about updating tests to validate metrics and a minor comment about removing the white line. Otherwise, change looks good to me.
...ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
Outdated
Show resolved
Hide resolved
@hemantk-12 Thanks again for the review. I've added validation for the metric in tests. |
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.
Thanks @ceekay47 for working on this.
LGTM.
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.
Thanks for the patch @ceekay47 LGTM
…anager. (apache#6138) (cherry picked from commit 301664e)
…anager. (apache#6138) (cherry picked from commit 301664e)
…anager. (apache#6138) (cherry picked from commit 301664e)
…anager. (apache#6138) (cherry picked from commit 301664e)
…anager. (apache#6138) (cherry picked from commit 301664e)
…anager. (apache#6138) (cherry picked from commit 301664e)
What changes were proposed in this pull request?
HDDS-10206. Expose jmx metrics for snapshot cache size on the ozone manager.
This PR introduces changes to expose snapshot cache size (
dbMap.size()
in https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java) to the Ozone Manager metrics.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10206
How was this patch tested?
Workflow run on the fork git repo.