smengcl commented on code in PR #11651:
URL: https://github.com/apache/ozone/pull/11651#discussion_r4393013274
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDiffManager.java:
##########
@@ -1644,21 +1841,74 @@ public void
testSubmitSnapshotDiffResubmitsWhenPreviouslyCancelled()
}
@Test
- public void testUpdateJobStatusToDoneSkipsSupersededJob() throws IOException
{
+ public void testUpdateJobStatusToDone() throws IOException {
SnapDiffTestContext ctx = setupRandomSnapDiffTestContext();
String liveJobId = UUID.randomUUID().toString();
snapshotDiffManager.getSnapDiffJobTable().put(ctx.diffJobKey,
new SnapshotDiffJob(0L, liveJobId, IN_PROGRESS, ctx.volumeName,
ctx.bucketName,
ctx.fromSnapshotName, ctx.toSnapshotName, false, false, 0L, null,
0.0, null));
// A superseded task's publish is ignored, the live job's own publish goes
through.
- snapshotDiffManager.updateJobStatusToDone(ctx.diffJobKey,
UUID.randomUUID().toString(), 5L, "staleKey");
+ snapshotDiffManager.updateJobStatusToDone(ctx.diffJobKey,
UUID.randomUUID().toString(), 5L, "staleKey", null);
+ assertEquals(IN_PROGRESS,
snapshotDiffManager.getSnapDiffJobTable().get(ctx.diffJobKey).getStatus());
+
+ snapshotDiffManager.updateJobStatusToDone(ctx.diffJobKey, liveJobId, 5L,
"liveKey", null);
+ SnapshotDiffJob legacyDone =
snapshotDiffManager.getSnapDiffJobTable().get(ctx.diffJobKey);
+ assertEquals(DONE, legacyDone.getStatus());
+ assertEquals("liveKey", legacyDone.getLargestEntryKey());
+ assertFalse(legacyDone.isOrderedReport());
+
assertTrue(org.apache.commons.lang3.StringUtils.isEmpty(legacyDone.getReason()));
+
+ SnapDiffTestContext legacyCtx = setupRandomSnapDiffTestContext();
+ String legacyJobId = UUID.randomUUID().toString();
+ SnapshotDiffJob legacyJob = new SnapshotDiffJob(0L, legacyJobId,
IN_PROGRESS, legacyCtx.volumeName,
+ legacyCtx.bucketName, legacyCtx.fromSnapshotName,
legacyCtx.toSnapshotName, false, false,
+ 0L, null, 0.0, null);
+ legacyJob.setOrderedReport(true);
+ snapshotDiffManager.getSnapDiffJobTable().put(legacyCtx.diffJobKey,
legacyJob);
+ snapshotDiffManager.updateJobStatusToDone(legacyCtx.diffJobKey,
legacyJobId, 3L, "legacyKey", null);
+ SnapshotDiffJob legacyPreserved =
snapshotDiffManager.getSnapDiffJobTable().get(legacyCtx.diffJobKey);
+ assertTrue(legacyPreserved.isOrderedReport());
+
assertTrue(org.apache.commons.lang3.StringUtils.isEmpty(legacyPreserved.getReason()));
+
+ SnapDiffTestContext optimizedCtx = setupRandomSnapDiffTestContext();
+ String optimizedJobId = UUID.randomUUID().toString();
+ snapshotDiffManager.getSnapDiffJobTable().put(optimizedCtx.diffJobKey,
+ new SnapshotDiffJob(0L, optimizedJobId, IN_PROGRESS,
optimizedCtx.volumeName,
+ optimizedCtx.bucketName, optimizedCtx.fromSnapshotName,
optimizedCtx.toSnapshotName,
+ false, false, 0L, null, 0.0, null));
+ snapshotDiffManager.updateJobStatusToDone(optimizedCtx.diffJobKey,
optimizedJobId, 4L, "optKey", false);
+ SnapshotDiffJob unorderedDone =
snapshotDiffManager.getSnapDiffJobTable().get(optimizedCtx.diffJobKey);
+ assertFalse(unorderedDone.isOrderedReport());
+ assertEquals("Report is dependency unordered.", unorderedDone.getReason());
+
+ SnapDiffTestContext orderedCtx = setupRandomSnapDiffTestContext();
+ String orderedJobId = UUID.randomUUID().toString();
+ snapshotDiffManager.getSnapDiffJobTable().put(orderedCtx.diffJobKey,
+ new SnapshotDiffJob(0L, orderedJobId, IN_PROGRESS,
orderedCtx.volumeName, orderedCtx.bucketName,
+ orderedCtx.fromSnapshotName, orderedCtx.toSnapshotName, false,
false, 0L, null, 0.0, null));
+ snapshotDiffManager.updateJobStatusToDone(orderedCtx.diffJobKey,
orderedJobId, 2L, "ordKey", true);
+ SnapshotDiffJob orderedDone =
snapshotDiffManager.getSnapDiffJobTable().get(orderedCtx.diffJobKey);
+ assertTrue(orderedDone.isOrderedReport());
Review Comment:
This will fail now: the job above is created with `orderedReport=false`, and
`updateJobStatusToDone(..., true)` no longer touches the flag. The flag should
stay as requested, which is what the fix intends:
```suggestion
assertFalse(orderedDone.isOrderedReport());
```
It would also be good to cover the case the fix is for: an `--ordered`
submit against a DONE FSO job that was requested ordered but whose ordering
fell back (reason "Report is dependency unordered.") should not create a new
job or call `generateSnapshotDiffReport`.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotObjectStore.java:
##########
@@ -18,13 +18,54 @@
package org.apache.hadoop.ozone.om.snapshot;
import static org.apache.hadoop.ozone.om.helpers.BucketLayout.OBJECT_STORE;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.io.IOException;
+import org.apache.hadoop.hdfs.protocol.SnapshotDiffReport;
+import org.apache.hadoop.ozone.client.OzoneBucket;
+import org.apache.hadoop.ozone.snapshot.SnapshotDiffReportOzone;
+import org.apache.hadoop.ozone.snapshot.SnapshotDiffResponse;
+import org.apache.ozone.test.GenericTestUtils;
+import org.junit.jupiter.api.Test;
/**
* Test OmSnapshot for Object Store bucket type.
*/
public class TestOmSnapshotObjectStore extends OmSnapshotTests {
public TestOmSnapshotObjectStore() throws Exception {
- super(OBJECT_STORE, false, false, false, false);
+ super(OBJECT_STORE, false, false, false, false, true);
+ }
+
+ @Test
+ public void testOptimizedObsDiff() throws Exception {
+ String snap1 = "snap1";
+ String snap2 = "snap2";
+ String key1 = "key1";
+ String key2 = "key2";
+ String renamedKey = "key3";
+ OzoneBucket bucket = getOzoneBucket();
Review Comment:
Same here: using a fresh volume and bucket instead of the shared one (and
its fixed `snap1`/`snap2` names) keeps this independent of other tests in the
suite.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotFsoWithNativeLib.java:
##########
@@ -28,6 +40,42 @@
@EnabledIfSystemProperty(named = ROCKS_TOOLS_NATIVE_PROPERTY, matches = "true")
class TestOmSnapshotFsoWithNativeLib extends OmSnapshotTests {
TestOmSnapshotFsoWithNativeLib() throws Exception {
- super(FILE_SYSTEM_OPTIMIZED, false, false, false, false);
+ super(FILE_SYSTEM_OPTIMIZED, false, false, false, false, true);
+ }
+
+ @Test
+ void testOptimizedDiffUnorderedThenOrdered() throws Exception {
+ String snap1 = "snap1";
Review Comment:
This writes to the class wide shared bucket with fixed `snap1`/`snap2`
names, while the other diff tests in `OmSnapshotTests` create their own `"vol"
+ counter` volume and bucket. Another test already creates and deletes `dir1`
in the shared bucket, so leftovers from it (or from a failed run) would change
this exact `assertEquals` depending on test order. Creating a fresh volume and
FSO bucket here, like the neighbouring tests, avoids that.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotFsoWithNativeLib.java:
##########
@@ -28,6 +40,42 @@
@EnabledIfSystemProperty(named = ROCKS_TOOLS_NATIVE_PROPERTY, matches = "true")
class TestOmSnapshotFsoWithNativeLib extends OmSnapshotTests {
TestOmSnapshotFsoWithNativeLib() throws Exception {
- super(FILE_SYSTEM_OPTIMIZED, false, false, false, false);
+ super(FILE_SYSTEM_OPTIMIZED, false, false, false, false, true);
Review Comment:
With this (and `TestOmSnapshotObjectStore`) switched to `true`, no subclass
runs native DAG diff through the legacy pipeline on FSO any more, although that
is still the default path (`ozone.om.snapshot.diff.optimized.enabled=false`).
The heavyweight tests pinned by `assumeCanonicalConfig(true)` also only run on
the optimized path now. Could these two keep `false`, with one new subclass
(e.g. `TestOmSnapshotFsoWithNativeLibOptimized`) turning the flag on and
holding the optimized only tests? Either way, worth running the whole suite on
the optimized path once, since several existing tests compare exact diff list
order.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
