You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2022/03/28 12:53:54 UTC
[GitHub] [hbase] bbeaudreault opened a new pull request #4285: HBASE-26891 Make MetricsConnection scope configurable
bbeaudreault opened a new pull request #4285:
URL: https://github.com/apache/hbase/pull/4285
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell commented on a change in pull request #4285: HBASE-26891 Make MetricsConnection scope configurable
Posted by GitBox <gi...@apache.org>.
apurtell commented on a change in pull request #4285:
URL: https://github.com/apache/hbase/pull/4285#discussion_r841106943
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -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());
Review comment:
Why not `this.toString()` for default value? This carries through as default previous code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4285: HBASE-26891 Make MetricsConnection scope configurable
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #4285:
URL: https://github.com/apache/hbase/pull/4285#issuecomment-1080655168
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 10s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 23s | master passed |
| +1 :green_heart: | compile | 0m 40s | master passed |
| +1 :green_heart: | checkstyle | 0m 18s | master passed |
| +1 :green_heart: | spotbugs | 0m 41s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 13s | the patch passed |
| +1 :green_heart: | compile | 0m 38s | the patch passed |
| +1 :green_heart: | javac | 0m 38s | the patch passed |
| -0 :warning: | checkstyle | 0m 16s | hbase-client: The patch generated 1 new + 1 unchanged - 0 fixed = 2 total (was 1) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 11m 32s | Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1. |
| +1 :green_heart: | spotbugs | 0m 45s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 12s | The patch does not generate ASF License warnings. |
| | | 25m 55s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4285/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4285 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile |
| uname | Linux b8e426d283e4 5.4.0-90-generic #101-Ubuntu SMP Fri Oct 15 20:00:55 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / a21179287b |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| checkstyle | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4285/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-client.txt |
| Max. process+thread count | 60 (vs. ulimit of 30000) |
| modules | C: hbase-client U: hbase-client |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4285/1/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4285: HBASE-26891 Make MetricsConnection scope configurable
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #4285:
URL: https://github.com/apache/hbase/pull/4285#issuecomment-1080644655
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 30s | Docker mode activated. |
| -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 51s | master passed |
| +1 :green_heart: | compile | 0m 20s | master passed |
| +1 :green_heart: | shadedjars | 3m 39s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 18s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 33s | the patch passed |
| +1 :green_heart: | compile | 0m 20s | the patch passed |
| +1 :green_heart: | javac | 0m 20s | the patch passed |
| +1 :green_heart: | shadedjars | 3m 39s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 17s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 31s | hbase-client in the patch passed. |
| | | 17m 0s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4285/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4285 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 8138265e2a17 5.4.0-90-generic #101-Ubuntu SMP Fri Oct 15 20:00:55 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 620e5c6d0a |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4285/1/testReport/ |
| Max. process+thread count | 184 (vs. ulimit of 30000) |
| modules | C: hbase-client U: hbase-client |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4285/1/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4285: HBASE-26891 Make MetricsConnection scope configurable
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #4285:
URL: https://github.com/apache/hbase/pull/4285#issuecomment-1080644235
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 2s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 28s | master passed |
| +1 :green_heart: | compile | 0m 18s | master passed |
| +1 :green_heart: | shadedjars | 3m 39s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 17s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 11s | the patch passed |
| +1 :green_heart: | compile | 0m 18s | the patch passed |
| +1 :green_heart: | javac | 0m 18s | the patch passed |
| +1 :green_heart: | shadedjars | 3m 40s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 15s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 13s | hbase-client in the patch passed. |
| | | 16m 28s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4285/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4285 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux ab718644ac02 5.4.0-90-generic #101-Ubuntu SMP Fri Oct 15 20:00:55 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / a21179287b |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4285/1/testReport/ |
| Max. process+thread count | 160 (vs. ulimit of 30000) |
| modules | C: hbase-client U: hbase-client |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4285/1/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a change in pull request #4285: HBASE-26891 Make MetricsConnection scope configurable
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on a change in pull request #4285:
URL: https://github.com/apache/hbase/pull/4285#discussion_r841107787
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -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());
Review comment:
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: `AsyncConnectionImpl@23fd322` and `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.
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 iwth this change is im not sure if it's a compatibility issue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a change in pull request #4285: HBASE-26891 Make MetricsConnection scope configurable
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on a change in pull request #4285:
URL: https://github.com/apache/hbase/pull/4285#discussion_r841108646
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -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());
Review comment:
If you disagree or think it's a compatibility issue, i'll be happy to revert that part. I plan to override scope in all cases, but thought this might be a good quality of life improvement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a change in pull request #4285: HBASE-26891 Make MetricsConnection scope configurable
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on a change in pull request #4285:
URL: https://github.com/apache/hbase/pull/4285#discussion_r841108646
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -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());
Review comment:
If you think it's a compatibility issue, i'll be happy to revert that part. I plan to override scope in all cases, but thought this might be a good quality of life improvement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org