-
Notifications
You must be signed in to change notification settings - Fork 594
HDDS-12715. Add integration tests for debug-replicas-verify-checksums tool #8209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDDS-12715. Add integration tests for debug-replicas-verify-checksums tool #8209
Conversation
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ptlrs for working on this.
Moving cluster creation to an extension reduces duplication, but:
- test execution is still unnecessarily long, because each test class needs to start its own cluster
- the specific implementation does not allow config to be customized if needed
Please check TestOzoneIntegrationNonHA and its parent classes for a better way to reuse a shared cluster for multiple test classes (HDDS-12183 and several later sub-tasks of HDDS-9000).
|
Thanks for the review @adoroszlai. With the custom extension, the cluster is created only once for both the test classes. The cluster is initialized once and stored as a static member. I had confirmed this by logging the creation of MiniOzoneCluster. Is the goal of the new approach to consolidate all tests that can reuse the same config/cluster? |
42555b8 to
68fa7bb
Compare
|
Hi @adoroszlai, the PR has been updated to use the existing TestOzoneIntegrationNonHA class. Could you please take another look. |
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ptlrs for updating the patch.
I think the PR title also needs to be updated.
...tegration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneDebugReplicasVerify.java
Outdated
Show resolved
Hide resolved
...tegration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneDebugReplicasVerify.java
Outdated
Show resolved
Hide resolved
...tegration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneDebugReplicasVerify.java
Outdated
Show resolved
Hide resolved
...tegration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneDebugReplicasVerify.java
Outdated
Show resolved
Hide resolved
...tegration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneDebugReplicasVerify.java
Outdated
Show resolved
Hide resolved
857aae0 to
a88c216
Compare
a88c216 to
0674e6c
Compare
|
Hi @adoroszlai @dombizita, could you please review this updated PR when you get a chance? Thanks. |
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ptlrs for updating the patch.
...tegration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneDebugReplicasVerify.java
Show resolved
Hide resolved
…-integration-test-cluster-for-debug-and-repair-tools # Conflicts: # hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestContainerScannerIntegrationAbstract.java
|
Hi @adoroszlai, the test has been updated. Could you please take another look. |
dombizita
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating your patch @ptlrs, it looks good to me! Please update the jira title, so it'll match with the actual implementation. One more thing, there is also a --block-existence and --container-state flag for the ozone debug replicas verify tool, we have acceptance tests for it, do you think it'd a good idea to add tests for those in this patch? I know this PR is pending for a while, so I'm fine with this state as well.
|
Thanks for the review @dombizita. Let's open a new task for adding the tests for the block-existance and container-state features. This PR is in continuation of the work that was done for the checksums tool. I am not familiar with the other two features, so a new task to handle it spearately would be better. |
|
@ptlrs sounds good to me, thanks! Could you please merge latest master to this branch? It's been a month since the last time, once that's done I'll start CI and if all looks good we can merge! |
…ntegration-test-cluster-for-debug-and-repair-tools Change-Id: I8c4bffdd79009c1755e89719ac3e79d0490515d0
sarvekshayr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @ptlrs.
Tests for the remaining verification flags can either be added in a follow-up JIRA or skipped since we already have acceptance tests covering them.
Overall, this LGTM.
Tejaskriya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @ptlrs , LGTM
|
Thanks for the reviews @dombizita @sarvekshayr @adoroszlai , and the contribution @ptlrs |
Please describe your PR in detail:
NonHATestsframework to obtain a shared MiniOzoneCluster that can be used by other CLI testsWhat is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-12715
How was this patch tested?
CI: https://github.com/ptlrs/ozone/actions/runs/16375920556