HBASE-26614 Refactor code related to "dump"ing ZK nodes by ndimiduk · Pull Request #3969 · apache/hbase

ndimiduk · 2021-12-21T02:29:15Z

The code starting at ZKUtil.dump(ZKWatcher) is a small mess – it has cyclic dependencies woven
through itself, ZKWatcher and RecoverableZooKeeper. It also initializes a static variable in
ZKUtil through the factory for RecoverableZooKeeper instances. Let's decouple and clean it
up.

ndimiduk · 2021-12-22T16:57:18Z

We crash errorprone? 😭

apurtell · 2021-12-23T02:51:25Z

Is the replication unit result in the precommit a repeatable failure?

joshelser

Of course, after I went through the new ZKDump class, I realized that this was all copy-paste :). Leaving the comments as-is, but I'm not concerned about them.

Are there any shell scripts which might have called the old dump() static method you removed? Is it possible to deprecate the old method and have it just call the new location of the utility in the ZKDump class instead?

joshelser · 2021-12-31T19:13:03Z

+ return connect(conf, ensemble, watcher);
+ }
+
+ public static RecoverableZooKeeper connect(Configuration conf, String ensemble,


Would be nice to have Javadoc on all of the connect public methods in this class, but just a nit since this is @private

joshelser · 2021-12-31T19:19:21Z

+ try {
+ sb.append("\n ").append(MasterAddressTracker.getMasterAddress(zkWatcher));
+ } catch (IOException e) {
+ sb.append("<<FAILED LOOKUP: ").append(e.getMessage()).append(">>");


"FAILED MASTER LOOKUP"? Something to indicate what we were trying to do which failed?

joshelser · 2021-12-31T19:19:57Z

+ for (String child : backupMasterChildrenNoWatchList) {
+ sb.append("\n ").append(child);
+ }
+ }


An else to indicate that we found no backups (or you think not printing anythign is sufficient?)

joshelser · 2021-12-31T19:20:50Z

+ }
+ }
+ try {
+ getReplicationZnodesDump(zkWatcher, sb);


Would recommend printing some header here like for the other sections.

joshelser · 2021-12-31T19:22:00Z

+ * @return The array of response strings.
+ * @throws IOException When the socket communication fails.
+ */
+ private static String[] getServerStats(String server, int timeout)


nit: getZkServerStats

joshelser · 2021-12-31T19:37:26Z

+ }
+ }
+ for (String zNodeChild : ZKUtil.listChildrenNoWatch(zkw, znodeToProcess)) {
+ stack.add(ZNodePaths.joinZNode(znodeToProcess, zNodeChild));


I think you want to insert these at the front of the list rather than append them to the rest, otherwise you would be doing a depth-first traversal but over the reverse-sorted order of the children of znodeToProcess (if I remember correctly that listChildrenNoWatch will return the children in sorter order).

ndimiduk · 2022-01-05T19:25:16Z

Are there any shell scripts which might have called the old dump() static method you removed?

Oh, nice thinking. Yeah, looks like admin.rb.

$ find . -type f ! -iname '*.java' -exec grep -iIn 'ZKUtil' {} \+ | grep dump
./hbase-shell/target/classes/hbase/admin.rb:480: org.apache.hadoop.hbase.zookeeper.ZKUtil.dump(@zk_wrapper)
./hbase-shell/src/main/ruby/hbase/admin.rb:480: org.apache.hadoop.hbase.zookeeper.ZKUtil.dump(@zk_wrapper)

sorry for the edits

Is it possible to deprecate the old method and have it just call the new location of the utility in the ZKDump class instead?

I could, but why? You want to protect users who are calling into an IA.Private class?

ndimiduk · 2022-01-05T19:47:34Z

Is the replication unit result in the precommit a repeatable failure?

I don't think so but I don't have logs from the failed test runs. Let's see if it's still there for the next one.

ndimiduk · 2022-01-06T00:00:13Z

I think this error prone failure is not related to my change. Mike is working on a fix/upgrade over on #3979

Apache9 · 2022-01-06T07:39:09Z

I think this error prone failure is not related to my change. Mike is working on a fix/upgrade over on #3979

It is related.

You call MetaTableLocator.getMetaRegionLocation in ZKDump but the method has a RestrictedApi annotation which only allow calling from ZKUtil and tests code. Since you just move some code in ZKUtil out to a separated class, you need to change the RestrictedApi annotation in MetaTableLocator.

Thanks.

Apache9

The error prone failure is related. Please fix it before merging. Use a 'request changes' to make it more clear since there are already too many comments.

ndimiduk · 2022-01-12T00:05:17Z

Thanks for the mention @Apache9 . I did miss your earlier comment. You are correct in that my patch introduces an ErrorProne violation according to the annotation that you point how. However, I didn't know this because the use of that annotation in our code is not compatible with the version of ErrorProne run by Yetus on pre-commit. I think Mike's patch that upgrades ErrorProne will fix this, but I'll need to do some testing to be sure.

In the mean time, let me progress on this ticket.

ndimiduk · 2022-01-12T01:38:36Z

@joshelser do you mind if we address your code comments in a subsequent change? I'd like to keep this one isolated to the relocations only, as much as possible.

ndimiduk · 2022-01-12T17:05:25Z

Unit test failure looks like HBASE-26651.

The code starting at `ZKUtil.dump(ZKWatcher)` is a small mess – it has cyclic dependencies woven through itself, `ZKWatcher` and `RecoverableZooKeeper`. It also initializes a static variable in `ZKUtil` through the factory for `RecoverableZooKeeper` instances. Let's decouple and clean it up.

ndimiduk · 2022-01-19T19:36:07Z

Argh. Build 9 Jdk11/Hadoop3 failed due to Read-only file system.

Apache-HBase · 2022-01-19T23:35:29Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 25s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 1s	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 _
+0 🆗	mvndep	0m 31s	Maven dependency ordering for branch
+1 💚	mvninstall	4m 0s	master passed
+1 💚	compile	4m 51s	master passed
+1 💚	checkstyle	1m 49s	master passed
+1 💚	spotbugs	3m 14s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 15s	Maven dependency ordering for patch
+1 💚	mvninstall	3m 49s	the patch passed
+1 💚	compile	4m 50s	the patch passed
-0 ⚠️	javac	0m 30s	hbase-zookeeper generated 5 new + 87 unchanged - 6 fixed = 92 total (was 93)
+1 💚	checkstyle	1m 47s	the patch passed
+1 💚	rubocop	0m 8s	There were no new rubocop issues.
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	hadoopcheck	19m 28s	Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1.
+1 💚	spotbugs	3m 48s	the patch passed
		_ Other Tests _
+1 💚	asflicense	0m 51s	The patch does not generate ASF License warnings.
		58m 40s

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-3969/11/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#3969
Optional Tests	dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile rubocop
uname	Linux eb0cf5892b63 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `1d2b8a0`
Default Java	AdoptOpenJDK-1.8.0_282-b08
javac	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3969/11/artifact/yetus-general-check/output/diff-compile-javac-hbase-zookeeper.txt
Max. process+thread count	96 (vs. ulimit of 30000)
modules	C: hbase-zookeeper hbase-server hbase-shell hbase-it U: .
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3969/11/console
versions	git=2.17.1 maven=3.6.3 spotbugs=4.2.2 rubocop=0.80.0
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2022-01-20T01:50:18Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 25s	Docker mode activated.
-0 ⚠️	yetus	0m 4s	Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
		_ Prechecks _
		_ master Compile Tests _
+0 🆗	mvndep	0m 31s	Maven dependency ordering for branch
+1 💚	mvninstall	3m 59s	master passed
+1 💚	compile	2m 15s	master passed
+1 💚	shadedjars	8m 21s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	1m 27s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 18s	Maven dependency ordering for patch
+1 💚	mvninstall	3m 51s	the patch passed
+1 💚	compile	2m 15s	the patch passed
+1 💚	javac	2m 15s	the patch passed
+1 💚	shadedjars	8m 18s	patch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	1m 27s	the patch passed
		_ Other Tests _
+1 💚	unit	0m 45s	hbase-zookeeper in the patch passed.
+1 💚	unit	148m 0s	hbase-server in the patch passed.
+1 💚	unit	7m 39s	hbase-shell in the patch passed.
+1 💚	unit	1m 18s	hbase-it in the patch passed.
		193m 30s

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-3969/11/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR	#3969
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux f0004fd6c333 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 tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `1d2b8a0`
Default Java	AdoptOpenJDK-1.8.0_282-b08
Test Results	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3969/11/testReport/
Max. process+thread count	4846 (vs. ulimit of 30000)
modules	C: hbase-zookeeper hbase-server hbase-shell hbase-it U: .
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3969/11/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 · 2022-01-20T01:53:08Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 25s	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 _
+0 🆗	mvndep	0m 11s	Maven dependency ordering for branch
+1 💚	mvninstall	4m 39s	master passed
+1 💚	compile	2m 38s	master passed
+1 💚	shadedjars	8m 25s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	1m 32s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 16s	Maven dependency ordering for patch
+1 💚	mvninstall	4m 41s	the patch passed
+1 💚	compile	2m 45s	the patch passed
+1 💚	javac	2m 45s	the patch passed
+1 💚	shadedjars	8m 39s	patch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	1m 34s	the patch passed
		_ Other Tests _
+1 💚	unit	0m 47s	hbase-zookeeper in the patch passed.
+1 💚	unit	148m 41s	hbase-server in the patch passed.
+1 💚	unit	7m 12s	hbase-shell in the patch passed.
+1 💚	unit	1m 12s	hbase-it in the patch passed.
		196m 14s

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-3969/11/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR	#3969
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux 2d25590a453f 4.15.0-156-generic #163-Ubuntu SMP Thu Aug 19 23:31:58 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `1d2b8a0`
Default Java	AdoptOpenJDK-11.0.10+9
Test Results	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3969/11/testReport/
Max. process+thread count	3944 (vs. ulimit of 30000)
modules	C: hbase-zookeeper hbase-server hbase-shell hbase-it U: .
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3969/11/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.

The code starting at `ZKUtil.dump(ZKWatcher)` is a small mess – it has cyclic dependencies woven through itself, `ZKWatcher` and `RecoverableZooKeeper`. It also initializes a static variable in `ZKUtil` through the factory for `RecoverableZooKeeper` instances. Let's decouple and clean it up. Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Josh Elser <elserj@apache.org>

This comment has been minimized.
Sign in to view

ndimiduk requested review from Apache9, apurtell, bharathv, joshelser and saintstack December 22, 2021 23:20

This comment has been minimized.
Sign in to view

joshelser approved these changes Dec 31, 2021
View reviewed changes

ndimiduk force-pushed the 26614-refector-zkdump branch from f010576 to 2aee01cCompare January 5, 2022 19:43

This comment has been minimized.
Sign in to view

ndimiduk force-pushed the 26614-refector-zkdump branch from 2aee01c to ecb0471Compare January 7, 2022 22:56

This comment has been minimized.
Sign in to view

Apache9 requested changes Jan 8, 2022
View reviewed changes

ndimiduk force-pushed the 26614-refector-zkdump branch from ecb0471 to 16ad718Compare January 12, 2022 00:08

This comment has been minimized.
Sign in to view

ndimiduk requested a review from Apache9 January 12, 2022 17:05

ndimiduk added 4 commits January 18, 2022 15:54

Update javadocs for RecoverableZooKeeper#connect factory methods
64e6c56

update usage from hbase-shell
37bfe68

fix ErrorProne violations
028f5b3

ndimiduk force-pushed the 26614-refector-zkdump branch from 16ad718 to 028f5b3Compare January 18, 2022 23:55

address javadoc nits
aa87ea7

This comment has been minimized.
Sign in to view

Apache9 approved these changes Jan 19, 2022
View reviewed changes

ndimiduk merged commit 625d610 into apache:masterJan 24, 2022

ndimiduk deleted the 26614-refector-zkdump branch January 24, 2022 19:33

Uh oh!

Conversation

ndimiduk commented Dec 21, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ndimiduk commented Dec 22, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

apurtell commented Dec 23, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

joshelser left a comment

Choose a reason for hiding this comment

Uh oh!

joshelserDec 31, 2021

Choose a reason for hiding this comment

Uh oh!

joshelserDec 31, 2021

Choose a reason for hiding this comment

Uh oh!

joshelserDec 31, 2021

Choose a reason for hiding this comment

Uh oh!

joshelserDec 31, 2021

Choose a reason for hiding this comment

Uh oh!

joshelserDec 31, 2021

Choose a reason for hiding this comment

Uh oh!

joshelserDec 31, 2021

Choose a reason for hiding this comment

Uh oh!

ndimiduk commented Jan 5, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ndimiduk commented Jan 5, 2022

Uh oh!

This comment has been minimized.

This comment has been minimized.

ndimiduk commented Jan 6, 2022

Uh oh!

This comment has been minimized.

Apache9 commented Jan 6, 2022

Uh oh!

This comment has been minimized.

Apache9 left a comment

Choose a reason for hiding this comment

Uh oh!

ndimiduk commented Jan 12, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

ndimiduk commented Jan 12, 2022

Uh oh!

This comment has been minimized.

This comment has been minimized.

ndimiduk commented Jan 12, 2022

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ndimiduk commented Jan 19, 2022

Uh oh!

Apache-HBase commented Jan 19, 2022

Uh oh!

Apache-HBase commented Jan 20, 2022

Uh oh!

Apache-HBase commented Jan 20, 2022

Uh oh!

Reviewers

ndimiduk commented Jan 5, 2022•
edited
Loading

ndimiduk commented Jan 12, 2022•
edited
Loading