HBASE-23639 : Moving classes out of hbase-it /test for direct API use of chaos. #1201
Conversation
This file contains hidden or 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
lokiore
changed the title
|
💔 -1 overall
This message was automatically generated. |
mihir6692
reviewed
Feb 25, 2020
| import java.util.HashSet; | ||
| import java.util.Set; | ||
|
|
||
| @InterfaceAudience.Private |
Contributor
There was a problem hiding this comment.
We should keep this Public. It would help when you are creating new MonkeyFactory and wants to use default constants like PERIODIC_ACTION1_PERIOD, MOVE_REGIONS_MAX_TIME etc.
Contributor
There was a problem hiding this comment.
By "creating new MonkeyFactory" , i mean creating outside hbase-it , in user's project
virajjasani
reviewed
Feb 25, 2020
Contributor
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
One minor suggestion in addition to Mihir's comment
| public class HBaseClusterManager extends Configured implements ClusterManager { | ||
| @InterfaceAudience.Public | ||
| public class HBaseClusterManager extends Configured implements | ||
| org.apache.hadoop.hbase.ClusterManager { |
Contributor
There was a problem hiding this comment.
nit: you can import it and then use
Contributor
|
Overall, looks good |
|
💔 -1 overall
This message was automatically generated. |
busbey
requested changes
Feb 25, 2020
Contributor
busbey
left a comment
busbey
left a comment
There was a problem hiding this comment.
committers please do not merge this. discussion on HBASE-23639.
Contributor
|
💔 -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. |
Contributor
Author
|
@apurtell @virajjasani @mihir6692 @busbey , can you take a look at the PR and see if anything else is required here.! |
virajjasani
reviewed
Mar 17, 2020
Contributor
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
+1 overall, need one confirmation
| import org.apache.hbase.thirdparty.com.google.common.collect.Sets; | ||
| import org.apache.hbase.thirdparty.org.apache.commons.cli.CommandLine; | ||
|
|
||
| @InterfaceAudience.LimitedPrivate({HBaseInterfaceAudience.CHAOS, HBaseInterfaceAudience.TOOLS}) |
Contributor
There was a problem hiding this comment.
Maybe I missed discussion around including HBaseInterfaceAudience.TOOLS for ChaosMonkeyRunner. Hope this is as per the discussions.
Contributor
Author
There was a problem hiding this comment.
@virajjasani did discussion internally as ChaosMonkeyRunner can be used as tool (can be used from command line), still needed confirmation. waiting from others.
Contributor
There was a problem hiding this comment.
IMHO it should be fine to add TOOLS to ChaosMonkeyRunner.
virajjasani
previously approved these changes
Mar 28, 2020
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ndimiduk
reviewed
Apr 2, 2020
Member
ndimiduk
left a comment
ndimiduk
left a comment
There was a problem hiding this comment.
- Please ensure your git commit and JIRA subjects match. The JIRA seems inappropriately named, given the scope of this change.
- Have you confirmed that existing IT tools still work out of the box after these changes? Is there any automated testing in our CI environment that provides this assurance?
- Do you have an example application that uses these APIs as public? Does it work correctly? Is it still reaching in to access private methods?
hbase-it/pom.xml
Outdated
| <groupId>org.apache.hbase</groupId> | ||
| <artifactId>hbase-zookeeper</artifactId> | ||
| <type>test-jar</type> | ||
| <scope>compile</scope> |
Member
There was a problem hiding this comment.
compile is the default scope, no need to mention it.
hbase-it/pom.xml
Outdated
| <dependency> | ||
| <groupId>org.apache.hbase</groupId> | ||
| <artifactId>hbase-server</artifactId> | ||
| <type>test-jar</type> |
Member
There was a problem hiding this comment.
Why are there compile scope dependencies on test jars? Is there a plan to move the required code into a src/main/java?
Contributor
Author
There was a problem hiding this comment.
yes I am moving these classes from src/test/java -> src/main/java as discussed with Sean and Andrew that any code under test with an audience and stability annotation is not meaningful and anti-pattern.
Member
There was a problem hiding this comment.
I see that you're moving classes within the hbase-it module. But that's not what I asked about. I'm curious about their transitive dependencies -- do these classes in hbase-it/src/main/java have dependencies on classes in abase-server/src/test/java? Why is it necessary to have dependences on the <type>test-jar</type> for classes that are our part of our API contract?
Contributor
Author
There was a problem hiding this comment.
Yes some classes in hbase-it/src/main/java have dependencies on classes present in hbase-server/src/test/java for ex: some DistributedHBaseCluster extends HBaseCluster which is present in hbase-server/src/test/java.
Contributor
Author
There was a problem hiding this comment.
Member
There was a problem hiding this comment.
This dependency graph goes deep, and makes this a non-trivial challenge to unravel. I agree we should be shipping a solid suite of tools for testing HBase and HBase applications. Sadly changing only the leaves of that graph is not sufficient. IMHO, you have to start at the root classes and work out to the leaves. I don't know if that effort is worth it when compared to building a new testing harness from scratch that's designed from the outset to be user-facing.
Contributor
Author
There was a problem hiding this comment.
@ndimiduk I moved the whole hierarchy in src/ on which chaos framework had a dependency, mostly classes are parent classes of IntegrationTestingUtility and DistributedHBaseCluster and their dependencies. Please take a look if this is the right way!!
Thanks.
| * Monkey factory to create a ChaosMonkey that will not need access to ssh. It will not | ||
| * kill any services and it will not perform any restarts. | ||
| */ | ||
| @InterfaceAudience.Private |
Member
There was a problem hiding this comment.
Why are the monkey factories private? Is a user not expected to call them directly?
Contributor
Author
There was a problem hiding this comment.
I think MonkeyFactories are mostly expected as a param to ChaosMonkeyRunner and are not used directly, but if you think to make them LimitedPrivate is more helpful, then I'll change this.
| * <p> | ||
| * Chaos monkey can be run from the command line, or can be invoked from integration tests. | ||
| * See {@link org.apache.hadoop.hbase.IntegrationTestIngest} or other integration tests that use | ||
| * See {org.apache.hadoop.hbase.IntegrationTestIngest in hbase-it /test} or other integration tests that use |
Member
There was a problem hiding this comment.
Is it intentional to hide the class name? Is this valid java doc?
Contributor
Author
There was a problem hiding this comment.
IntegrationTestIngest is from src/test/java and was inaccessible as ChaosMonkey was moved to src/main/java.
Contributor
Author
I changed JIRA subject to match with git commit. |
Member
I'm very glad to hear you're able to make progress here. As an alternative to teasing out these APIs, have you considered contributing that |
Contributor
Author
Yes Definitely! |
Contributor
|
@lokiore please resolve the conflicts. |
added 6 commits
May 4, 2020 23:00
Contributor
Author
@virajjasani I have resolved all the conflicts |
Contributor
Author
Looks like some other PR got merged right away will resolve them as well |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
@virajjasani @ndimiduk please take a look, I have resolved all the conflicts. |
|
💔 -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. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
AssignmentTestingUtil, HBaseZKTestingUtility, MapReduceTestingShim and the most important HBaseCommonTestingUtility is also being moved to src/main/java/ ? Any chance we can avoid these classes? |
Contributor
|
@lokiore See queries above? Any chance of landing this one. You've done a load of work on it. |
Contributor
|
Closing. Open new PR after addressing the outstanding comments. Can refer to this PR in your new one. Thanks. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
No description provided.