Core: Call the replaceCurrentSnapshot with snapshotId for rollback operations#4135
Conversation
| if (base.snapshot(newSnapshot.snapshotId()) != null) { | ||
| // this is a rollback operation | ||
| updated = base.replaceCurrentSnapshot(newSnapshot.snapshotId()); | ||
| } else if (stageOnly) { |
There was a problem hiding this comment.
Can you port the changes from the other fix? That removed methods from TableMetadata rather than adding new ones. I think it would be best to keep 0.13.x and master in sync rather than having slightly different fixes.
Thanks, @sririshindra!
There was a problem hiding this comment.
@rdblue, would the following be acceptable? (from line 285)
TableMetadata.Builder update = TableMetadata.buildFrom(base);
if (base.snapshot(newSnapshot.snapshotId()) != null) {
// this is a rollback or cherrypick operation
update.setCurrentSnapshot(newSnapshot.snapshotId());
} else if (stageOnly) {
update.addSnapshot(newSnapshot);
} else {
update.setCurrentSnapshot(newSnapshot);
}
TableMetadata updated = update.build();
if (updated.changes().isEmpty()) {
// do not commit if the metadata has not changed. for example, this may happen when setting the current
// snapshot to an ID that is already current. note that this check uses identity.
return;
}
I know the point of #4089 is to replace setCurrentSnapshot with setBranchSnapshot, but in 0.13.x, we don't support branching yet, and the commits #4089 builds on are not present. The above mirrors your change to SnapshotProducer in #4089.
There was a problem hiding this comment.
Yes, calling the right setCurrentSnapshot is a good way to backport this. Thanks!
(cherry picked from commit a3a112d)
|
Looks good! I'll merge when tests are passing. |
Thanks @rdblue |
| public TableMetadata replaceCurrentSnapshot(long snapshotId) { | ||
| return new Builder(this).setCurrentSnapshot(snapshotId).build(); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this was an oversight, right? We don't need this new method; it is not used.
In fact, with the above change to SnapshotProducer, even replaceCurrentSnapshot(Snapshot) is no longer used and can be removed, I think.
There was a problem hiding this comment.
Yes, that's right. I missed this.
(cherry picked from commit e2af91c)
(cherry picked from commit e2af91c) Co-authored-by: Rishi <sririshindra@gmail.com>
While testing rollback_to_snaphot feature, we found that this is feature is failing in some cases.
In the setSetCurrentSnapshot method 'null' is being passed for timestamp which eventually causes the failure of the following precondition check. The timeStamp should also be passed along with the Snapshot just like here
This PR fixes this bug in 0.13.x. I tested this with our internal tests and also by running the unit tests in Iceberg. This issue no longer exists in master because of 4089. The current fix is ported from here
Steps to reproduce the rollback_to_snapshot bug in spark.
Each insert operation below is done separately in its own spark app.
launch a new spark shell and follow the following steps