HBASE-23648: Re-use underlying connection registry in RawAsyncHBaseAdmin #994

bharathv · 2020-01-06T22:59:01Z

No need to create and close a new registry on demand. Audited other
usages of getRegistry() and the code looks fine.

bharathv · 2020-01-06T23:02:40Z

@saintstack / @Apache9 A small fix that fixes ad-hoc creation of registries. I audited the code and other invocations look fine.

Apache-HBase · 2020-01-06T23:55:33Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 34s	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.
-0 ⚠️	test4tests	0m 0s	The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
		_ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _
+1 💚	mvninstall	5m 33s	HBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚	compile	0m 26s	HBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚	checkstyle	0m 34s	HBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚	shadedjars	4m 37s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 25s	HBASE-18095/client-locate-meta-no-zookeeper passed
+0 🆗	spotbugs	1m 9s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	1m 8s	HBASE-18095/client-locate-meta-no-zookeeper passed
		_ Patch Compile Tests _
+1 💚	mvninstall	5m 7s	the patch passed
+1 💚	compile	0m 26s	the patch passed
+1 💚	javac	0m 26s	the patch passed
-1 ❌	checkstyle	0m 31s	hbase-client: The patch generated 1 new + 13 unchanged - 0 fixed = 14 total (was 13)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	shadedjars	4m 41s	patch has no errors when building our shaded downstream artifacts.
+1 💚	hadoopcheck	15m 50s	Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚	javadoc	0m 23s	the patch passed
+1 💚	findbugs	1m 14s	the patch passed
		_ Other Tests _
+1 💚	unit	1m 59s	hbase-client in the patch passed.
+1 💚	asflicense	0m 14s	The patch does not generate ASF License warnings.
		50m 13s

Subsystem	Report/Notes
Docker	Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-994/1/artifact/out/Dockerfile
GITHUB PR	#994
JIRA Issue	HBASE-23648
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname	Linux 2561f2b295d6 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 GNU/Linux
Build tool	maven
Personality	/home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-994/out/precommit/personality/provided.sh
git revision	HBASE-18095/client-locate-meta-no-zookeeper / `d016cd9`
Default Java	1.8.0_181
checkstyle	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-994/1/artifact/out/diff-checkstyle-hbase-client.txt
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-994/1/testReport/
Max. process+thread count	288 (vs. ulimit of 10000)
modules	C: hbase-client U: hbase-client
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-994/1/console
versions	git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered by	Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

bharathv · 2020-01-07T00:18:08Z

Existing test coverage should be good enough, hence no new tests are added.

Apache-HBase · 2020-01-07T01:14:47Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 33s	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.
-0 ⚠️	test4tests	0m 0s	The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
		_ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _
+1 💚	mvninstall	5m 31s	HBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚	compile	0m 25s	HBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚	checkstyle	0m 32s	HBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚	shadedjars	4m 42s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 25s	HBASE-18095/client-locate-meta-no-zookeeper passed
+0 🆗	spotbugs	1m 8s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	1m 5s	HBASE-18095/client-locate-meta-no-zookeeper passed
		_ Patch Compile Tests _
+1 💚	mvninstall	5m 0s	the patch passed
+1 💚	compile	0m 25s	the patch passed
+1 💚	javac	0m 25s	the patch passed
+1 💚	checkstyle	0m 31s	hbase-client: The patch generated 0 new + 0 unchanged - 13 fixed = 0 total (was 13)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	shadedjars	4m 36s	patch has no errors when building our shaded downstream artifacts.
+1 💚	hadoopcheck	15m 47s	Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚	javadoc	0m 24s	the patch passed
+1 💚	findbugs	1m 12s	the patch passed
		_ Other Tests _
+1 💚	unit	1m 58s	hbase-client in the patch passed.
+1 💚	asflicense	0m 14s	The patch does not generate ASF License warnings.
		49m 36s

Subsystem	Report/Notes
Docker	Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-994/2/artifact/out/Dockerfile
GITHUB PR	#994
JIRA Issue	HBASE-23648
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname	Linux b8d4fae3599c 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 GNU/Linux
Build tool	maven
Personality	/home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-994/out/precommit/personality/provided.sh
git revision	HBASE-18095/client-locate-meta-no-zookeeper / `d016cd9`
Default Java	1.8.0_181
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-994/2/testReport/
Max. process+thread count	289 (vs. ulimit of 10000)
modules	C: hbase-client U: hbase-client
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-994/2/console
versions	git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered by	Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

saintstack

LGTM. Where does it get committed? On branch? Leaving it for a day in case @Apache9 wants to take a look.

For the future, try to minimize your changes. Most of the below is just refactoring. Takes a while to verify no accidental breakage. For the future.

Good stuff.

bharathv · 2020-01-07T06:22:49Z

LGTM. Where does it get committed? On branch?

Yes please, feature branch. It builds on the "ConnectionRegistry" change that exists only in the branch. I'm hoping the branch gets merged into the master soon.

For the future, try to minimize your changes. Most of the below is just refactoring. Takes a while to verify no accidental breakage. For the future.

Ack. I see your point. Generally I'm trying to fix all the check style issues around the places I'm touching, there are tons of violations. But I guess it is better to bulk fix them in separate changes.

bharathv · 2020-01-08T20:45:37Z

@saintstack is this good to go? thanks for the review.

saintstack · 2020-01-09T20:12:50Z

Sorry @bharathv I left it too long? There are conflicts? Mind fixing please?

No need to create and close a new registry on demand. Audited other usages of getRegistry() and the code looks fine.

bharathv · 2020-01-09T20:27:38Z

@saintstack No problemo! Appreciate the reviews.