szlta opened a new issue, #5507:
URL: https://github.com/apache/iceberg/issues/5507
### Apache Iceberg version
0.14.0 (latest release)
### Query engine
_No response_
### Please describe the bug 🐞
Example test case for `TestSnapshotManager.java`:
```
@Test
public void testAttemptToRollbackToCurrentSnapshot() {
table.newAppend().appendFile(FILE_A).commit();
long currentSnapshotTimestampPlus100 =
table.currentSnapshot().timestampMillis() + 100;
table
.manageSnapshots()
.rollbackToTime(currentSnapshotTimestampPlus100)
.commit();
long currentSnapshotId = table.currentSnapshot().snapshotId();
table
.manageSnapshots()
.rollbackTo(currentSnapshotId)
.commit();
}
```
The above case fails with
```
Cannot commit transaction: last operation has not committed
java.lang.IllegalStateException: Cannot commit transaction: last operation
has not committed
at
org.apache.iceberg.relocated.com.google.common.base.Preconditions.checkState(Preconditions.java:502)
at
org.apache.iceberg.BaseTransaction.commitTransaction(BaseTransaction.java:252)
at org.apache.iceberg.SnapshotManager.commit(SnapshotManager.java:158)
...
```
I think in such cases the API should still return without errors, even if
the request turned out to be a no-op.
This was working well in 0.13.0, but since 0.14.0 it is failing due to the
refactor in this commit:
https://github.com/apache/iceberg/commit/e41e8de673628baa6d641f5ad6b03680c18cc0fe
The reason for the failure is that `SetSnapshotOperation#commit()` doesn't
commit on `taskOps` at
https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/SetSnapshotOperation.java#L126-L137
leaving
https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L105
in false state and eventually causing the transaction commit to throw the
above exception.
IMHO there isn't a good reason for `SetSnapshotOperation#commit()` to return
without committing in such cases as the commitTransaction() invocation would do
a similar logic later anyway.
So I think the solution would be to remove this "return if no changes" logic
from `SetSnapshotOperation`. @rdblue - as the original contributor, what's your
opinion?
cc: @lcspinter @pvary
--
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]
