laskoviymishka commented on code in PR #17791:
URL: https://github.com/apache/iceberg/pull/17791#discussion_r3958305555
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java:
##########
@@ -130,7 +130,8 @@ private static void checkAndUpdateGCProperties(
// To prevent accidental deletion of files that are still referenced by
other branches/tags,
// setting GC_ENABLED to 'false' is recommended, so that all Iceberg's gc
operations like
- // expire_snapshots, remove_orphan_files, drop_table with purge will fail
with an error.
+ // expire_snapshots, remove_orphan_files, drop_table with purge will fail
with an error unless
Review Comment:
"unless they are configured to skip file deletion" reads as if it applies to
all three, but only `expire_snapshots` has that knob (`CleanupLevel.NONE`) —
`remove_orphan_files` and drop-with-purge still hard-fail. I'd scope it: "…will
fail with an error; `expire_snapshots` can still run with `CleanupLevel.NONE`
to expire metadata without deleting files."
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -385,6 +384,15 @@ ExpireSnapshots withIncrementalCleanup(boolean
useIncrementalCleanup) {
return this;
}
+ private void validateCleanupLevel() {
Review Comment:
Moving the guard to a commit-time check is the right call, and validating
after `internalApply()` refreshes `base` is exactly what makes the gate see the
current `gc.enabled`.
One thing I'd tighten: `validateCleanupLevel()` reads the `base` field, so
it's only correct because it happens to be called right after `internalApply()`
reassigns it — that ordering isn't enforced anywhere. A future caller, or
`apply()`, or a subclass that calls it without refreshing first would silently
validate against stale metadata. I'd pass the metadata in explicitly,
`validateCleanupLevel(base)`, so the dependency is visible at the call site and
the method is testable on its own. wdyt?
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -369,6 +366,8 @@ public void commit() {
.run(
item -> {
TableMetadata updated = internalApply();
+ // validate after internalApply so that gc.enabled is read from
the refreshed base
+ validateCleanupLevel();
ops.commit(base, updated);
});
LOG.info(
Review Comment:
With `NONE` now a real path, this logs "prepare to clean up files at
level=NONE" right before the `CleanupLevel.NONE != cleanupLevel` guard skips
all cleanup — reads as a contradiction in the log. I'd branch it so the NONE
case says something like "committed, no file cleanup (level=NONE)". It's the
line an operator will stare at when debugging a gc-disabled expiry.
(`cleanupLevel.name()` a couple lines down can also just be `cleanupLevel` —
slf4j calls toString.)
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -584,17 +585,37 @@ public void testListTables() {
}
@Test
- public void testGCDisabled() {
+ public void testGCDisabled() throws IOException {
Table icebergTable = catalog.loadTable(TABLE_IDENTIFIER);
assertThat(icebergTable.properties()).containsEntry(TableProperties.GC_ENABLED,
"false");
+ String fileLocation = addRecordsToFile(icebergTable, "file");
+ DataFile file = makeDataFile(icebergTable, fileLocation);
+ icebergTable.newAppend().appendFile(file).commit();
+
+ long expiredSnapshotId = icebergTable.currentSnapshot().snapshotId();
+ String manifestListLocation =
+ icebergTable.currentSnapshot().manifestListLocation().replace("file:",
"");
+
+ icebergTable.newDelete().deleteFile(file).commit();
+
assertThatThrownBy(
() ->
icebergTable.expireSnapshots().expireOlderThan(System.currentTimeMillis()).commit())
.isInstanceOf(ValidationException.class)
- .hasMessage(
- "Cannot expire snapshots: GC is disabled (deleting files may
corrupt other tables)");
+ .hasMessageStartingWith("Cannot expire snapshots with cleanup level
ALL: GC is disabled");
+
+ icebergTable
+ .expireSnapshots()
+ .expireOlderThan(Long.MAX_VALUE)
+ .cleanupLevel(ExpireSnapshots.CleanupLevel.NONE)
+ .commit();
+
+ icebergTable.refresh();
+ assertThat(icebergTable.snapshot(expiredSnapshotId)).isNull();
+ assertThat(new File(fileLocation)).exists();
+ assertThat(new File(manifestListLocation)).exists();
Review Comment:
This is the assertion I'd most want added before this lands. You check the
manifest list survives, but for the Nessie case the files that actually matter
are the avro manifests inside it — those are what other branches share, and
"don't delete shared files" is the whole point of the NONE path. A regression
that deleted the manifests but left the manifest list would pass this test.
I'd capture the expired snapshot's manifest paths before the NONE expiry —
`icebergTable.snapshot(expiredSnapshotId).allManifests(icebergTable.io())`,
mapping each to `ManifestFile::path` — and assert each still exists afterward,
alongside the data file and manifest list you already check.
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -968,13 +968,102 @@ public void testWithExpiringStagedThenCherrypick() {
@TestTemplate
public void testExpireSnapshotsWhenGarbageCollectionDisabled() {
+ table.newAppend().appendFile(FILE_A).commit();
+ Snapshot firstSnapshot = table.currentSnapshot();
+ table.newAppend().appendFile(FILE_B).commit();
+ long tAfterCommits =
waitUntilAfter(table.currentSnapshot().timestampMillis());
+
table.updateProperties().set(TableProperties.GC_ENABLED, "false").commit();
+ assertThatThrownBy(() ->
removeSnapshots(table).expireOlderThan(tAfterCommits).commit())
+ .isInstanceOf(ValidationException.class)
+ .hasMessageStartingWith("Cannot expire snapshots with cleanup level
ALL: GC is disabled");
+
+ assertThatThrownBy(
+ () ->
+ removeSnapshots(table)
+ .expireOlderThan(tAfterCommits)
+ .cleanupLevel(ExpireSnapshots.CleanupLevel.METADATA_ONLY)
+ .commit())
+ .isInstanceOf(ValidationException.class)
+ .hasMessageStartingWith(
+ "Cannot expire snapshots with cleanup level METADATA_ONLY: GC is
disabled");
+
+ // apply() deletes nothing, so it is not blocked by GC being disabled
+ assertThat(removeSnapshots(table).expireOlderThan(tAfterCommits).apply())
+ .containsExactly(firstSnapshot);
+
+ assertThat(table.snapshot(firstSnapshot.snapshotId())).isNotNull();
+ }
+
+ @TestTemplate
+ public void testExpireSnapshotsWithoutCleanupWhenGarbageCollectionDisabled()
{
table.newAppend().appendFile(FILE_A).commit();
+ Snapshot firstSnapshot = table.currentSnapshot();
+ table.newDelete().deleteFile(FILE_A).commit();
+ Snapshot secondSnapshot = table.currentSnapshot();
+ table.newAppend().appendFile(FILE_B).commit();
+ Snapshot currentSnapshot = table.currentSnapshot();
+ long tAfterCommits = waitUntilAfter(currentSnapshot.timestampMillis());
+
+ table.updateProperties().set(TableProperties.GC_ENABLED, "false").commit();
+
+ Set<String> deletedFiles = Sets.newHashSet();
+ removeSnapshots(table)
+ .expireOlderThan(tAfterCommits)
+ .cleanupLevel(ExpireSnapshots.CleanupLevel.NONE)
+ .deleteWith(deletedFiles::add)
+ .commit();
- assertThatThrownBy(() -> table.expireSnapshots())
+ assertThat(table.snapshot(firstSnapshot.snapshotId())).isNull();
+ assertThat(table.snapshot(secondSnapshot.snapshotId())).isNull();
+ assertThat(table.currentSnapshot()).isEqualTo(currentSnapshot);
+ assertThat(deletedFiles).isEmpty();
+ }
+
+ @TestTemplate
+ public void testDisableGarbageCollectionBeforeCommit() {
+ table.newAppend().appendFile(FILE_A).commit();
+ Snapshot firstSnapshot = table.currentSnapshot();
+ table.newAppend().appendFile(FILE_B).commit();
+ long tAfterCommits =
waitUntilAfter(table.currentSnapshot().timestampMillis());
+
+ Set<String> deletedFiles = Sets.newHashSet();
+ ExpireSnapshots expireSnapshots =
+ removeSnapshots(table)
+ .expireOlderThan(tAfterCommits)
+ .cleanupLevel(ExpireSnapshots.CleanupLevel.ALL)
+ .deleteWith(deletedFiles::add);
+
+ table.updateProperties().set(TableProperties.GC_ENABLED, "false").commit();
+
+ assertThatThrownBy(expireSnapshots::commit)
.isInstanceOf(ValidationException.class)
- .hasMessageStartingWith("Cannot expire snapshots: GC is disabled");
+ .hasMessageStartingWith("Cannot expire snapshots with cleanup level
ALL: GC is disabled");
+
+ assertThat(table.snapshot(firstSnapshot.snapshotId())).isNotNull();
+ assertThat(deletedFiles).isEmpty();
+ }
+
+ @TestTemplate
+ public void testDisableGarbageCollectionBeforeCommitWithoutCleanup() {
+ table.newAppend().appendFile(FILE_A).commit();
+ Snapshot firstSnapshot = table.currentSnapshot();
+ table.newAppend().appendFile(FILE_B).commit();
+ long tAfterCommits =
waitUntilAfter(table.currentSnapshot().timestampMillis());
+
+ Set<String> deletedFiles = Sets.newHashSet();
+ ExpireSnapshots expireSnapshots =
+ removeSnapshots(table)
+ .expireOlderThan(tAfterCommits)
+ .cleanupLevel(ExpireSnapshots.CleanupLevel.NONE)
+ .deleteWith(deletedFiles::add);
+
+ table.updateProperties().set(TableProperties.GC_ENABLED, "false").commit();
+ expireSnapshots.commit();
+
+ assertThat(table.snapshot(firstSnapshot.snapshotId())).isNull();
+ assertThat(deletedFiles).isEmpty();
Review Comment:
Could you assert the current snapshot survived here too?
`testExpireSnapshotsWithoutCleanupWhenGarbageCollectionDisabled` does it
(`assertThat(table.currentSnapshot())...`), and without it a regression that
expired everything would still pass, since `deletedFiles` is empty either way.
##########
api/src/main/java/org/apache/iceberg/ExpireSnapshots.java:
##########
@@ -143,6 +143,10 @@ enum CleanupLevel {
* <p>Consider {@link CleanupLevel#NONE} when data and metadata files may be
more efficiently
* removed using a distributed framework through the actions API.
*
+ * <p>When the table property {@code gc.enabled} is false, any level other
than {@link
+ * CleanupLevel#NONE} causes {@link #commit()} to fail: the expiration is
not committed and no
Review Comment:
Good to document the gc.enabled behavior on the level here. While you're in
this file — the class-level javadoc up top still says manifest and data files
"will be deleted" unconditionally, which isn't true anymore for the `NONE`
path. Worth a line up there pointing at `cleanupLevel(CleanupLevel)` so the two
don't contradict.
--
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]
