Skip to content

HBASE-27652 Client-side lock contention around Configuration when using read replica regions - #5036

Merged
ndimiduk merged 3 commits into
apache:branch-2from
ndimiduk:27652-read-replica-connectionconfiguration-contention-branch-2
Mar 15, 2023
Merged

HBASE-27652 Client-side lock contention around Configuration when using read replica regions#5036
ndimiduk merged 3 commits into
apache:branch-2from
ndimiduk:27652-read-replica-connectionconfiguration-contention-branch-2

Conversation

@ndimiduk

Copy link
Copy Markdown
Member

No description provided.

@ndimidukndimiduk left a comment

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.

Posting up a PR vs. branch-2. I didn't find RpcRetryingCallerFactory on master. I guess the master patch will look quite different.

}

public static RpcRetryingCallerFactory instantiate(Configuration configuration,
@RestrictedApi(explanation = "Should only be called on process initialization", link = "",

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.

Avoid calling this method as much as is reasonable.

The HRegionServer call site is particularly insidious. It is called from the HRegionServer constructor and looks like this,

rpcRetryingCallerFactory = RpcRetryingCallerFactory.instantiate(this.conf,
clusterConnection == null ? null : clusterConnection.getConnectionMetrics());

I think that clusterConnection is always null here, because clusterConnection is instantiated in protected synchronized void setupClusterConnection(), which, as far as I can tell, is only called from public void run().

@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

This comment was marked as outdated.

@Apache9

Copy link
Copy Markdown
Contributor

Posting up a PR vs. branch-2. I didn't find RpcRetryingCallerFactory on master. I guess the master patch will look quite different.

On master the whole sync client is built upon async client, so the retrying caller related classes are all removed.

@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.

Most code are just boiler plate as we add a parameter for the constructor.

Only one concern is about changing RpcRetryingCallerFactory to IA.LimitedPrivate, as I do not think we want users to customize it, otherwise we should have introduced an interface. I think the configuration is just for tests, just like we can also set the implementation class of HMaster and HRegionServer, but we not expect users to do this.

@ndimiduk
ndimidukforce-pushed the 27652-read-replica-connectionconfiguration-contention-branch-2 branch from 49358d4 to 3f219b2CompareMarch 3, 2023 12:29
@ndimiduk
ndimiduk requested a review from Apache9March 3, 2023 12:31
@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

This comment was marked as outdated.

@ndimiduk
ndimidukforce-pushed the 27652-read-replica-connectionconfiguration-contention-branch-2 branch from 3f219b2 to 34ed035CompareMarch 8, 2023 14:59
@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

This comment was marked as outdated.

@ndimiduk
ndimidukforce-pushed the 27652-read-replica-connectionconfiguration-contention-branch-2 branch from 34ed035 to 37591b1CompareMarch 14, 2023 12:39
@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 38sDocker 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.
_ branch-2 Compile Tests _
+0 🆗mvndep0m 20sMaven dependency ordering for branch
+1 💚mvninstall4m 0sbranch-2 passed
+1 💚compile3m 40sbranch-2 passed
+1 💚checkstyle1m 0sbranch-2 passed
+1 💚spotless0m 44sbranch has no errors when running spotless:check.
+1 💚spotbugs2m 50sbranch-2 passed
_ Patch Compile Tests _
+0 🆗mvndep0m 10sMaven dependency ordering for patch
+1 💚mvninstall4m 0sthe patch passed
+1 💚compile3m 43sthe patch passed
+1 💚javac3m 43sthe patch passed
+1 💚checkstyle0m 57sthe patch passed
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚hadoopcheck20m 8sPatch does not cause any errors with Hadoop 2.10.2 or 3.2.4 3.3.4.
+1 💚spotless0m 45spatch has no errors when running spotless:check.
+1 💚spotbugs3m 22sthe patch passed
_ Other Tests _
+1 💚asflicense0m 23sThe patch does not generate ASF License warnings.
48m 54s
SubsystemReport/Notes
DockerClientAPI=1.42 ServerAPI=1.42 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5036/5/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#5036
Optional Testsdupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile
unameLinux af466fe53727 5.4.0-1097-aws #105~18.04.1-Ubuntu SMP Mon Feb 13 17:50:57 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionbranch-2 / b5dcd48
Default JavaEclipse Adoptium-11.0.17+8
Max. process+thread count81 (vs. ulimit of 30000)
modulesC: hbase-client hbase-server hbase-endpoint U: .
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5036/5/console
versionsgit=2.34.1 maven=3.8.6 spotbugs=4.7.3
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 13sDocker mode activated.
-0 ⚠️yetus0m 4sUnprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ branch-2 Compile Tests _
+0 🆗mvndep0m 40sMaven dependency ordering for branch
+1 💚mvninstall5m 36sbranch-2 passed
+1 💚compile1m 39sbranch-2 passed
+1 💚shadedjars5m 42sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 59sbranch-2 passed
_ Patch Compile Tests _
+0 🆗mvndep0m 10sMaven dependency ordering for patch
+1 💚mvninstall5m 10sthe patch passed
+1 💚compile1m 39sthe patch passed
+1 💚javac1m 39sthe patch passed
+1 💚shadedjars5m 29spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 54sthe patch passed
_ Other Tests _
+1 💚unit8m 13shbase-client in the patch passed.
+1 💚unit198m 1shbase-server in the patch passed.
+1 💚unit2m 27shbase-endpoint in the patch passed.
242m 25s
SubsystemReport/Notes
DockerClientAPI=1.42 ServerAPI=1.42 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5036/5/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR#5036
Optional Testsjavac javadoc unit shadedjars compile
unameLinux e338f2f2e8b9 5.4.0-1093-aws #102~18.04.2-Ubuntu SMP Wed Dec 7 00:31:59 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionbranch-2 / b5dcd48
Default JavaEclipse Adoptium-11.0.17+8
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5036/5/testReport/
Max. process+thread count2818 (vs. ulimit of 30000)
modulesC: hbase-client hbase-server hbase-endpoint U: .
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5036/5/console
versionsgit=2.34.1 maven=3.8.6
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 33sDocker mode activated.
-0 ⚠️yetus0m 5sUnprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ branch-2 Compile Tests _
+0 🆗mvndep0m 11sMaven dependency ordering for branch
+1 💚mvninstall4m 20sbranch-2 passed
+1 💚compile1m 53sbranch-2 passed
+1 💚shadedjars6m 26sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 10sbranch-2 passed
_ Patch Compile Tests _
+0 🆗mvndep0m 13sMaven dependency ordering for patch
+1 💚mvninstall3m 35sthe patch passed
+1 💚compile1m 44sthe patch passed
+1 💚javac1m 44sthe patch passed
+1 💚shadedjars6m 29spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 8sthe patch passed
_ Other Tests _
+1 💚unit8m 19shbase-client in the patch passed.
-1 ❌unit238m 3shbase-server in the patch failed.
+1 💚unit3m 14shbase-endpoint in the patch passed.
283m 31s
SubsystemReport/Notes
DockerClientAPI=1.42 ServerAPI=1.42 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5036/5/artifact/yetus-jdk8-hadoop2-check/output/Dockerfile
GITHUB PR#5036
Optional Testsjavac javadoc unit shadedjars compile
unameLinux 0e0fd983582c 5.4.0-144-generic #161-Ubuntu SMP Fri Feb 3 14:49:04 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionbranch-2 / b5dcd48
Default JavaTemurin-1.8.0_352-b08
unithttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5036/5/artifact/yetus-jdk8-hadoop2-check/output/patch-unit-hbase-server.txt
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5036/5/testReport/
Max. process+thread count2112 (vs. ulimit of 30000)
modulesC: hbase-client hbase-server hbase-endpoint U: .
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5036/5/console
versionsgit=2.34.1 maven=3.8.6
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@ndimiduk
ndimiduk merged commit dbb7838 into apache:branch-2Mar 15, 2023
@ndimiduk
ndimiduk deleted the 27652-read-replica-connectionconfiguration-contention-branch-2 branch March 15, 2023 15:10
bbeaudreault pushed a commit to HubSpot/hbase that referenced this pull request Jul 31, 2023
…iguration when using read replica regions (apache#5036)
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ndimiduk@Apache-HBase@Apache9