[HBASE-22874] Canary should not be IA.Public by shahrs87 · Pull Request #580 · apache/hbase

shahrs87 · 2019-09-03T21:46:53Z

Things I changed in this PR:

Created CanaryInterface class which will be marked as Public. (I am not that good with names. Please let me know if you can think of something better).
Created 3 methods to call RegionCanary, RegionServerCanary and ZookeeperCanary via Api.
Created conf properties for all options that we pass via ToolRunner.
Tried to commonalize the code between ToolRunner and API usage.

Outstanding questions that I need help from community.

Does it make sense to have "daemon" flag via api usage ?
Should we allow using "interval" argument via api. If I specify interval it means canary will run every "interval" seconds.
There are 3 properties which fails on error. -f, -treatFailureAsError, -failureArError
Can we just create 1 conf property for usage via API. We still will support all 3 arguments via ToolRunner interface.
How to figure out which constructor for CanaryInterface to expose ?
Do we need to instantiate executor if it is null ? Today Canary has one constructor which accepts executor as argument. We never verify whether the passed executor is instantiated or not. Feels like bug to me. Or is it understood that if someone calls that constructor, its his/her responsibility to instantiate executor.

For some unknown reason asfgit user closed my earlier PR: #565 so I created another one.

…2874

Apache-HBase · 2019-09-04T20:59:09Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	82	Docker mode activated.
		_ Prechecks _
+1	dupname	0	No case conflicting files found.
+1	hbaseanti	0	Patch does not have any anti-patterns.
+1	@author	0	The patch does not contain any @author tags.
+1	test4tests	0	The patch appears to include 1 new or modified test files.
		_ master Compile Tests _
+1	mvninstall	401	master passed
+1	compile	68	master passed
+1	checkstyle	94	master passed
+1	shadedjars	304	branch has no errors when building our shaded downstream artifacts.
+1	javadoc	40	master passed
0	spotbugs	276	Used deprecated FindBugs config; considering switching to SpotBugs.
+1	findbugs	273	master passed
		_ Patch Compile Tests _
+1	mvninstall	331	the patch passed
+1	compile	60	the patch passed
+1	javac	60	the patch passed
-1	checkstyle	89	hbase-server: The patch generated 2 new + 21 unchanged - 2 fixed = 23 total (was 23)
+1	whitespace	0	The patch has no whitespace issues.
+1	shadedjars	323	patch has no errors when building our shaded downstream artifacts.
+1	hadoopcheck	1108	Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1	javadoc	37	the patch passed
+1	findbugs	282	the patch passed
		_ Other Tests _
-1	unit	16255	hbase-server in the patch failed.
+1	asflicense	27	The patch does not generate ASF License warnings.
		20185

Reason	Tests
Failed junit tests	hadoop.hbase.client.TestFromClientSide

Subsystem	Report/Notes
Docker	Client=19.03.2 Server=19.03.2 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-580/1/artifact/out/Dockerfile
GITHUB PR	#580
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname	Linux 12056470573a 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-580/out/precommit/personality/provided.sh
git revision	master / `dfe5280`
Default Java	1.8.0_181
checkstyle	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-580/1/artifact/out/diff-checkstyle-hbase-server.txt
unit	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-580/1/artifact/out/patch-unit-hbase-server.txt
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-580/1/testReport/
Max. process+thread count	4621 (vs. ulimit of 10000)
modules	C: hbase-server U: hbase-server
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-580/1/console
versions	git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered by	Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2019-09-05T21:40:08Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
💙	reexec	0m 36s	Docker mode activated.
		_ Prechecks _
💚	dupname	0m 1s	No case conflicting files found.
💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
💚	@author	0m 0s	The patch does not contain any @author tags.
💚	test4tests	0m 0s	The patch appears to include 1 new or modified test files.
		_ master Compile Tests _
💚	mvninstall	5m 24s	master passed
💚	compile	0m 55s	master passed
💚	checkstyle	1m 18s	master passed
💚	shadedjars	4m 31s	branch has no errors when building our shaded downstream artifacts.
💚	javadoc	0m 37s	master passed
💙	spotbugs	4m 0s	Used deprecated FindBugs config; considering switching to SpotBugs.
💚	findbugs	3m 58s	master passed
		_ Patch Compile Tests _
💚	mvninstall	4m 49s	the patch passed
💚	compile	0m 54s	the patch passed
💚	javac	0m 54s	the patch passed
💔	checkstyle	1m 19s	hbase-server: The patch generated 1 new + 21 unchanged - 2 fixed = 22 total (was 23)
💚	whitespace	0m 0s	The patch has no whitespace issues.
💚	shadedjars	4m 34s	patch has no errors when building our shaded downstream artifacts.
💚	hadoopcheck	15m 34s	Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
💚	javadoc	0m 35s	the patch passed
💚	findbugs	4m 16s	the patch passed
		_ Other Tests _
💔	unit	158m 29s	hbase-server in the patch failed.
💚	asflicense	0m 28s	The patch does not generate ASF License warnings.
		215m 1s

Subsystem	Report/Notes
Docker	Client=19.03.1 Server=19.03.1 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-580/2/artifact/out/Dockerfile
GITHUB PR	#580
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname	Linux 8bfe6c5d2891 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 GNU/Linux
Build tool	maven
Personality	/home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-580/out/precommit/personality/provided.sh
git revision	master / `0d338a1`
Default Java	1.8.0_181
checkstyle	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-580/2/artifact/out/diff-checkstyle-hbase-server.txt
unit	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-580/2/artifact/out/patch-unit-hbase-server.txt
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-580/2/testReport/
Max. process+thread count	4916 (vs. ulimit of 10000)
modules	C: hbase-server U: hbase-server
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-580/2/console
versions	git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered by	Apache Yetus 0.11.0 https://yetus.apache.org

This message was automatically generated.

shahrs87 · 2019-09-12T01:51:56Z

@jatsakthi Would like to see this commit in 1.3 release also. Apologies if I am requesting to review too many PR's. I will really appreciate if you can review this PR. Thank you !

saintstack · 2019-09-14T18:17:27Z

-@InterfaceAudience.Private
-public final class Canary implements Tool {
+@InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.TOOLS)
+public class Canary implements Tool, CanaryInterface {


Can Canary be an Interface?

Yeah please make the interface Canary and the tool CanaryTool or CanaryImpl or something.

@saintstack @busbey Addressed your comments in latest commit. Could you please review. Thank you !

saintstack · 2019-09-14T18:36:04Z

Outstanding questions that I need help from community.
Does it make sense to have "daemon" flag via api usage ?
Should we allow using "interval" argument via api. If I specify interval it means canary will run every "interval" seconds.
There are 3 properties which fails on error. -f, -treatFailureAsError, -failureArError
Can we just create 1 conf property for usage via API. We still will support all 3 arguments via ToolRunner interface.
How to figure out which constructor for CanaryInterface to expose ?
Do we need to instantiate executor if it is null ? Today Canary has one constructor which accepts executor as argument. We never verify whether the passed executor is instantiated or not. Feels like bug to me. Or is it understood that if someone calls that constructor, its his/her responsibility to instantiate executor.

On 'daemon' api option, it is an odd option to expose IMO. Would think it daemon would be set always when calling something via API (i.e. in context).

Interval as argument seems fine. What you thinking?

One error option makes sense.

" Feels like bug to me. " Me too.

From an API, I'd think you'd be passing an executor?

shahrs87 · 2019-09-16T21:23:48Z

Thank you @saintstack for your review.

Interval as argument seems fine. What you thinking?

If I understand the interval parameter correctly, it will run the given canary periodically every interval seconds. If we are running the Canary via ToolRunner, it will create a new process and we can make it run every few seconds but if we call via api from some application, then it is the application's responsibility to call canary every interval seconds.
Any thoughts ?

saintstack · 2019-09-16T22:13:33Z

Makes sense. Interval is for the daemon version of Canary which makes no sense when being called from API. Yeah, no 'interval'. No 'daemon' option either.

Apache-HBase · 2019-09-18T02:24:27Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
💙	reexec	0m 42s	Docker mode activated.
		_ Prechecks _
💚	dupname	0m 0s	No case conflicting files found.
💚	hbaseanti	0m 1s	Patch does not have any anti-patterns.
💚	@author	0m 0s	The patch does not contain any @author tags.
💚	test4tests	0m 0s	The patch appears to include 1 new or modified test files.
		_ master Compile Tests _
💚	mvninstall	6m 54s	master passed
💚	compile	0m 57s	master passed
💚	checkstyle	1m 26s	master passed
💚	shadedjars	4m 32s	branch has no errors when building our shaded downstream artifacts.
💚	javadoc	0m 38s	master passed
💙	spotbugs	4m 16s	Used deprecated FindBugs config; considering switching to SpotBugs.
💚	findbugs	4m 14s	master passed
💛	patch	4m 23s	Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary.
		_ Patch Compile Tests _
💚	mvninstall	5m 12s	the patch passed
💚	compile	1m 0s	the patch passed
💚	javac	1m 0s	the patch passed
💔	checkstyle	1m 25s	hbase-server: The patch generated 3 new + 21 unchanged - 2 fixed = 24 total (was 23)
💚	whitespace	0m 0s	The patch has no whitespace issues.
💚	shadedjars	4m 46s	patch has no errors when building our shaded downstream artifacts.
💚	hadoopcheck	15m 38s	Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
💚	javadoc	0m 35s	the patch passed
💚	findbugs	4m 26s	the patch passed
		_ Other Tests _
💚	unit	160m 44s	hbase-server in the patch passed.
💚	asflicense	0m 38s	The patch does not generate ASF License warnings.
		220m 12s

Subsystem	Report/Notes
Docker	Client=19.03.1 Server=19.03.1 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-580/4/artifact/out/Dockerfile
GITHUB PR	#580
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname	Linux b9370cd1227d 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-580/out/precommit/personality/provided.sh
git revision	master / `cb62f73`
Default Java	1.8.0_181
checkstyle	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-580/4/artifact/out/diff-checkstyle-hbase-server.txt
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-580/4/testReport/
Max. process+thread count	4357 (vs. ulimit of 10000)
modules	C: hbase-server U: hbase-server
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-580/4/console
versions	git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered by	Apache Yetus 0.11.0 https://yetus.apache.org

This message was automatically generated.

shahrs87 · 2019-09-23T16:32:39Z

@saintstack @busbey could you guys please make another pass ? Thank you !

busbey · 2019-10-03T20:47:21Z

overall this looks good to me. I've made a correction locally to make sure bin/hbase references the new CanaryTool class. I'm running a last set of tests locally then I'll merge.

…tool implementation Closes#580 * Canary is now an IA.Public interface * CanaryTool is now the implementation Signed-off-by: Sean Busbey <busbey@apache.org>

…tool implementation Closes#580 * Canary is now an IA.Public interface * CanaryTool is now the implementation Signed-off-by: Sean Busbey <busbey@apache.org> (cherry picked from commit b5a3967)

shahrs87 · 2019-10-03T21:58:53Z

Thank you @busbey for the review and merge. Let me know if you want me to create PR for branch-1 or branch-2.

the-sakthi · 2019-10-03T22:03:13Z

@shahrs87 I am waiting for this commit to go into branch-1.3 for the 1.3 release. Could you please take care of the backports to branch-1 & branch-2 as well?

shahrs87 · 2019-10-03T22:06:13Z

Could you please take care of the backports to branch-1 & branch-2 as well?

I am relatively new to this project. I don't know the protocol of who handles the backport to branch-1 and branch-2. I was of the view that committer will take care of backports and if it needs more work than simple cherry-pick then he/she will let me know. In any case, I am fine to create a new PR for branch-1 and branch-2. Please let me know.

shahrs87 · 2019-10-03T22:07:25Z

Also I see that @busbey merged this commit to branch-2.1 and branch-2.2. So I think he is taking care of backports. :)

…tool implementation Closes#580 * Canary is now an IA.Public interface * CanaryTool is now the implementation Branch-1 specific changes for differences in APIs and cleanup of the ref guide using a classname. Co-authored-by: Sean Busbey <busbey@apache.org> Signed-off-by: Sean Busbey <busbey@apache.org>

…tool implementation Closes#580 * Canary is now an IA.Public interface * CanaryTool is now the implementation Branch-1.3 specific changes for differences in APIs and cleanup of the ref guide using a classname. Co-authored-by: Sean Busbey <busbey@apache.org> Signed-off-by: Sean Busbey <busbey@apache.org>

…tool implementation Closes#580 * Canary is now an IA.Public interface * CanaryTool is now the implementation Branch-1 specific changes for differences in APIs and cleanup of the ref guide using a classname. Co-authored-by: Sean Busbey <busbey@apache.org> Signed-off-by: Sean Busbey <busbey@apache.org>

…tool implementation Closesapache#580 * Canary is now an IA.Public interface * CanaryTool is now the implementation Signed-off-by: Sean Busbey <busbey@apache.org> (cherry picked from commit b5a3967) (cherry picked from commit f37105c) Change-Id: I5023d011922e17398d76dd8585008e0eccb9e989

Rushabh added 8 commits August 20, 2019 12:40

[HBASE-22874] Mark Canary as LA(Tools)
bb17c9c

Merge branch 'master' ofhttps://github.com/apache/hbaseinto HBASE-2…
9f31c54
…2874

Merge branch 'master' ofhttps://github.com/apache/hbaseinto HBASE-2…
9fe44bd
…2874

Merge branch 'master' ofhttps://github.com/apache/hbaseinto HBASE-2…
c625973
…2874

[HBASE-22874] Canary should not be IA.Public
bef668d

[HBASE-22874] Code formatting.
272f6d8

[HBASE-22874] Addressing review comments.
f5ee630

Merge branch 'master' ofhttps://github.com/apache/hbaseinto HBASE-2…
0cc0e84
…2874

[HBASE-22874] Fixing Checkstyle warnings.
3c09fb7

saintstack reviewed Sep 14, 2019
View reviewed changes

Rushabh added 4 commits September 17, 2019 14:36

Resolving merge conflicts.
b6f0560

[HBASE-22874] Rename Canary to CanaryTool
872b621

[HBASE-22874] Rename CanaryInterface to Canary
2cdcbda

[HBASE-22874] Addressing review comments.
80c3bac

shahrs87 force-pushed the HBASE-22874 branch from 96fd5eb to 80c3bacCompare September 17, 2019 22:06

asfgit closed this in 5aa8d3aOct 3, 2019

Uh oh!

Conversation

shahrs87 commented Sep 3, 2019

Uh oh!

Apache-HBase commented Sep 4, 2019

Uh oh!

Apache-HBase commented Sep 5, 2019

Uh oh!

shahrs87 commented Sep 12, 2019

Uh oh!

saintstackSep 14, 2019

Choose a reason for hiding this comment

Uh oh!

busbeySep 14, 2019

Choose a reason for hiding this comment

Uh oh!

shahrs87Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

saintstack commented Sep 14, 2019

Uh oh!

shahrs87 commented Sep 16, 2019

Uh oh!

saintstack commented Sep 16, 2019

Uh oh!

Apache-HBase commented Sep 18, 2019

Uh oh!

shahrs87 commented Sep 23, 2019

Uh oh!

busbey commented Oct 3, 2019

Uh oh!

shahrs87 commented Oct 3, 2019

Uh oh!

the-sakthi commented Oct 3, 2019

Uh oh!

shahrs87 commented Oct 3, 2019

Uh oh!

shahrs87 commented Oct 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants