Skip to content

Core: Add snapshot table property #1795

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
aokolnychyi wants to merge 1 commit into from
Closed

Core: Add snapshot table property #1795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions core/src/main/java/org/apache/iceberg/RemoveSnapshots.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.apache.iceberg.exceptions.CommitFailedException;
import org.apache.iceberg.exceptions.NotFoundException;
import org.apache.iceberg.exceptions.RuntimeIOException;
import org.apache.iceberg.exceptions.ValidationException;
import org.apache.iceberg.io.CloseableIterable;
import org.apache.iceberg.relocated.com.google.common.base.Joiner;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
Expand All @@ -51,6 +52,8 @@
import static org.apache.iceberg.TableProperties.COMMIT_NUM_RETRIES_DEFAULT;
import static org.apache.iceberg.TableProperties.COMMIT_TOTAL_RETRY_TIME_MS;
import static org.apache.iceberg.TableProperties.COMMIT_TOTAL_RETRY_TIME_MS_DEFAULT;
import static org.apache.iceberg.TableProperties.SNAPSHOT;
import static org.apache.iceberg.TableProperties.SNAPSHOT_DEFAULT;

@SuppressWarnings("UnnecessaryAnonymousClass")
class RemoveSnapshots implements ExpireSnapshots {
Expand Down Expand Up @@ -78,6 +81,10 @@ public void accept(String file) {
RemoveSnapshots(TableOperations ops) {
this.ops = ops;
this.base = ops.current();

ValidationException.check(
!PropertyUtil.propertyAsBoolean(base.properties(), SNAPSHOT, SNAPSHOT_DEFAULT),
"Not allowed to expire snapshots in a snapshot table as this may remove files in the original table");
Copy link
Member

@RussellSpitzer RussellSpitzer Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really need better naming. This is extremely confusing :)

Copy link
Member

@RussellSpitzer RussellSpitzer Nov 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The target table was created using the snapshot action against another table which means it is not the exclusive owner of its data files. This means that we cannot determine if data files are actually no longer needed after running an expireSnapshots command and could unintentionally remove files needed by the original table. Unset the SNAPSHOT table property if you are sure this what you intend to do." -- Maybe?

Copy link
Contributor Author

@aokolnychyi aokolnychyi Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, let me update this.

Copy link

@jacques-n jacques-n Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about thinking about this as "un-gc'able" instead of specifically a snapshot table to be more generic?

Copy link
Contributor Author

@aokolnychyi aokolnychyi Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is a good idea, @jacques-n. We will need a good name, though. Is gc.enabled descriptive enough? Any other alternatives?

Also, we should maybe respect this property while removing orphan files.

Copy link
Contributor

@rdblue rdblue Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the more specific gc enabled flag. That seems like a better way to do this.

}

@Override
Expand Down
3 changes: 3 additions & 0 deletions core/src/main/java/org/apache/iceberg/TableProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,4 +134,7 @@ private TableProperties() {

public static final String ENGINE_HIVE_ENABLED = "engine.hive.enabled";
public static final boolean ENGINE_HIVE_ENABLED_DEFAULT = false;

public static final String SNAPSHOT = "snapshot";
public static final boolean SNAPSHOT_DEFAULT = false;
}
16 changes: 16 additions & 0 deletions core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.util.concurrent.Executors;
import java.util.concurrent.atomic.AtomicInteger;
import org.apache.iceberg.ManifestEntry.Status;
import org.apache.iceberg.exceptions.ValidationException;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
Expand Down Expand Up @@ -1044,4 +1045,19 @@ public void testWithExpiringStagedThenCherrypick() {
});
});
}

@Test
public void testExpireSnapshotsInSnapshotTable() {
table.updateProperties()
.set(TableProperties.SNAPSHOT, "true")
.commit();

table.newAppend()
.appendFile(FILE_A)
.commit();

AssertHelpers.assertThrows("Should complain about expiring snapshots",
ValidationException.class, "Not allowed to expire snapshots",
() -> table.expireSnapshots());
}
}
20 changes: 20 additions & 0 deletions spark/src/test/java/org/apache/iceberg/actions/TestExpireSnapshotsAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.apache.iceberg.Table;
import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.TableProperties;
import org.apache.iceberg.exceptions.ValidationException;
import org.apache.iceberg.hadoop.HadoopTables;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
Expand Down Expand Up @@ -382,6 +383,25 @@ public void testRetainLastKeepsExpiringSnapshot() {
checkExpirationResults(0L, 0L, 1L, result);
}

@Test
public void testExpireSnapshotsInSnapshotTable() {
table.updateProperties()
.set(TableProperties.SNAPSHOT, "true")
.commit();

table.newAppend()
.appendFile(FILE_A)
.commit();

ExpireSnapshotsAction action = Actions.forTable(table).expireSnapshots()
.expireOlderThan(System.currentTimeMillis())
.retainLast(2);

AssertHelpers.assertThrows("Should complain about expiring snapshots",
ValidationException.class, "Not allowed to expire snapshots",
action::execute);
}

@Test
public void testExpireOlderThanMultipleCalls() {
table.newAppend()
Expand Down