Skip to content

HBASE-26782 Minor code cleanup in and around RpcExecutor - #4144

Merged
ndimiduk merged 1 commit into
apache:masterfrom
ndimiduk:26782-cleanup-RpcScheduler-RpcExecutor
Mar 7, 2022
Merged

HBASE-26782 Minor code cleanup in and around RpcExecutor#4144
ndimiduk merged 1 commit into
apache:masterfrom
ndimiduk:26782-cleanup-RpcScheduler-RpcExecutor

Conversation

@ndimiduk

@ndimidukndimiduk commented Mar 1, 2022

Copy link
Copy Markdown
Member

While working on tracing, I see some minor cleanup that can be done in the RpcScheduler and
RpcExecutor classes. The implementations of the dispatch methods don't actually throw the
exceptions defined in their signatures, and there's some simplification to be done and checkstyle
warnings to resolve while I'm in there.

@ndimiduk

ndimiduk commented Mar 1, 2022

Copy link
Copy Markdown
MemberAuthor

Since RpcScheduler is exposed to coprocessors and Phoenix, I guess this can only go to master and branch-2.

@ndimiduk

ndimiduk commented Mar 1, 2022

Copy link
Copy Markdown
MemberAuthor

I don't think these files have been touched recently, so pining a wide reviewer audience. Sorry for the spam.

@Apache-HBase

This comment was marked as outdated.

Sign in to view
@Apache-HBase

This comment was marked as outdated.

Sign in to view
@Apache-HBase

This comment was marked as outdated.

Sign in to view
@ndimiduk

ndimiduk commented Mar 3, 2022

Copy link
Copy Markdown
MemberAuthor

FYI Phoenix maintainers: @stoty@twdsilva, maybe you have some thoughts about this change?

@Apache-HBase

This comment was marked as outdated.

Sign in to view
@stoty

stoty commented Mar 3, 2022

Copy link
Copy Markdown
Contributor

Thanks for the heads-up @ndimiduk .

Yes, Phoenix does use custom RpcSchedulers, and would be affected by the change.

While we can probably shim over the differences, it would be easier for us if the signature wouldn't change.

We'd prefer that this wouldn't change in branch-2.4, or even better on branch-2.

@Apache-HBase

This comment was marked as outdated.

Sign in to view
@Apache-HBase

This comment was marked as outdated.

Sign in to view
@ndimiduk

ndimiduk commented Mar 4, 2022

Copy link
Copy Markdown
MemberAuthor

@stoty

Yes, Phoenix does use custom RpcSchedulers, and would be affected by the change.

While we can probably shim over the differences, it would be easier for us if the signature wouldn't change.

We'd prefer that this wouldn't change in branch-2.4, or even better on branch-2.

Okay, let me separate the throws interface change from the rest.

@ndimiduk
ndimidukforce-pushed the 26782-cleanup-RpcScheduler-RpcExecutor branch from 6b1b4f2 to c3821cdCompareMarch 4, 2022 09:56
@ndimidukndimiduk changed the title HBASE-26782 Minor code cleanup in RpcScheduler and RpcExecutorHBASE-26782 Minor code cleanup in and around RpcExecutorMar 4, 2022
@Apache-HBase

Apache-HBase commented Mar 4, 2022

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec3m 34sDocker 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 _
+1 💚mvninstall3m 57smaster passed
+1 💚compile2m 14smaster passed
+1 💚checkstyle0m 37smaster passed
+1 💚spotbugs1m 14smaster passed
_ Patch Compile Tests _
+1 💚mvninstall2m 23sthe patch passed
+1 💚compile2m 10sthe patch passed
+1 💚javac2m 10sthe patch passed
+1 💚checkstyle0m 33shbase-server: The patch generated 0 new + 2 unchanged - 16 fixed = 2 total (was 18)
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚hadoopcheck11m 45sPatch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1.
+1 💚spotbugs1m 18sthe patch passed
_ Other Tests _
+1 💚asflicense0m 10sThe patch does not generate ASF License warnings.
35m 4s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4144/3/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#4144
Optional Testsdupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile
unameLinux f0890da88b50 5.4.0-1025-aws #25~18.04.1-Ubuntu SMP Fri Sep 11 12:03:04 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / 9c037b1
Default JavaAdoptOpenJDK-1.8.0_282-b08
Max. process+thread count60 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4144/3/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.

@ndimiduk
HBASE-26782 Minor code cleanup in and around RpcExecutor
629fbac
Signed-off-by: Bharath Vissapragada <bharathv@apache.org>
Signed-off-by: Duo Zhang <zhangduo@apache.org>
@ndimiduk
ndimidukforce-pushed the 26782-cleanup-RpcScheduler-RpcExecutor branch from c3821cd to 629fbacCompareMarch 7, 2022 10:47
@ndimiduk
ndimiduk merged commit f3f2aa9 into apache:masterMar 7, 2022
@ndimiduk
ndimiduk deleted the 26782-cleanup-RpcScheduler-RpcExecutor branch March 7, 2022 10:55
@Apache-HBase

Apache-HBase commented Mar 7, 2022

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 40sDocker 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 _
+1 💚mvninstall2m 23smaster passed
+1 💚compile2m 14smaster passed
+1 💚checkstyle0m 34smaster passed
+1 💚spotbugs1m 12smaster passed
_ Patch Compile Tests _
+1 💚mvninstall2m 21sthe patch passed
+1 💚compile2m 11sthe patch passed
+1 💚javac2m 11sthe patch passed
+1 💚checkstyle0m 33shbase-server: The patch generated 0 new + 2 unchanged - 16 fixed = 2 total (was 18)
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚hadoopcheck11m 45sPatch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1.
+1 💚spotbugs1m 18sthe patch passed
_ Other Tests _
+1 💚asflicense0m 7sThe patch does not generate ASF License warnings.
30m 28s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4144/4/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#4144
Optional Testsdupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile
unameLinux 922ad315201a 5.4.0-1025-aws #25~18.04.1-Ubuntu SMP Fri Sep 11 12:03:04 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / 591f781
Default JavaAdoptOpenJDK-1.8.0_282-b08
Max. process+thread count60 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4144/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 Mar 7, 2022

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 41sDocker 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 _
+1 💚mvninstall2m 50smaster passed
+1 💚compile0m 39smaster passed
+1 💚shadedjars3m 35sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 22smaster passed
_ Patch Compile Tests _
+1 💚mvninstall2m 37sthe patch passed
+1 💚compile0m 39sthe patch passed
+1 💚javac0m 39sthe patch passed
+1 💚shadedjars3m 35spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 22sthe patch passed
_ Other Tests _
+1 💚unit306m 37shbase-server in the patch passed.
323m 39s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4144/4/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR#4144
Optional Testsjavac javadoc unit shadedjars compile
unameLinux 217a58e52a7a 5.4.0-1025-aws #25~18.04.1-Ubuntu SMP Fri Sep 11 12:03:04 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / 591f781
Default JavaAdoptOpenJDK-11.0.10+9
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4144/4/testReport/
Max. process+thread count3122 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4144/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 Mar 7, 2022

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 49sDocker 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 _
+1 💚mvninstall3m 20smaster passed
+1 💚compile0m 53smaster passed
+1 💚shadedjars4m 17sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 29smaster passed
_ Patch Compile Tests _
+1 💚mvninstall2m 58sthe patch passed
+1 💚compile0m 42sthe patch passed
+1 💚javac0m 42sthe patch passed
+1 💚shadedjars4m 3spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 27sthe patch passed
_ Other Tests _
+1 💚unit381m 44shbase-server in the patch passed.
403m 29s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4144/4/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR#4144
Optional Testsjavac javadoc unit shadedjars compile
unameLinux cea12854da49 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 toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / 591f781
Default JavaAdoptOpenJDK-1.8.0_282-b08
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4144/4/testReport/
Max. process+thread count3043 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4144/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.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bharathvbharathvbharathv approved these changes
@Apache9Apache9Apache9 approved these changes
@YutSeanYutSeanAwaiting requested review from YutSean
@virajjasanivirajjasaniAwaiting requested review from virajjasani
@binlijinbinlijinAwaiting requested review from binlijin
@apurtellapurtellAwaiting requested review from apurtell
@busbeybusbeyAwaiting requested review from busbey
@jojochuangjojochuangAwaiting requested review from jojochuang
@joshelserjoshelserAwaiting requested review from joshelser
@z-yorkz-yorkAwaiting requested review from z-york

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ndimiduk@Apache-HBase@stoty@bharathv@Apache9