Skip to content

HBASE-26474 Implement connection-level attributes - #3952

Merged
ndimiduk merged 5 commits into
apache:masterfrom
ndimiduk:26474-tracing-connection-level-attributes
Jan 5, 2022
Merged

HBASE-26474 Implement connection-level attributes#3952
ndimiduk merged 5 commits into
apache:masterfrom
ndimiduk:26474-tracing-connection-level-attributes

Conversation

@ndimiduk

@ndimidukndimiduk commented Dec 15, 2021

Copy link
Copy Markdown
Member

@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
Apache9
Apache9 reviewed Dec 17, 2021
View reviewed changes
Comment threadhbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncRegionLocator.java Outdated
Comment threadhbase-client/src/main/java/org/apache/hadoop/hbase/client/trace/TableOperationSpanBuilder.java Outdated

@Override
@SuppressWarnings("unchecked")
public B self() {

@Apache9Apache9Dec 17, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need to have so complicated class hierarchy? A class has a generic type parameter which is just itself?

@ndimidukndimidukDec 17, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like it either. Java really sucks for inheritance with the builder pattern. I could try to rebuild it using mix-ins, but I think that will be equally complex.

@ndimidukndimidukDec 17, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a change that replaces build inheritance with a mix-in style of builder. I don't really like it any better, let me know what you think.

Comment threadhbase-client/src/test/java/org/apache/hadoop/hbase/client/TestAsyncTableTracing.java Outdated
@ndimiduk
ndimidukforce-pushed the 26474-tracing-connection-level-attributes branch from 6df2a9d to 7df1343CompareDecember 17, 2021 20:28
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
Apache9
Apache9 reviewed Dec 21, 2021
View reviewed changes
Comment threadhbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncRegionLocator.java Outdated
Comment threadhbase-client/src/test/java/org/apache/hadoop/hbase/client/TestAsyncRegionLocatorTracing.java
@ndimiduk
ndimidukforce-pushed the 26474-tracing-connection-level-attributes branch from 3dc359f to c39630dCompareDecember 22, 2021 23:40
Apache9
Apache9 approved these changes Dec 23, 2021
View reviewed changes

@Apache9Apache9 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM.

Just a minor question on the naming, not a veto. I'm not sure if there are other meanings for a connection string so you choose to use something like a hostname. Just go with your opinion.

Comment threadhbase-server/src/main/java/org/apache/hadoop/hbase/client/ShortCircuitConnectionRegistry.java Outdated

@Override
public String getConnectionString() {
return "localhost";

@Apache9Apache9Dec 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just call it short-circuit? I do not think localhost is accurate enough, it may implies that we are connecting to a regsitry through network but on the same machine? For short-circuit we know that we are using the ShortCircuitConnectionRegistry.

@ndimidukndimidukDec 23, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good point; localhost does have a specific meaning. Let me consider it, perhaps I can propose a better name. "short-circuit" isn't too bad.

@Apache-HBase

Apache-HBase commented Dec 23, 2021

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 20sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
+1 💚@author0m 0sThe patch does not contain any @author tags.
_ master Compile Tests _
+0 🆗mvndep0m 18sMaven dependency ordering for branch
+1 💚mvninstall4m 28smaster passed
+1 💚compile5m 5smaster passed
+1 💚checkstyle2m 1smaster passed
+1 💚spotbugs3m 56smaster passed
_ Patch Compile Tests _
+0 🆗mvndep0m 13sMaven dependency ordering for patch
+1 💚mvninstall4m 15sthe patch passed
+1 💚compile5m 7sthe patch passed
+1 💚javac5m 7sthe patch passed
+1 💚checkstyle2m 5sthe patch passed
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚hadoopcheck21m 40sPatch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1.
+1 💚spotbugs4m 31sthe patch passed
_ Other Tests _
+1 💚asflicense0m 35sThe patch does not generate ASF License warnings.
64m 57s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/4/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#3952
Optional Testsdupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile
unameLinux b116f0d0b1fd 4.15.0-163-generic #171-Ubuntu SMP Fri Nov 5 11:55:11 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / f16b7b1
Default JavaAdoptOpenJDK-1.8.0_282-b08
Max. process+thread count85 (vs. ulimit of 30000)
modulesC: hbase-common hbase-client hbase-server U: .
Console outputhttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/4/console
versionsgit=2.17.1 maven=3.6.3 spotbugs=4.2.2
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Apache-HBase commented Dec 23, 2021

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 26sDocker mode activated.
-0 ⚠️yetus0m 4sUnprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+0 🆗mvndep0m 17sMaven dependency ordering for branch
+1 💚mvninstall4m 9smaster passed
+1 💚compile1m 55smaster passed
+1 💚shadedjars8m 23sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 27smaster passed
_ Patch Compile Tests _
+0 🆗mvndep0m 18sMaven dependency ordering for patch
+1 💚mvninstall3m 53sthe patch passed
+1 💚compile1m 55sthe patch passed
+1 💚javac1m 55sthe patch passed
+1 💚shadedjars8m 15spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 27sthe patch passed
_ Other Tests _
+1 💚unit1m 43shbase-common in the patch passed.
+1 💚unit1m 21shbase-client in the patch passed.
-1 ❌unit150m 29shbase-server in the patch failed.
188m 56s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/4/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR#3952
Optional Testsjavac javadoc unit shadedjars compile
unameLinux 366e8bf224c9 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / f16b7b1
Default JavaAdoptOpenJDK-1.8.0_282-b08
unithttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/4/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-server.txt
Test Resultshttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/4/testReport/
Max. process+thread count3908 (vs. ulimit of 30000)
modulesC: hbase-common hbase-client hbase-server U: .
Console outputhttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/4/console
versionsgit=2.17.1 maven=3.6.3
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Apache-HBase commented Dec 23, 2021

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 4sDocker mode activated.
-0 ⚠️yetus0m 3sUnprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+0 🆗mvndep0m 14sMaven dependency ordering for branch
+1 💚mvninstall5m 14smaster passed
+1 💚compile2m 18smaster passed
+1 💚shadedjars9m 8sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 33smaster passed
_ Patch Compile Tests _
+0 🆗mvndep0m 15sMaven dependency ordering for patch
+1 💚mvninstall5m 3sthe patch passed
+1 💚compile2m 17sthe patch passed
+1 💚javac2m 17sthe patch passed
+1 💚shadedjars9m 4spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 32sthe patch passed
_ Other Tests _
+1 💚unit2m 43shbase-common in the patch passed.
+1 💚unit1m 42shbase-client in the patch passed.
+1 💚unit203m 37shbase-server in the patch passed.
247m 56s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/4/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR#3952
Optional Testsjavac javadoc unit shadedjars compile
unameLinux affd4cd8d81e 4.15.0-163-generic #171-Ubuntu SMP Fri Nov 5 11:55:11 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / f16b7b1
Default JavaAdoptOpenJDK-11.0.10+9
Test Resultshttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/4/testReport/
Max. process+thread count2746 (vs. ulimit of 30000)
modulesC: hbase-common hbase-client hbase-server U: .
Console outputhttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/4/console
versionsgit=2.17.1 maven=3.6.3
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@ndimiduk
HBASE-26474 Implement connection-level attributes
e3c0158
Add support for `db.system`, `db.connection_string`, `db.user`.
@ndimiduk
PR Feedback
262f85f
* address checkstyle complaints
* remove member variable `user` from TestAsyncTableTracing
@ndimiduk
PR Feedback
e906647
* replace inheritance in *SpanBuilders with a mix-in approach
@ndimiduk
PR Feedback
ea144c4
@ndimiduk
PR Feedback
acfac73
@ndimiduk
ndimidukforce-pushed the 26474-tracing-connection-level-attributes branch from c39630d to acfac73CompareJanuary 5, 2022 17:35
@Apache-HBase

Apache-HBase commented Jan 5, 2022

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 20sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 1sNo case conflicting files found.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
+1 💚@author0m 0sThe patch does not contain any @author tags.
_ master Compile Tests _
+0 🆗mvndep0m 14sMaven dependency ordering for branch
+1 💚mvninstall4m 35smaster passed
+1 💚compile5m 9smaster passed
+1 💚checkstyle2m 4smaster passed
+1 💚spotbugs4m 2smaster passed
_ Patch Compile Tests _
+0 🆗mvndep0m 13sMaven dependency ordering for patch
+1 💚mvninstall4m 15sthe patch passed
+1 💚compile5m 7sthe patch passed
+1 💚javac5m 7sthe patch passed
+1 💚checkstyle2m 1sthe patch passed
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚hadoopcheck21m 50sPatch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1.
+1 💚spotbugs4m 36sthe patch passed
_ Other Tests _
+1 💚asflicense0m 35sThe patch does not generate ASF License warnings.
65m 23s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/5/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#3952
Optional Testsdupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile
unameLinux 134e29479239 4.15.0-163-generic #171-Ubuntu SMP Fri Nov 5 11:55:11 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / baeb51f
Default JavaAdoptOpenJDK-1.8.0_282-b08
Max. process+thread count86 (vs. ulimit of 30000)
modulesC: hbase-common hbase-client hbase-server U: .
Console outputhttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/5/console
versionsgit=2.17.1 maven=3.6.3 spotbugs=4.2.2
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Apache-HBase commented Jan 5, 2022

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 27sDocker mode activated.
-0 ⚠️yetus0m 3sUnprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+0 🆗mvndep0m 16sMaven dependency ordering for branch
+1 💚mvninstall4m 4smaster passed
+1 💚compile1m 56smaster passed
+1 💚shadedjars8m 22sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 26smaster passed
_ Patch Compile Tests _
+0 🆗mvndep0m 18sMaven dependency ordering for patch
+1 💚mvninstall3m 53sthe patch passed
+1 💚compile1m 53sthe patch passed
+1 💚javac1m 53sthe patch passed
+1 💚shadedjars8m 22spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 25sthe patch passed
_ Other Tests _
+1 💚unit1m 42shbase-common in the patch passed.
+1 💚unit1m 22shbase-client in the patch passed.
+1 💚unit150m 10shbase-server in the patch passed.
188m 3s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/5/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR#3952
Optional Testsjavac javadoc unit shadedjars compile
unameLinux cf60ce1249b8 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / baeb51f
Default JavaAdoptOpenJDK-1.8.0_282-b08
Test Resultshttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/5/testReport/
Max. process+thread count3954 (vs. ulimit of 30000)
modulesC: hbase-common hbase-client hbase-server U: .
Console outputhttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/5/console
versionsgit=2.17.1 maven=3.6.3
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Apache-HBase commented Jan 5, 2022

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 1sDocker mode activated.
-0 ⚠️yetus0m 3sUnprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+0 🆗mvndep0m 16sMaven dependency ordering for branch
+1 💚mvninstall5m 21smaster passed
+1 💚compile2m 17smaster passed
+1 💚shadedjars9m 19sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 34smaster passed
_ Patch Compile Tests _
+0 🆗mvndep0m 15sMaven dependency ordering for patch
+1 💚mvninstall5m 6sthe patch passed
+1 💚compile2m 16sthe patch passed
+1 💚javac2m 16sthe patch passed
+1 💚shadedjars9m 18spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 31sthe patch passed
_ Other Tests _
+1 💚unit2m 16shbase-common in the patch passed.
+1 💚unit1m 47shbase-client in the patch passed.
-1 ❌unit209m 35shbase-server in the patch failed.
254m 2s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/5/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR#3952
Optional Testsjavac javadoc unit shadedjars compile
unameLinux ecc40f0727d4 4.15.0-163-generic #171-Ubuntu SMP Fri Nov 5 11:55:11 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / baeb51f
Default JavaAdoptOpenJDK-11.0.10+9
unithttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/5/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-server.txt
Test Resultshttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/5/testReport/
Max. process+thread count2762 (vs. ulimit of 30000)
modulesC: hbase-common hbase-client hbase-server U: .
Console outputhttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3952/5/console
versionsgit=2.17.1 maven=3.6.3
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@ndimiduk
ndimiduk merged commit 5cb7ed8 into apache:masterJan 5, 2022
@ndimiduk

ndimiduk commented Jan 5, 2022

Copy link
Copy Markdown
MemberAuthor

Thank you, @Apache9 !

@ndimiduk
ndimiduk deleted the 26474-tracing-connection-level-attributes branch January 5, 2022 23:32
ndimiduk added a commit to ndimiduk/hbase that referenced this pull request Jan 6, 2022
@ndimiduk
HBASE-26474 Implement connection-level attributes (apache#3952)
486eb4d
Add support for `db.system`, `db.connection_string`, `db.user`.
Signed-off-by: Duo Zhang <zhangduo@apache.org>
@ndimidukndimiduk mentioned this pull request Jan 6, 2022
Merged
ndimiduk added a commit to ndimiduk/hbase that referenced this pull request Jan 12, 2022
@ndimiduk
HBASE-26474 Implement connection-level attributes (apache#3952)
706be8a
Add support for `db.system`, `db.connection_string`, `db.user`.
Signed-off-by: Duo Zhang <zhangduo@apache.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Apache9Apache9Apache9 approved these changes
@taklwutaklwuAwaiting requested review from taklwu

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ndimiduk@Apache-HBase@Apache9