HBASE-27938 - PE load any custom implementation of tests at runtime #5307
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
virajjasani
reviewed
Jun 22, 2023
There was a problem hiding this comment.
Thanks @gvprathyusha6, could you please also update printUsage with this new option?
protected static void printUsage(final String shortName, final String message) {
|
💔 -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. |
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
requested changes
Jun 27, 2023
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Instead of Command name we need to give the classname of Test impl and it should work as is.
That is true, but if the custom test impl is very much usecase specific and isnt generic enough to be contributed back we wont be able to utilise the framework code completely, because as of now it mandates to have a compile time dependency of the implemented test class. |
So we need to explicitly say that in the printUsage. I couldn't get that by the
What I meant was to put some notes on the description for this generic command option. Something like "Please consider contribute back this custom command into a builtin PE command for the benefit of the community". |
|
Got it, how about we update
And add a section for Class just below Commad: Seems inline with current documentation standard also to me |
Yeah, that would be good. Thanks! |
gvprathyusha6
force-pushed
the
HBASE-27938
branch
from
6a7fe01 to
fc77191
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
approved these changes
Aug 10, 2023
wchevreuil
reviewed
Aug 10, 2023
There was a problem hiding this comment.
LGTM, but need to fix the spotless reported issue before I can merge this, @gvprathyusha6 .
Apache9
reviewed
Aug 13, 2023
| @@ -3089,7 +3115,20 @@ public int run(String[] args) throws Exception { | |||
| } | |||
|
|
|||
| private static boolean isCommandClass(String cmd) { | |||
| return COMMANDS.containsKey(cmd); | |||
| return !COMMANDS.containsKey(cmd) ? isCustomTestClass(cmd) : true; | |||
There was a problem hiding this comment.
return COMMANDS.containsKey(cmd) || isCustomTestClass(cmd);
|
Could you please provide a UT or add an example in hbase-examples to show how to make use of this feature? Thanks. |
gvprathyusha6
force-pushed
the
HBASE-27938
branch
from
fc77191 to
986f71b
Compare
|
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
May 9, 2024
| @@ -2308,7 +2316,7 @@ protected byte[] generateRow(final int i) { | |||
| } | |||
|
|
|||
| @Override | |||
| boolean testRow(final int i, final long startTime) throws IOException { | |||
| protected boolean testRow(final int i, final long startTime) throws IOException { | |||
There was a problem hiding this comment.
Why change this to protected?
There was a problem hiding this comment.
to override this in impl class, so that we can customise row format. If the custom test knows the dataset, it would be beneficial to repro any specific scenario, like if I have the rowkey similar to what tsdb puts, I can create scan load on particular dataset and write load on another and try to get closer to prod scenarios
There was a problem hiding this comment.
But the methods in TestBase class are all package private? Even if you change this to protected, you can not inherit this class in other package because you can not call the constructor?
There was a problem hiding this comment.
I was managing them by using the same package name, removing it
gvprathyusha6
force-pushed
the
HBASE-27938
branch
from
05b01ea to
29af40a
Compare
|
🎊 +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. |
virajjasani
approved these changes
May 13, 2024
Apache9
approved these changes
May 14, 2024
gvprathyusha6
force-pushed
the
HBASE-27938
branch
from
29af40a to
13d3715
Compare
|
🎊 +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. |
…test class and properties
gvprathyusha6
force-pushed
the
HBASE-27938
branch
from
13d3715 to
94d76fe
Compare
|
🎊 +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. |
|
@gvprathyusha6 could you also create branch-2 PR? we can merge both of them together and backport as required. |
This was referenced May 14, 2024
virajjasani
pushed a commit
that referenced
this pull request
May 15, 2024
) (#5899) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
virajjasani
pushed a commit
that referenced
this pull request
May 15, 2024
) (#5899) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
virajjasani
pushed a commit
that referenced
this pull request
May 15, 2024
) (#5899) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
May 21, 2024
…ache#5307) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
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.
Enable PE to load any custom implementation of tests at runtime