HDDS-10934. Refactor TestOzoneRpcClient hierarchy #6747
Merged
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
errose28
reviewed
May 29, 2024
There was a problem hiding this comment.
Overall fix makes sense. There's some other weird stuff in this area, not sure if we want to handle it in this PR or a follow up:
- The tests run for both ratis and non-ratis cases (in
TestOzoneRpcClientAbstract) and those run only for Ratis cases (inTestOzoneRpcClientWithRatis) seem kind of arbitrary.- Maybe we can either unify them so all tests run in both setups, or only a minimal amount of validation is done in the non-ratis setup.
TestSecureOzoneRpcClientextendsTestOzoneRpcClient, so now the secure tests are only running in a non-ratis OM setup.- Also tests that are unique to
TestOzoneRpcClientWithRatisare not run in a secure setup.
- Also tests that are unique to
|
Thanks @errose28 for the review.
Agree, we should move them to the parent class.
This is not the case, since Will push a few more commits. |
adoroszlai
changed the title
|
@errose28 patch is updated, please take another look |
errose28
approved these changes
May 31, 2024
|
Thanks @errose28 for the review. |
jojochuang
pushed a commit
to jojochuang/ozone
that referenced
this pull request
Jun 15, 2024
(cherry picked from commit 385c4ec) Change-Id: I99e2c6332816ea345c11928d9d1182642beb22c7
jojochuang
pushed a commit
to jojochuang/ozone
that referenced
this pull request
Jun 17, 2024
(cherry picked from commit 385c4ec) Conflicts: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestSecureOzoneRpcClient.java Change-Id: I78df84af142772a188f48680b5e1aaa189afd7c3
jojochuang
pushed a commit
to jojochuang/ozone
that referenced
this pull request
Jun 17, 2024
(cherry picked from commit 385c4ec) Conflicts: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestSecureOzoneRpcClient.java Change-Id: I78df84af142772a188f48680b5e1aaa189afd7c3 HDDS-10934 amendment Change-Id: I7dfb930a6d113315dc6652a2d68e83337cdca752
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.
What changes were proposed in this pull request?
Refactor
TestOzoneRpcClientAbstractand its subclasses:TestOzoneRpcClientandTestOzoneRpcClientWithRatisare subclasses ofTestOzoneRpcClientAbstract.They used to execute the same test cases with OM Ratis disabled (relying on default setting) and enabled (explicitly), respectively. HDDS-4498 enabled OM Ratis by default, so now both classes test with OM Ratis enabled. This change disables OM Ratis forTestOzoneRpcClient, and fixes one test case that was failing with NPE in this setup.TestSecureOzoneRpcClientshould directly extendTestOzoneRpcClientAbstractto avoid confusion caused by shadowed lifecycle methods.MiniOzoneCluster.BuildertostartClusterto avoid code duplication inTestSecureOzoneRpcClient.TestOzoneRpcClientAbstracttoOzoneRpcClientTeststo follow naming conventions.TestOzoneRpcClientWithRatisto the base class to extend coverage for other configs.(Items 2-5 were added after initial round of review.)
https://issues.apache.org/jira/browse/HDDS-10934
How was this patch tested?
CI:
https://github.com/adoroszlai/ozone/actions/runs/9282222672/job/25540423674#step:5:2276