allthingssecurity commented on code in PR #18762:
URL: https://github.com/apache/iceberg/pull/18762#discussion_r4532689437
##########
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:
Good catch, thanks. You're right that the reachable runs spend ~7s each on a
path that already passes on `main`, so they're pure overhead here. Taking the
suggestion.
That still leaves 4 runs (~28s), since the `Tasks` back-off isn't injectable
from the test. This cleanup path is format-version independent, so I'm happy to
pin `formatVersion` as well and get it down to a single run if you'd prefer --
I didn't want to narrow the coverage without asking.
##########
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:
Agreed, that's a real hole. Without asserting the commit landed, the test
would still pass if the failure ever escaped before `ops.commit()` -- which is
exactly the regression it's supposed to catch. Taking the suggestion, and I'll
drop the `doesNotContainAnyElementsOf` line above it as well, since `isEmpty()`
subsumes it.
--
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]
