HBASE-26891 Make MetricsConnection scope configurable #4285
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. |
apurtell
approved these changes
Apr 2, 2022
| @@ -132,7 +133,8 @@ public AsyncConnectionImpl(Configuration conf, ConnectionRegistry registry, Stri | |||
| this.connConf = new AsyncConnectionConfiguration(conf); | |||
| this.registry = registry; | |||
| if (conf.getBoolean(CLIENT_SIDE_METRICS_ENABLED_KEY, false)) { | |||
| this.metrics = Optional.of(new MetricsConnection(this.toString(), () -> null, () -> null)); | |||
| String scope = conf.get(METRICS_SCOPE_KEY, clusterId + "-" + hashCode()); | |||
There was a problem hiding this comment.
Why not this.toString() for default value? This carries through as default previous code.
There was a problem hiding this comment.
This scope is added to the JMX bean name by JmxReporter. A good default value here can make it easier for find the JMX metrics for a specific connection.
I personally think this.toString() is a relatively bad default because it doesn't help identify at all. If you have multiple connections going, you'll just see 2 beans with: scope=AsyncConnectionImpl@23fd322 and scope=AsyncConnectionImpl@abcd123. What do those signify? Impossible to really know.
The one benefit of that default is that the hashCode ensures that it will indeed be 2 distinct beans in jmx.
The change I propose still may not help if you have 2 connections to the same cluster -- for clusterId foo you might then see foo-23fd322 and foo-abcd123. In that case, if you care, you should customize the scope with my new setting.
But if you have two connections to different clusters, it will be really helpful i think -- for clusterIds foo and bar you'd then see foo-23fd322 and bar-abcd123.
The only problem i see with this change is im not sure if it's a compatibility issue?
There was a problem hiding this comment.
If you disagree or think it's a compatibility issue, i'll be happy to revert that part. Internally, I plan to override scope in all cases, but thought this might be a good quality of life improvement.
There was a problem hiding this comment.
One small thing: with this implementation, you use hashCode() as an int value, while the default implementation provided by Object#toString() does Integer.toHexString(hashCode()) on the value.
From https://hbase.apache.org/book.html#hbase.versioning, all "metrics changes" are covered under the "Operational Compatibility" category. That category has "N" under both "Major" and "Minor" columns and a "Y" under the "Patch" column, which I interpret to mean that we are free to make changes to metrics in minor version updates.
There was a problem hiding this comment.
Thanks! I'll fix that and also the one checkstyle warning before merging. I'll plan to merge this as is to master, branch-2, and branch-2.5. I'll leave the default alone for branch-2.4
There was a problem hiding this comment.
Ok I just pushed those fixes
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Latest commit is just a result of my starting to work on branch-2 backport, where I wanted to DRY up the default scope generation rather than duplicate in both clients. I figured it can't hurt to have here as well for encapsulation and to keep the 2 branches a little more in sync. |
|
🎊 +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. |
|
🎊 +1 overall
This message was automatically generated. |
apurtell
approved these changes
Apr 13, 2022
| @@ -33,14 +33,16 @@ | |||
| import java.util.concurrent.ThreadPoolExecutor; | |||
| import java.util.concurrent.TimeUnit; | |||
| import java.util.function.Supplier; | |||
|
|
|||
There was a problem hiding this comment.
We're actually going back and forth on this. I think these extra line breaks are still present in the eclipse formatter and thus are being returned via spotless. but anyway i've given up on reviewing for style because spotless is in progress.
There was a problem hiding this comment.
Thank you both. Since I got 2 approvals and spotless is imminent, I'm going to merge this and get started on the backports. Just trying to avoid unnecessary work around re-approvals and re-running commit hooks. Let me know if that's unacceptable @apurtell -- If i don't hear anything, i'll try to go through this whole process tomorrow morning.
ndimiduk
approved these changes
Apr 13, 2022
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.
While I was here, I also changed the default scope. Just my opinion, but I feel like the toString representation is not a good default value because it is relatively meaningless. The one good thing it does is ensure uniqueness of the scopes. I changed the default value to be
${clusterId}-${connection.hashCode}. This should provide the same uniqueness, but using clusterId will help users identify metrics in processes that connect to more than one hbase cluster.I'm not sure if we'd consider that change to be a compatibility issue. It only affects the
scope=part of the JMX MBean objectName. If this is a concern, I can revert that part.Note: once approved I will have to submit a PR for branch-2, since there we have to also account for the blocking client.