HBASE-27966 HBase Master/RS JVM metrics populated incorrectly #5323
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Hi @apurtell could you please help review this? |
| @@ -24,10 +24,12 @@ | |||
| @InterfaceAudience.Private | |||
| public class MetricsIO { | |||
|
|
|||
| private static MetricsIO instance; | |||
There was a problem hiding this comment.
This probably should be volatile. Below in your new method, you should do synchronization if it's null so it only gets instantiated once. Example: https://github.com/apache/hbase/blob/master/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcClientConfigHelper.java#L87
There was a problem hiding this comment.
This probably should be volatile. Below in your new method, you should do synchronization if it's null so it only gets instantiated once. Example: https://github.com/apache/hbase/blob/master/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcClientConfigHelper.java#L87
Update the logic as per suggestion.
Done, please review. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Reidddddd
reviewed
Aug 2, 2023
| private final MetricsIOSource source; | ||
| private final MetricsIOWrapper wrapper; | ||
|
|
||
| public MetricsIO(MetricsIOWrapper wrapper) { | ||
| // visible for testing only |
There was a problem hiding this comment.
could use the annotation @VisibleForTesting?
There was a problem hiding this comment.
I initially thought to add same but checked code and found this is banned now. See
Line 2535 in f6c5dbe
So added a comment "visible for testing only" as done at other places in code base. See https://github.com/search?q=repo%3Aapache%2Fhbase+%22visible+for+testing%22&type=code
There was a problem hiding this comment.
Could use RestrictedApi to restrict the place where we can call this method. It will cause a compilation error if called from other places in the error prone check.
There was a problem hiding this comment.
Done with commit 0a7b561 and updated PR.
Reidddddd
reviewed
Aug 2, 2023
| private final MetricsIOSource source; | ||
| private final MetricsIOWrapper wrapper; | ||
|
|
||
| public MetricsIO(MetricsIOWrapper wrapper) { | ||
| // visible for testing only | ||
| MetricsIO(MetricsIOWrapper wrapper) { |
There was a problem hiding this comment.
weird here, here is implementing a singleton, but there is still a public constructor...
is it used somewhere? if not, could we just delete it.
There was a problem hiding this comment.
Yes it is being used in a test case, hence had to keep it and changed scope from public
|
Thank you for the reviews @bbeaudreault and @Reidddddd. |
bbeaudreault
approved these changes
Aug 10, 2023
|
After checking the code, I think we should change the implementation for DefaultMetricsSystemInitializer? It should be called when we create HMaster or HRegionServer, not in BaseSourceImpl? Have you read the commit history about why we just add a call in the Constructor of BaseSourceImpl? It looks strange to initialize JvmMetrics with a metrics name instead of the actual process name. Thanks. |
Hi @Apache9 thanks for your input. I had put up an RCA at https://issues.apache.org/jira/browse/HBASE-27966?focusedCommentId=17741087&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17741087. One of the proposed solution was to do following (let's call this approach 1) :
But based on discussion went ahead with a simpler fix, i.e. approach 2, which was as follow:
and hence I did not try to see the details of how to fix using approach 1. Could we work on approach 1 with another ticket after taking this as a stop-gap? Or do you want me to switch to approach 1 and revert the code changes done here? Please let me know how we should move ahead. I will work on following review comment:
based on what we decide here |
|
I think we should finally go with approach 1 but I'm also OK for having a quick fix. So if you have already tested the patch on your deployment and it does work, I'm OK with merging the current PR first. But let's open an issue for approach 1 first so we will not forget to do it. Thanks. |
|
@Apache9 Thanks for the quick response.
Yes this has been tested E2E in our deployment along with the dependent component i.e. ambari.
Sure let me create a JIRA for the same for follow up work. Also will update this PR with the review comments' fix as another commit here. |
|
Created https://issues.apache.org/jira/browse/HBASE-28026 for follow up work |
Apache9
approved these changes
Aug 16, 2023
|
Addressed review comment and updated PR with commit 0a7b561 |
|
💔 -1 overall
This message was automatically generated. |
…hadoop.hbase.io.MetricsIO#MetricsIO(org.apache.hadoop.hbase.io.MetricsIOWrapper)
|
Over-wrote last commit to also take care of adding the file MetricIO.java itself into list of restricted paths, which was missed earlier. See diff. (My bad, used |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
NihalJain
added a commit
to NihalJain/hbase
that referenced
this pull request
Aug 21, 2023
NihalJain
added a commit
that referenced
this pull request
Aug 29, 2023
…#5364) Signed-off-by: Bryan Beaudreault <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Reid Chan <[email protected]>
NihalJain
added a commit
that referenced
this pull request
Aug 29, 2023
…#5364) Signed-off-by: Bryan Beaudreault <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Reid Chan <[email protected]> (cherry picked from commit 2713a3c)
bbeaudreault
pushed a commit
to HubSpot/hbase
that referenced
this pull request
Dec 13, 2023
…ncorrectly (apache#5323) (apache#5364) Signed-off-by: Bryan Beaudreault <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Reid Chan <[email protected]>
bbeaudreault
pushed a commit
that referenced
this pull request
Jan 23, 2024
Signed-off-by: Bryan Beaudreault <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Reid Chan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.