You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iceberg.apache.org by am...@apache.org on 2023/12/19 17:03:57 UTC
(iceberg) branch 1.4.x updated: [1.4.x] Core: Expired Snapshot files in a transaction should be deleted (#9223)
This is an automated email from the ASF dual-hosted git repository.
amoghj pushed a commit to branch 1.4.x
in repository https://gitbox.apache.org/repos/asf/iceberg.git
The following commit(s) were added to refs/heads/1.4.x by this push:
new 63e0204f1c [1.4.x] Core: Expired Snapshot files in a transaction should be deleted (#9223)
63e0204f1c is described below
commit 63e0204f1cb2c1efc6c7dd91c482f15c1ddd4328
Author: Andrew Sherman <as...@cloudera.com>
AuthorDate: Tue Dec 19 09:03:50 2023 -0800
[1.4.x] Core: Expired Snapshot files in a transaction should be deleted (#9223)
* Core: Expired Snapshot files in a transaction should be deleted. (#9183)
When a snapshot is expired as part of a transaction, the manifest list
file(s) should be deleted when the transaction commits. A recent change
(#6634) ensured that these files are not deleted when they have also
been committed as part of a transaction, but this breaks the simple
case where no new files are committed. Fix this by not skipping
deletion when the list of committed files is empty.
* Core: Fix logic for determining set of committed files in BaseTransaction when there are no new snapshots (#9221)
---------
Co-authored-by: Amogh Jahagirdar <am...@tabular.io>
---
core/src/main/java/org/apache/iceberg/BaseTransaction.java | 5 ++++-
core/src/test/java/org/apache/iceberg/TableTestBase.java | 9 +++++++++
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java | 7 ++++++-
.../java/org/apache/iceberg/TestSequenceNumberForV2Table.java | 8 ++++++++
4 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/core/src/main/java/org/apache/iceberg/BaseTransaction.java b/core/src/main/java/org/apache/iceberg/BaseTransaction.java
index c1ebd554be..5abb69da01 100644
--- a/core/src/main/java/org/apache/iceberg/BaseTransaction.java
+++ b/core/src/main/java/org/apache/iceberg/BaseTransaction.java
@@ -44,6 +44,7 @@ import org.apache.iceberg.metrics.LoggingMetricsReporter;
import org.apache.iceberg.metrics.MetricsReporter;
import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.apache.iceberg.util.PropertyUtil;
@@ -505,9 +506,11 @@ public class BaseTransaction implements Transaction {
}
}
+ // committedFiles returns null whenever the set of committed files
+ // cannot be determined from the provided snapshots
private static Set<String> committedFiles(TableOperations ops, Set<Long> snapshotIds) {
if (snapshotIds.isEmpty()) {
- return null;
+ return ImmutableSet.of();
}
Set<String> committedFiles = Sets.newHashSet();
diff --git a/core/src/test/java/org/apache/iceberg/TableTestBase.java b/core/src/test/java/org/apache/iceberg/TableTestBase.java
index d50845933e..5ff97f42c3 100644
--- a/core/src/test/java/org/apache/iceberg/TableTestBase.java
+++ b/core/src/test/java/org/apache/iceberg/TableTestBase.java
@@ -212,6 +212,15 @@ public class TableTestBase {
&& Files.getFileExtension(name).equalsIgnoreCase("avro")));
}
+ List<File> listManifestLists(String tableDirToList) {
+ return Lists.newArrayList(
+ new File(tableDirToList, "metadata")
+ .listFiles(
+ (dir, name) ->
+ name.startsWith("snap")
+ && Files.getFileExtension(name).equalsIgnoreCase("avro")));
+ }
+
public static long countAllMetadataFiles(File tableDir) {
return Arrays.stream(new File(tableDir, "metadata").listFiles())
.filter(f -> f.isFile())
diff --git a/core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java b/core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java
index 71455c5712..fc3aa6c916 100644
--- a/core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java
+++ b/core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java
@@ -440,7 +440,10 @@ public class TestRemoveSnapshots extends TableTestBase {
t3 = System.currentTimeMillis();
}
- // Retain last 2 snapshots
+ Assert.assertEquals(
+ "Should be 3 manifest lists", 3, listManifestLists(table.location()).size());
+
+ // Retain last 2 snapshots, which means 1 is deleted.
Transaction tx = table.newTransaction();
removeSnapshots(tx.table()).expireOlderThan(t3).retainLast(2).commit();
tx.commitTransaction();
@@ -449,6 +452,8 @@ public class TestRemoveSnapshots extends TableTestBase {
"Should have two snapshots.", 2, Lists.newArrayList(table.snapshots()).size());
Assert.assertEquals(
"First snapshot should not present.", null, table.snapshot(firstSnapshotId));
+ Assert.assertEquals(
+ "Should be 2 manifest lists", 2, listManifestLists(table.location()).size());
}
@Test
diff --git a/core/src/test/java/org/apache/iceberg/TestSequenceNumberForV2Table.java b/core/src/test/java/org/apache/iceberg/TestSequenceNumberForV2Table.java
index 08bdc64a08..86842b6812 100644
--- a/core/src/test/java/org/apache/iceberg/TestSequenceNumberForV2Table.java
+++ b/core/src/test/java/org/apache/iceberg/TestSequenceNumberForV2Table.java
@@ -309,6 +309,8 @@ public class TestSequenceNumberForV2Table extends TableTestBase {
V2Assert.assertEquals("Snapshot sequence number should be 1", 1, snap1.sequenceNumber());
V2Assert.assertEquals(
"Last sequence number should be 1", 1, readMetadata().lastSequenceNumber());
+ V2Assert.assertEquals(
+ "Should be 1 manifest list", 1, listManifestLists(table.location()).size());
table.newAppend().appendFile(FILE_B).commit();
Snapshot snap2 = table.currentSnapshot();
@@ -319,12 +321,18 @@ public class TestSequenceNumberForV2Table extends TableTestBase {
V2Assert.assertEquals("Snapshot sequence number should be 2", 2, snap2.sequenceNumber());
V2Assert.assertEquals(
"Last sequence number should be 2", 2, readMetadata().lastSequenceNumber());
+ V2Assert.assertEquals(
+ "Should be 2 manifest lists", 2, listManifestLists(table.location()).size());
Transaction txn = table.newTransaction();
txn.expireSnapshots().expireSnapshotId(commitId1).commit();
txn.commitTransaction();
V2Assert.assertEquals(
"Last sequence number should be 2", 2, readMetadata().lastSequenceNumber());
+ V2Assert.assertEquals(
+ "Should be 1 manifest list as 1 was deleted",
+ 1,
+ listManifestLists(table.location()).size());
}
@Test