HDDS-8389. [Snapshot] Added integration test for SnapDiff when OM leader failover happens #4657
Conversation
hemantk-12
force-pushed
the
HDDS-8389
branch
4 times, most recently
from
1313363 to
116cd7c
Compare
…der failover happens
|
@swamirishi Could you take a look at this? |
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshot.java
Outdated
Show resolved
Hide resolved
...ne/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHASnapshot.java
Outdated
Show resolved
Hide resolved
...ne/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHASnapshot.java
Outdated
Show resolved
Hide resolved
...one/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java
Outdated
Show resolved
Hide resolved
smengcl
left a comment
smengcl
left a comment
There was a problem hiding this comment.
Thanks @hemantk-12 . Great test additions. Nicely written.
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshot.java
Outdated
Show resolved
Hide resolved
|
|
||
| // If job was IN_PROGRESS or DONE state when OM restarted, it should be | ||
| // DONE by this time. | ||
| // If job FAILED during crash (which mostly happens in the test because |
There was a problem hiding this comment.
nit: Do we have a unit test case for the FAILED state?
There was a problem hiding this comment.
No integration test for FAILED state. I added few tests as unit test in PR: https://github.com/apache/ozone/pull/4716/files#diff-11392a9810b911a6b87f9f4440cb5e5342b2a9c112851e7dceafa5103d999e1dR427
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshot.java
Outdated
Show resolved
Hide resolved
| }); | ||
| } | ||
|
|
||
| assertEquals(1, snapshotIds.stream().distinct().count()); |
There was a problem hiding this comment.
Is snapshotId assertion enough? I guess we should also check rocks db contents.
There was a problem hiding this comment.
Checking only snapshotId should be enough. Test is to make sure that snapshotId is consistent among all the OM nodes. We don't care about it value.
What do you mean by "check rocks db contents"?
There was a problem hiding this comment.
the list of sst files should be same I believe.
There was a problem hiding this comment.
I don't think that should be tested as part of this test.
...ne/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHASnapshot.java
Outdated
Show resolved
Hide resolved
|
There are some conflicts in the test due to HDDS-8645. |
Resolved conflicts and also ran test 100 times to make sure there is no flakiness. It passed all the time (Workflow failed because of timeout) : https://github.com/hemantk-12/ozone/actions/runs/5049594952/jobs/9059656428 |
|
Thanks @hemantk-12 for the test addition, and @swamirishi for the review. |
4 participants
What changes were proposed in this pull request?
snapshotDiffManagerand invalidatesnapshotCacheuponOmSnapshotManager#close.snapDiffExecutorandsstDumpToolExecutorinSnapshotDiffManager#close.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8389
How was this patch tested?
Ran newly added tests locally and verified they pass.