HBASE-26240 Set InterfaceAudience to Private for BalanceRequest$Builder #3663

bbeaudreault · 2021-09-07T12:31:35Z

Apache-HBase · 2021-09-07T13:09:57Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	6m 23s	Docker mode activated.
-0 ⚠️	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 💚	mvninstall	4m 24s	master passed
+1 💚	compile	0m 27s	master passed
+1 💚	shadedjars	9m 0s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 23s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	4m 10s	the patch passed
+1 💚	compile	0m 25s	the patch passed
+1 💚	javac	0m 25s	the patch passed
+1 💚	shadedjars	9m 4s	patch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 21s	the patch passed
		_ Other Tests _
+1 💚	unit	1m 40s	hbase-client in the patch passed.
		37m 32s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3663/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR	#3663
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux f4e6f86213d4 4.15.0-147-generic #151-Ubuntu SMP Fri Jun 18 19:21:19 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `afbdd41`
Default Java	AdoptOpenJDK-1.8.0_282-b08
Test Results	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3663/1/testReport/
Max. process+thread count	221 (vs. ulimit of 30000)
modules	C: hbase-client U: hbase-client
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3663/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.

Apache-HBase · 2021-09-07T13:12:38Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	7m 18s	Docker mode activated.
-0 ⚠️	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 💚	mvninstall	5m 2s	master passed
+1 💚	compile	0m 31s	master passed
+1 💚	shadedjars	9m 10s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 27s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	4m 43s	the patch passed
+1 💚	compile	0m 30s	the patch passed
+1 💚	javac	0m 30s	the patch passed
+1 💚	shadedjars	9m 5s	patch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 26s	the patch passed
		_ Other Tests _
+1 💚	unit	1m 44s	hbase-client in the patch passed.
		40m 11s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3663/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR	#3663
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux d97f7d2698b0 4.15.0-143-generic #147-Ubuntu SMP Wed Apr 14 16:10:11 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `afbdd41`
Default Java	AdoptOpenJDK-11.0.10+9
Test Results	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3663/1/testReport/
Max. process+thread count	212 (vs. ulimit of 30000)
modules	C: hbase-client U: hbase-client
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3663/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.

Apache-HBase · 2021-09-07T13:16:47Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 30s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
		_ master Compile Tests _
+1 💚	mvninstall	4m 2s	master passed
+1 💚	compile	0m 59s	master passed
+1 💚	checkstyle	0m 27s	master passed
+1 💚	spotbugs	1m 4s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	3m 39s	the patch passed
+1 💚	compile	1m 1s	the patch passed
+1 💚	javac	1m 1s	the patch passed
+1 💚	checkstyle	0m 26s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	hadoopcheck	20m 13s	Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0.
+1 💚	spotbugs	1m 37s	the patch passed
		_ Other Tests _
+1 💚	asflicense	0m 15s	The patch does not generate ASF License warnings.
		44m 20s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3663/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#3663
Optional Tests	dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile
uname	Linux 61e2e4d12543 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `afbdd41`
Default Java	AdoptOpenJDK-1.8.0_282-b08
Max. process+thread count	96 (vs. ulimit of 30000)
modules	C: hbase-client U: hbase-client
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3663/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.

Apache-HBase · 2021-09-07T13:50:02Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 7s	Docker mode activated.
-0 ⚠️	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 💚	mvninstall	4m 23s	master passed
+1 💚	compile	0m 26s	master passed
+1 💚	shadedjars	9m 6s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 24s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	4m 6s	the patch passed
+1 💚	compile	0m 26s	the patch passed
+1 💚	javac	0m 26s	the patch passed
+1 💚	shadedjars	9m 11s	patch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 21s	the patch passed
		_ Other Tests _
+1 💚	unit	1m 39s	hbase-client in the patch passed.
		32m 25s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3663/2/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR	#3663
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux 6feea6c3e431 4.15.0-147-generic #151-Ubuntu SMP Fri Jun 18 19:21:19 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `afbdd41`
Default Java	AdoptOpenJDK-1.8.0_282-b08
Test Results	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3663/2/testReport/
Max. process+thread count	227 (vs. ulimit of 30000)
modules	C: hbase-client U: hbase-client
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3663/2/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.

Apache-HBase · 2021-09-07T13:51:40Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 7s	Docker mode activated.
-0 ⚠️	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 💚	mvninstall	5m 6s	master passed
+1 💚	compile	0m 31s	master passed
+1 💚	shadedjars	9m 7s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 27s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	4m 50s	the patch passed
+1 💚	compile	0m 31s	the patch passed
+1 💚	javac	0m 31s	the patch passed
+1 💚	shadedjars	9m 3s	patch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 26s	the patch passed
		_ Other Tests _
+1 💚	unit	1m 43s	hbase-client in the patch passed.
		34m 7s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3663/2/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR	#3663
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux 79357b691aa7 4.15.0-143-generic #147-Ubuntu SMP Wed Apr 14 16:10:11 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `afbdd41`
Default Java	AdoptOpenJDK-11.0.10+9
Test Results	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3663/2/testReport/
Max. process+thread count	222 (vs. ulimit of 30000)
modules	C: hbase-client U: hbase-client
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3663/2/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.

Apache-HBase · 2021-09-07T14:10:30Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 46s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
		_ master Compile Tests _
+1 💚	mvninstall	4m 53s	master passed
+1 💚	compile	1m 11s	master passed
+1 💚	checkstyle	0m 32s	master passed
+1 💚	spotbugs	1m 15s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	4m 47s	the patch passed
+1 💚	compile	1m 11s	the patch passed
+1 💚	javac	1m 12s	the patch passed
+1 💚	checkstyle	0m 32s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	hadoopcheck	25m 11s	Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0.
+1 💚	spotbugs	1m 28s	the patch passed
		_ Other Tests _
+1 💚	asflicense	0m 14s	The patch does not generate ASF License warnings.
		52m 52s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3663/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#3663
Optional Tests	dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile
uname	Linux c54850f41a7e 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `afbdd41`
Default Java	AdoptOpenJDK-1.8.0_282-b08
Max. process+thread count	96 (vs. ulimit of 30000)
modules	C: hbase-client U: hbase-client
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3663/2/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.

ndimiduk · 2021-09-08T16:39:05Z

Sorry to nit-pick, but if the build is explicitly private API, can it be its own top-level class, rather than being an inner class of the public API? I think it's confusing to downstream developers when we inter-mingle public and private API resources in the same class.

otherwise, +1

bbeaudreault · 2021-09-08T17:48:33Z

@ndimiduk I hear you, but honestly I feel like there are pros and cons on both sides of that. If I break it out, I'd need to make BalanceResponse's constructor package-private at least, and the org.apache.hadoop.hbase.client package is quite large. I'd probably also get rid of the convenience BalanceResponse.newBuilder() since putting that in BalanceResponseBuilder would be sort of redundant. So we'd change the access pattern to new BalanceResponseBuilder().build(). Not a huge deal, but it's less intuitive and now there's an asymmetry with BalanceRequest. I could do the same with BalanceRequest, but that's just expanding non-intuitiveness. I could leave the asymmetry, but I do think there's some value there and I feel like inner Builder classes are sort of conventional.

None of that is a non-starter, but I'm not sure if it's explicitly better than having the inner class be IA.Private, or even IA.Public. I can do it if you feel strongly about it, but just wanted to put that out there for consideration.

ndimiduk

Thanks for the context and your thinking here, @bbeaudreault . I feel strongly about it, but in the larger context. This is exactly the kind of improvement that we might one-day pursue. In the mean time, let's go forward with what you have presented here.

…er (apache#3663) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Nick Dimiduk <[email protected]>

…er (#3663) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Nick Dimiduk <[email protected]>

joshelser · 2021-09-10T00:57:29Z

Sorry for the belated reply. @bbeaudreault my ask was to move BalanceResponse$Builder to be private, not BalanceRequest$Builder to be private. Users are expected to be able to use BalancerRequest, as this is on Admin#balance(BalanceRequest).

hbase/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java

Line 847 in a15e94a

BalanceResponse balance(BalanceRequest request) throws IOException;

My point was that the Master is the only thing which should be creating BalanceResponse objects, thus end users don't need to know about the BalanceResponse$Builder class. Because the constructors on BalanceRequest are private, users must use the Builder class.

Object	Client Visibility
`BalanceRequest`	Public
`BalanceRequest$Builder`	Public
`BalanceResponse`	Public
`BalanceResponse$Builder`	Private

Please correct me if I'm wrong. I think we want to revert the change to BalancerRequest$Builder and apply it to BalancerRespons$Builder instead.

Apache9 · 2021-09-10T01:36:56Z

Oh, thanks @joshelser for pointing this out.

The IA.Private class should be BalanceResponse, not BalanceRequest...

bbeaudreault · 2021-09-10T01:48:36Z

Ugh! You are right, I totally messed that up. I'll push a fix tomorrow. Sorry all

HBASE-26240 Set InterfaceAudience to Private for BalanceRequest$Builder

ec95bfe

bbeaudreault force-pushed the balance_request_ia branch from 4a28afa to ec95bfe Compare September 7, 2021 12:35

Apache9 approved these changes Sep 8, 2021

View reviewed changes

ndimiduk approved these changes Sep 8, 2021

View reviewed changes

ndimiduk merged commit 8679e08 into apache:master Sep 9, 2021

ndimiduk pushed a commit to ndimiduk/hbase that referenced this pull request Sep 9, 2021

HBASE-26240 Set InterfaceAudience to Private for BalanceRequest$Build…

b4dc9af

…er (apache#3663) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Nick Dimiduk <[email protected]>

bbeaudreault deleted the balance_request_ia branch September 9, 2021 18:03

ndimiduk pushed a commit that referenced this pull request Sep 9, 2021

HBASE-26240 Set InterfaceAudience to Private for BalanceRequest$Build…

bb05901

…er (#3663) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Nick Dimiduk <[email protected]>

bbeaudreault mentioned this pull request Sep 10, 2021

HBASE-26277: Fix InterfaceAudience for BalanceRequest$Builder and BalanceResponse$Builder #3674

Closed

HBASE-26240 Set InterfaceAudience to Private for BalanceRequest$Builder #3663

HBASE-26240 Set InterfaceAudience to Private for BalanceRequest$Builder #3663

Uh oh!

Conversation

bbeaudreault commented Sep 7, 2021

Uh oh!

Apache-HBase commented Sep 7, 2021

Uh oh!

Apache-HBase commented Sep 7, 2021

Uh oh!

Apache-HBase commented Sep 7, 2021

Uh oh!

Apache-HBase commented Sep 7, 2021

Uh oh!

Apache-HBase commented Sep 7, 2021

Uh oh!

Apache-HBase commented Sep 7, 2021

Uh oh!

ndimiduk commented Sep 8, 2021

Uh oh!

bbeaudreault commented Sep 8, 2021

Uh oh!

ndimiduk left a comment

Choose a reason for hiding this comment

Uh oh!

joshelser commented Sep 10, 2021

Uh oh!

Apache9 commented Sep 10, 2021

Uh oh!

bbeaudreault commented Sep 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants