[HBASE-22874] Canary should not be IA.Public #565
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 5 commits
August 20, 2019 12:40
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
busbey
requested changes
Aug 30, 2019
| public interface CanaryInterface { | ||
|
|
||
| static CanaryInterface create(Configuration conf, ExecutorService executor) { | ||
| return new Canary(conf); |
| import java.util.concurrent.ExecutorService; | ||
|
|
||
| @InterfaceAudience.Public | ||
| public interface CanaryInterface { |
There was a problem hiding this comment.
What do folks think about having the interface be Canary and the tool be CanaryTool or CanaryImpl?
There was a problem hiding this comment.
I also thought about this. If we backport this to working branches. In my case it is 1.3. We already have other tools which calls Canary via toolRunner and if I backport this patch to 1.3 then my current code will fail since Canary will be an interface now instead of ToolRunner implementation.
There was a problem hiding this comment.
I think when you backport this patch to 1.3 you can also change the code in 1.3 to call CanaryTool instead of Canary? No big problem? CanaryInterface is not a good class name I'd say.
There was a problem hiding this comment.
I think when you backport this patch to 1.3 you can also change the code in 1.3 to call CanaryTool instead of Canary?
Wouldn't that be an incompatible change ? In minor upgrades, if we ask downstream projects to change how they invoke public exposed classes. I am just trying to understand the compatibility challenges here. In my company, I can change my downstream project. I am not sure if other people would be fine with it. @Apache9 @busbey Please advise.
| @@ -1629,7 +1684,7 @@ public static void main(String[] args) throws Exception { | |||
| final Configuration conf = HBaseConfiguration.create(); | |||
|
|
|||
| // Loading the generic options to conf | |||
| new GenericOptionsParser(conf, args); | |||
| //new GenericOptionsParser(conf, args); | |||
There was a problem hiding this comment.
why aren't we parsing generic options anymore?
There was a problem hiding this comment.
This was redundant. We already do parsing in https://github.com/apache/hadoop/blob/branch-2.8/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ToolRunner.java#L70
| @@ -843,9 +876,9 @@ public int run(String[] args) throws Exception { | |||
| } | |||
| } | |||
| currentTimeLength = System.currentTimeMillis() - startTime; | |||
| if (currentTimeLength > this.timeout) { | |||
| if (currentTimeLength > timeout) { | |||
There was a problem hiding this comment.
nit: avoid unrelated style changes like removing the this variables
There was a problem hiding this comment.
I removed bunch of class variables and store them in conf object. I replaced the class variable with method local variable. So I can't use "this" anymore.
|
Thank you @busbey for your reviews. This is still a WIP patch. I wanted to run all the test once and see if I introduced any regression. Will add unit tests, cleanup code, address your comments in next revision. Hopefully by EOD I should be able to get it to review state. :) |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
requested changes
Sep 1, 2019
| import java.util.concurrent.ExecutorService; | ||
|
|
||
| @InterfaceAudience.Public | ||
| public interface CanaryInterface { |
There was a problem hiding this comment.
I think when you backport this patch to 1.3 you can also change the code in 1.3 to call CanaryTool instead of Canary? No big problem? CanaryInterface is not a good class name I'd say.
| * @return the exit code of the Canary tool. | ||
| * @throws Exception | ||
| */ | ||
| public int runRegionCanary(String[] targets) throws Exception; |
There was a problem hiding this comment.
If we want to pass tables here, why not just use TableName as the parameter?
There was a problem hiding this comment.
Sure. Will address in next revision.
There was a problem hiding this comment.
If we want to pass tables here, why not just use TableName as the parameter?
The reason to not use TableName is we can pass table names as regular expression also.
| * @return the exit code of the Canary tool. | ||
| * @throws Exception | ||
| */ | ||
| public int runRegionServerCanary(String[] targets) throws Exception; |
There was a problem hiding this comment.
Ditto. And call it checkRegionServers?
There was a problem hiding this comment.
Sure. Will address in next revision.
| * @return the exit code of the Canary tool. | ||
| * @throws Exception | ||
| */ | ||
| public int runZookeeperCanary() throws Exception; |
| /** | ||
| * Runs Canary in Zookeeper mode. | ||
| * @return the exit code of the Canary tool. | ||
| * @throws Exception |
There was a problem hiding this comment.
nit: remove the empty throws.
| * Runs Canary in Region server mode. | ||
| * @param targets -- list of monitor tables. | ||
| * @return the exit code of the Canary tool. | ||
| * @throws Exception |
There was a problem hiding this comment.
nit: remove the empty throws.
| * Run Canary in Region mode. | ||
| * @param targets -- list of monitor tables. | ||
| * @return the exit code of the Canary tool. | ||
| * @throws Exception |
There was a problem hiding this comment.
nit: remove the empty throws.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
It usually means a commit got pushed to master that says it's fixed. I don't see anything obvious but haven't gotten to look much. |
|
I don't see anything in the git log that obviously relates to this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Things I changed in this PR:
Outstanding questions that I need help from community.
Can we just create 1 conf property for usage via API. We still will support all 3 arguments via ToolRunner interface.
@busbey @Apache9 @gjacoby126 @apurtell please review.