Core: Replace setCurrentSnapshot with setBranchSnapshot in metadata builder by rdblue · Pull Request #4089 · apache/iceberg

rdblue · 2022-02-11T01:03:09Z

This updates the TableMetadata.Builder methods and replaces the previous setCurrentSnapshot with a branch-aware setBranchSnapshot.

I'm posting this for early feedback. This is the most straight-forward change, but it doesn't support modifying the ref settings like max age and it doesn't support creating/removing tags. We may want to change the builder API to be more generic and if so this is a step toward that API.

rdblue · 2022-02-11T01:03:48Z

@amogh-jahagirdar and @jackye1995, this is a first attempt at updating the table metadata builder. It doesn't support everything yet but I want to get this up for early feedback so we can compare the most direct changes with what you were proposing on the other PRs.

rdblue · 2022-02-11T01:04:55Z

core/src/main/java/org/apache/iceberg/MetadataUpdate.java


-  class SetCurrentSnapshot implements MetadataUpdate {
-    private final Long snapshotId;
+  class RemoveSnapshotRef implements MetadataUpdate {


RemoveSnapshotRef is used for dropping a branch instead of SetCurrentSnapshot(null). Now the snapshot can be required instead of nullable.

rdblue · 2022-02-11T01:05:51Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

-      if (!snapshotsById.containsKey(currentSnapshotId)) {
-        setCurrentSnapshot(null, System.currentTimeMillis());
+
+      // remove any refs that are no longer valid


Should this fail if there are still refs?

Based on the current usage, we are only calling this when expiring snapshots. At that time the snapshots send in here should already be the ones that have to be removed after evaluating all retention policies. I think that's why we directly remove the refs instead of failing.

I think it is in the scope for removeSnapshots(snapshots) to remove the references itself for any snapshots we are removing. Otherwise we would have to have the caller do a separate removeReferences and then do a removeSnapshots which seems more cumbersome.

rdblue · 2022-02-11T01:06:57Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

+
+      if (SnapshotRef.MAIN_BRANCH.equals(branch)) {
+        this.currentSnapshotId = snapshot.snapshotId();
+        snapshotLog.add(new SnapshotLogEntry(lastUpdatedMillis, snapshot.snapshotId()));


The snapshot log is only for main right now. We may want to update and add logs for branches? Probably not for tags because tags are labels that we probably don't track history for (they should be static).

core/src/main/java/org/apache/iceberg/TableMetadata.java

jackye1995 · 2022-02-14T08:32:30Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

+        snapshotLog.add(new SnapshotLogEntry(lastUpdatedMillis, snapshot.snapshotId()));
+      }
+
+      SnapshotRef.Builder refBuilder = SnapshotRef.branchBuilder(snapshot.snapshotId());


I think you can just call SnapshotRef.builderFrom here

The problem is that ref may be null. If it is null, then we create a new one with defaults. If it is non-null, then we copy it. I'll make this a bit more clear.

rdblue · 2022-02-14T23:45:26Z

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

+            TableMetadata.Builder update = TableMetadata.buildFrom(base);
+            if (base.snapshot(newSnapshot.snapshotId()) != null) {
+              // this is a rollback operation
+              update.setBranchSnapshot(newSnapshot.snapshotId(), SnapshotRef.MAIN_BRANCH);


@wypoon and @sririshindra, I think this fixes the problem from #4088. The previous call to replaceCurrentSnapshot always used setCurrentSnapshot(Snapshot, String) rather than setCurrentSnapshot(long, String). This detects when the snapshot already exists and calls the correct method.

Yes, that makes sense.
I cherry-picked some prerequisite commits, and then applied this change to our 0.13.0 branch, and reran @sririshindra's rollback test, and it passed.

rdblue · 2022-02-14T23:47:05Z

spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/source/TestIcebergSourceTablesBase.java

    TestHelpers.assertEqualsSafe(historyTable.schema().asStruct(), expected.get(0), actual.get(0));
    TestHelpers.assertEqualsSafe(historyTable.schema().asStruct(), expected.get(1), actual.get(1));
    TestHelpers.assertEqualsSafe(historyTable.schema().asStruct(), expected.get(2), actual.get(2));
+    TestHelpers.assertEqualsSafe(historyTable.schema().asStruct(), expected.get(3), actual.get(3));


This test was failing, I think due to the problem in #4088. While looking at it, I realized that the last record was not being checked as well.

I was wrong about the cause of the history test failure. It turns out that I called the wrong method from setBranchSnapshot(Snapshot, String).

jackye1995

looks good to me, just 1 nit comment, up to you if you think it's worth adding a util method for that.

jackye1995 · 2022-02-15T19:41:31Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

-      snapshotLog.add(new SnapshotLogEntry(lastUpdatedMillis, snapshot.snapshotId()));
-      changes.add(new MetadataUpdate.SetCurrentSnapshot(snapshot.snapshotId()));
+
+      if (SnapshotRef.MAIN_BRANCH.equals(branch)) {


looks like the check for main branch is done in many places, should we also have a method for ref.isMainBranch()?

I think the problem is that there aren't many places where we actually have a SnapshotRef instance. Here, for example, we don't necessarily have one because ref might be null. So we actually want to use the string branch name.

Below, we have a SetSnapshotRef instance, which is a change. And in places like removeBranch, we do the check before loading the ref. If there is no main ref to remove, we should still ensure that we set current-snapshot-id appropriately.

rdblue · 2022-02-15T22:21:29Z

Thanks for reviewing, @jackye1995 and @amogh-jahagirdar!

…uilder (apache#4089) (cherry picked from commit a3a112d)

Replace setCurrentSnapshot with setBranchSnapshot in metadata builder.

6dba027

github-actions bot added API core NESSIE labels Feb 11, 2022

rdblue commented Feb 11, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/TableMetadata.java Outdated Show resolved Hide resolved

Fix intermediate snapshot IDs.

90dc50d

rdblue changed the title ~~Replace setCurrentSnapshot with setBranchSnapshot in metadata builder~~ Core: Replace setCurrentSnapshot with setBranchSnapshot in metadata builder Feb 11, 2022

wypoon mentioned this pull request Feb 11, 2022

Core: Set current snapshot with currentTimeMillis #4088

Closed

jackye1995 reviewed Feb 14, 2022

View reviewed changes

rdblue added 2 commits February 14, 2022 15:22

Clean up setBranchSnapshot.

eaeca5b

Fix test failures.

edd5d68

github-actions bot added the spark label Feb 14, 2022

rdblue commented Feb 14, 2022

View reviewed changes

rdblue added 2 commits February 14, 2022 15:58

Fix Nessie tests.

e997e7f

Fix history.

523f388

jackye1995 approved these changes Feb 15, 2022

View reviewed changes

rdblue merged commit a3a112d into apache:master Feb 15, 2022

sririshindra mentioned this pull request Feb 16, 2022

Core: Call the replaceCurrentSnapshot with snapshotId for rollback operations #4135

Merged

sririshindra pushed a commit to sririshindra/iceberg that referenced this pull request Feb 16, 2022

Core: Replace setCurrentSnapshot with setBranchSnapshot in metadata b…

d8b2c48

…uilder (apache#4089) (cherry picked from commit a3a112d)

sririshindra pushed a commit to sririshindra/iceberg that referenced this pull request Feb 16, 2022

Core: Replace setCurrentSnapshot with setBranchSnapshot in metadata b…

be37738

…uilder (apache#4089) (cherry picked from commit a3a112d)

wypoon mentioned this pull request Feb 17, 2022

Core: Remove unused method accidentally added to TableMetadata #4155

Merged

wypoon mentioned this pull request Mar 4, 2022

Improve snapshot rollback failure when snapshots produced too frequent. #4144

Closed

Conversation

rdblue commented Feb 11, 2022

Uh oh!

rdblue commented Feb 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackye1995 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue commented Feb 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amogh-jahagirdar Feb 14, 2022 •

edited

Loading