jbonofre commented on code in PR #18762:
URL: https://github.com/apache/iceberg/pull/18762#discussion_r4525609108
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -2373,4 +2375,58 @@ public void
testAppendOnlyManifestsNotScannedDuringCleanup() {
appendOnlyManifestPaths.forEach(
path -> Mockito.verify(spyFileIO, Mockito.never()).newInputFile(path));
}
+
+ @TestTemplate
+ public void testFailedRetainedManifestListReadDoesNotDeleteLiveManifests() {
+ TestTables.LocalFileIO spyFileIO = Mockito.spy(new
TestTables.LocalFileIO());
+ String tableName = "testFailedRetainedManifestListRead";
+ Table testTable =
+ TestTables.create(
+ tableDir,
+ tableName,
+ SCHEMA,
+ SPEC,
+ SortOrder.unsorted(),
+ formatVersion,
+ new TestTables.TestTableOperations(tableName, tableDir,
spyFileIO));
+
+ testTable.newFastAppend().appendFile(FILE_A).commit();
+ Snapshot firstSnapshot = testTable.currentSnapshot();
+ Set<String> firstSnapshotManifests =
+ firstSnapshot.allManifests(testTable.io()).stream()
+ .map(ManifestFile::path)
+ .collect(Collectors.toSet());
+
+ waitUntilAfter(firstSnapshot.timestampMillis());
+
+ testTable.newFastAppend().appendFile(FILE_B).commit();
+ Snapshot secondSnapshot = testTable.currentSnapshot();
+ long tAfterCommits = waitUntilAfter(secondSnapshot.timestampMillis());
+
+ // the retained snapshot still references the first snapshot's manifest
+ assertThat(
+ secondSnapshot.allManifests(testTable.io()).stream()
+ .map(ManifestFile::path)
+ .collect(Collectors.toSet()))
+ .containsAll(firstSnapshotManifests);
+
+ // the manifest list of the retained snapshot cannot be read during cleanup
+ Mockito.doThrow(new RuntimeIOException("Injected failure"))
+ .when(spyFileIO)
+ .newInputFile(secondSnapshot.manifestListLocation());
+
+ Set<String> deletedFiles = Sets.newHashSet();
+ Throwable thrown =
+ catchThrowable(
+ () ->
+ removeSnapshots(testTable)
+ .expireOlderThan(tAfterCommits)
+ .deleteWith(deletedFiles::add)
+ .commit());
+
+
assertThat(deletedFiles).doesNotContainAnyElementsOf(firstSnapshotManifests);
+ assertThat(thrown)
+ .isInstanceOf(RuntimeIOException.class)
+ .hasMessageContaining("Injected failure");
Review Comment:
The description relies on the expiration being committed before cleanup
fails, but the test doesn't check that. If the injected failure ever came out
before `ops.commit`, the test would still pass with no snapshots expired.
Can we assert that the commit happened and that cleanup was aborted
entirely? `isEmpty()` also covers the current `doesNotContainAnyElementOf`
check.
```suggestion
assertThat(thrown)
.isInstanceOf(RuntimeIOException.class)
.hasMessageContaining("Injected failure");
testTable.refresh();
assertThat(testTable.snapshot(firstSnapshot.snapshotId())).isNull();
assertThat(deletedFiles).isEmpty();
```
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -2373,4 +2375,58 @@ public void
testAppendOnlyManifestsNotScannedDuringCleanup() {
appendOnlyManifestPaths.forEach(
path -> Mockito.verify(spyFileIO, Mockito.never()).newInputFile(path));
}
+
+ @TestTemplate
+ public void testFailedRetainedManifestListReadDoesNotDeleteLiveManifests() {
Review Comment:
The injected failure goes through `Tasks.retry(3)` with the default 1s/2s/4s
back-off, so each run sleeps about 7s before the exception comes out.
The template runs 8 times, which adds roughly 56s to the core tests. As the
description says, the reachable cleanup runs already pass on `main`, so they
don't exercise this change.
Can we limit the test to incremental cleanup?
```suggestion
public void testFailedRetainedManifestListReadDoesNotDeleteLiveManifests()
{
assumeThat(incrementalCleanup).isTrue();
```
--
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]
