You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/09/29 11:53:17 UTC
[GitHub] [iceberg] singhpk234 opened a new pull request, #5888: Core, Spark: Rollback compaction on conflicts
singhpk234 opened a new pull request, #5888:
URL: https://github.com/apache/iceberg/pull/5888
### About the change :
Revert compaction in case a conflict is detected with the non compaction writes.
* Tag / mark the snapshot which was created as part of the compaction process (can use snapshot summary and introduce a flag is_compaction), so that it can be used for identification.
While committing the pending updates of the transaction, check if the current snapshot conflicts with the transaction updates.
* If the current snapshot conflicts and it’s a snapshot created by the compaction process (check snapshot summary key exists). Then revert the current snapshot (rollback to its parent snapshot) and then try re-applying updates on top of it.
Introduce a new table property `rollback.compaction.on-conflicts.enabled`, which essentially helps in identifying if we want to rollback the compaction commit if conflicts are detected.
* Let’s say a transaction has U1, U2, U3 ... update to be applied, let say B is the base snapshot, now when we were going to actually commit the transaction it saw the current snapshot now is B`(a snapshot created due to compaction), so make current snapshot as B` and try re-apply updates i.e {U1, U2, U3}, when applying U3 on top of { B` -> U1 -> U2 } a metadata conflict was detected, then try updates of transactions by rolling back B` to it’s parent, Let's say rollback B` to its parent is called RollbackB` so now try applying {RollbackB`, U1, U2, U3} on top of B` and see if it still conflicts if yes fail, else commit and update table metadata.
Based on proposal : https://docs.google.com/document/d/1pSqxf5A59J062j9VFF5rcCpbW9vdTbBKTmjps80D-B0/edit#
Adding this here to get a feedback on the approach. Once agreed upon, can make engines (for ex: spark) use transaction api.
-----
### Testing done :
- Added ut's for validating is_compacted is present in compacted snapshot summary
- Added ut for e2e using transaction API and concurrent rewrite.
- TODO add more exhaustive UT's
cc @rdblue @danielcweeks @jackye1995 @rajarshisarkar @amogh-jahagirdar
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1120523639
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -615,4 +630,49 @@ private static void updateTotal(
}
}
}
+
+ private Long snapshotIdToRollbackToOnConflict(Long parentSnapshotId) {
+ Long newParentSnapshotId = parentSnapshotId;
+ if (shouldRollbackReplaceOnConflict()) {
+ // add a set snapshot op on top of base to roll back to parent snapshot
+ boolean isCommitSuccessfullyApplied = false;
+ // Update parentSnapshot to it's grandParent
+ // provided parentSnapshot is of type replace and never rollback beyond startingSnapshotId
+ while (newParentSnapshotId != null
+ && DataOperations.REPLACE.equals(base.snapshot(newParentSnapshotId).operation())
+ && !newParentSnapshotId.equals(startingSnapshotId())) {
+ // create a tempTableOperation to pass base with rollback to validate of update
+ TableOperations tempTableOps = ops.temp(base);
+ newParentSnapshotId = base.snapshot(newParentSnapshotId).parentId();
+ if (newParentSnapshotId == null) {
+ return null;
+ }
+ Snapshot parentSnapshot = base.snapshot(newParentSnapshotId);
+
+ SetSnapshotOperation setSnapshotOp = new SetSnapshotOperation(tempTableOps);
+ setSnapshotOp.rollbackTo(newParentSnapshotId).commit();
+ try {
+ validate(tempTableOps.current(), parentSnapshot);
+ isCommitSuccessfullyApplied = true;
+ } catch (ValidationException validationException) {
+ // swallow the exception for re-trying
+ }
+ }
+
+ if (!isCommitSuccessfullyApplied) {
+ return null;
+ }
+
+ return newParentSnapshotId;
+ } else {
+ return null;
+ }
+ }
+
+ private Boolean shouldRollbackReplaceOnConflict() {
Review Comment:
it already has a default, can be type `boolean`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1120535272
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -615,4 +630,49 @@ private static void updateTotal(
}
}
}
+
+ private Long snapshotIdToRollbackToOnConflict(Long parentSnapshotId) {
+ Long newParentSnapshotId = parentSnapshotId;
+ if (shouldRollbackReplaceOnConflict()) {
+ // add a set snapshot op on top of base to roll back to parent snapshot
+ boolean isCommitSuccessfullyApplied = false;
+ // Update parentSnapshot to it's grandParent
Review Comment:
you mean ancestor?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Rollback compaction on conflicts [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#issuecomment-2408257978
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1260377940
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -199,6 +201,8 @@ protected void validate(TableMetadata currentMetadata, Snapshot snapshot) {}
*/
protected abstract List<ManifestFile> apply(TableMetadata metadataToUpdate, Snapshot snapshot);
+ protected abstract Long startingSnapshotId();
Review Comment:
are you suggesting the `starting snapshot ID` will be the base on which we are applying the snapshot ?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r984319484
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -479,6 +480,49 @@ private void applyUpdates(TableOperations underlyingOps) {
// Cannot pass even with retry due to conflicting metadata changes. So, break the
// retry-loop.
throw new PendingUpdateFailedException(e);
+
+ } catch (ValidationException e) {
+ if (PropertyUtil.propertyAsBoolean(
+ base.properties(),
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED,
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED_DEFAULT)) {
Review Comment:
> We are not just rolling back, but we are also reapplying the changes after rolling back. Maybe something like REAPPLY_REPLACE_ON_CONFLICTS_ENABLED? Any other suggestions for the name?
makes sense, how about something like `RETRY_WITH_ROLLBACK_REPLACE_ON_CONFLICTS_ENABLED`, it gives an impression that we are retrying the applying the changes with roll-backing replace operations snapshot.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Rollback compaction on conflicts [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#issuecomment-2297732020
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1131772531
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -615,4 +630,49 @@ private static void updateTotal(
}
}
}
+
+ private Long snapshotIdToRollbackToOnConflict(Long parentSnapshotId) {
+ Long newParentSnapshotId = parentSnapshotId;
+ if (shouldRollbackReplaceOnConflict()) {
+ // add a set snapshot op on top of base to roll back to parent snapshot
+ boolean isCommitSuccessfullyApplied = false;
+ // Update parentSnapshot to it's grandParent
+ // provided parentSnapshot is of type replace and never rollback beyond startingSnapshotId
+ while (newParentSnapshotId != null
+ && DataOperations.REPLACE.equals(base.snapshot(newParentSnapshotId).operation())
+ && !newParentSnapshotId.equals(startingSnapshotId())) {
+ // create a tempTableOperation to pass base with rollback to validate of update
+ TableOperations tempTableOps = ops.temp(base);
+ newParentSnapshotId = base.snapshot(newParentSnapshotId).parentId();
+ if (newParentSnapshotId == null) {
+ return null;
+ }
+ Snapshot parentSnapshot = base.snapshot(newParentSnapshotId);
+
+ SetSnapshotOperation setSnapshotOp = new SetSnapshotOperation(tempTableOps);
+ setSnapshotOp.rollbackTo(newParentSnapshotId).commit();
+ try {
+ validate(tempTableOps.current(), parentSnapshot);
+ isCommitSuccessfullyApplied = true;
+ } catch (ValidationException validationException) {
+ // swallow the exception for re-trying
+ }
+ }
+
+ if (!isCommitSuccessfullyApplied) {
+ return null;
+ }
+
+ return newParentSnapshotId;
+ } else {
+ return null;
+ }
+ }
+
+ private Boolean shouldRollbackReplaceOnConflict() {
+ return PropertyUtil.propertyAsBoolean(
+ base.properties(),
+ TableProperties.COMMIT_ROLLBACK_REPLACE_ON_CONFLICT_ENABLED,
Review Comment:
Explored a bit more offline, I think maybe it's better to do this through an update in related action APIs, for example:
```
public interface SupportsRollbackReplace<T> {
T rollbackReplaceOnConflict();
}
public interface AppendFiles extends SnapshotUpdate<AppendFiles>, SupportsRollbackReplace<AppendFiles> {
...
}
```
So that in core we can do:
```
table.newAppend().rollbackReplaceOnConflict().addFiles(...).commit()
```
And then we can make this a session config or runtime option in engine.
What do you think?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core, Spark: Rollback compaction on conflicts
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r983805711
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -479,6 +480,52 @@ private void applyUpdates(TableOperations underlyingOps) {
// Cannot pass even with retry due to conflicting metadata changes. So, break the
// retry-loop.
throw new PendingUpdateFailedException(e);
+
+ } catch (ValidationException e) {
+ if (PropertyUtil.propertyAsBoolean(
+ base.properties(),
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED,
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED_DEFAULT)) {
+
+ // use refreshed metadata
+ this.base = underlyingOps.current();
+ this.current = underlyingOps.current();
+ Long rollbackToSnapshotId = current.currentSnapshot().parentId();
+ long currentSnapshotId = current.currentSnapshot().snapshotId();
+ boolean updatesAppliedSuccessfully = false;
+
+ while (rollbackToSnapshotId != null
+ && !updatesAppliedSuccessfully
+ && PropertyUtil.propertyAsBoolean(
Review Comment:
I discussed with you a bit offline as well, I think instead of introducing a new property of `is-compacted`, it seems like we can rollback whenever we have conflict and `current.snapshot(currentSnapshotId).equals(DataOperations.REPLACE)`? Which applies to both compaction and rewrite manifests and feels more generic. Is there any risk of doing that way?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a diff in pull request #5888: Core, Spark: Rollback compaction on conflicts
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r983872476
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -479,6 +480,52 @@ private void applyUpdates(TableOperations underlyingOps) {
// Cannot pass even with retry due to conflicting metadata changes. So, break the
// retry-loop.
throw new PendingUpdateFailedException(e);
+
+ } catch (ValidationException e) {
+ if (PropertyUtil.propertyAsBoolean(
+ base.properties(),
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED,
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED_DEFAULT)) {
+
+ // use refreshed metadata
+ this.base = underlyingOps.current();
+ this.current = underlyingOps.current();
+ Long rollbackToSnapshotId = current.currentSnapshot().parentId();
+ long currentSnapshotId = current.currentSnapshot().snapshotId();
+ boolean updatesAppliedSuccessfully = false;
+
+ while (rollbackToSnapshotId != null
+ && !updatesAppliedSuccessfully
+ && PropertyUtil.propertyAsBoolean(
Review Comment:
> I discussed with you a bit offline as well, I think instead of introducing a new property of is-compacted, it seems like we can rollback whenever we have conflict and current.snapshot(currentSnapshotId).equals(DataOperations.REPLACE)
Apologies, I missed that. I think it makes sense to use snapshot#operation to identify if it's a compaction. Updated the PR & description.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1116602078
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -284,6 +284,10 @@ private TableProperties() {}
public static final String WRITE_AUDIT_PUBLISH_ENABLED = "write.wap.enabled";
public static final String WRITE_AUDIT_PUBLISH_ENABLED_DEFAULT = "false";
+ public static final String ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED =
Review Comment:
I think the name feels a bit awkward to me, first it's a write side feature, so I think it's better to make it start with `write.`. Secondly this is to rollback a REPLACE operation, so feels to me it's better to call it something like `write.rollback-replace-enabled`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1120532647
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -215,7 +219,18 @@ public Snapshot apply() {
long sequenceNumber = base.nextSequenceNumber();
Long parentSnapshotId = parentSnapshot == null ? null : parentSnapshot.snapshotId();
- validate(base, parentSnapshot);
+ try {
+ validate(base, parentSnapshot);
+ } catch (ValidationException ve) {
+ parentSnapshotId = snapshotIdToRollbackToOnConflict(parentSnapshotId);
Review Comment:
I think the logic that makes more sense in my mind is:
```
try {
validate(base, parentSnapshot);
} catch (ValidationException ve) {
if (shouldRollbackReplaceOnConflict()) {
// do rollback logic
} else {
// rethrow exception
}
}
```
instead of having the logic to determine the flag as a part of `snapshotIdToRollbackToOnConflict` which is already determining the new parent snapshot.
What do you think?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1120907547
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -199,6 +201,8 @@ protected void validate(TableMetadata currentMetadata, Snapshot snapshot) {}
*/
protected abstract List<ManifestFile> apply(TableMetadata metadataToUpdate, Snapshot snapshot);
+ protected abstract Long startingSnapshotId();
Review Comment:
I think this has to be non null? For the cases which it is currently null, it's just because the value is missing. But we always know what is the starting snapshot ID of the commit.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r984039784
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -479,6 +480,49 @@ private void applyUpdates(TableOperations underlyingOps) {
// Cannot pass even with retry due to conflicting metadata changes. So, break the
// retry-loop.
throw new PendingUpdateFailedException(e);
+
+ } catch (ValidationException e) {
+ if (PropertyUtil.propertyAsBoolean(
+ base.properties(),
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED,
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED_DEFAULT)) {
Review Comment:
also the term rollback seems to be not accurate. We are not just rolling back, but we are also reapplying the changes after rolling back. Maybe something like `REAPPLY_REPLACE_ON_CONFLICTS_ENABLED`?
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -479,6 +480,49 @@ private void applyUpdates(TableOperations underlyingOps) {
// Cannot pass even with retry due to conflicting metadata changes. So, break the
// retry-loop.
throw new PendingUpdateFailedException(e);
+
+ } catch (ValidationException e) {
+ if (PropertyUtil.propertyAsBoolean(
+ base.properties(),
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED,
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED_DEFAULT)) {
Review Comment:
also the term rollback seems to be not accurate. We are not just rolling back, but we are also reapplying the changes after rolling back. Maybe something like `REAPPLY_REPLACE_ON_CONFLICTS_ENABLED`? Any other suggestions for the name?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on pull request #5888: Core: Rollback compaction on conflicts
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#issuecomment-1310757061
Apologies for the delay in updating the pr, i was in the middle of changing my work location.
Have addressed the feedback and moved the changes to SnapshotProducer from BaseTransaction.
@rdblue @jackye1995 please have a look when you get some time.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Rollback compaction on conflicts [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #5888: Core: Rollback compaction on conflicts
URL: https://github.com/apache/iceberg/pull/5888
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1120522120
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -215,7 +219,18 @@ public Snapshot apply() {
long sequenceNumber = base.nextSequenceNumber();
Long parentSnapshotId = parentSnapshot == null ? null : parentSnapshot.snapshotId();
- validate(base, parentSnapshot);
+ try {
+ validate(base, parentSnapshot);
+ } catch (ValidationException ve) {
+ parentSnapshotId = snapshotIdToRollbackToOnConflict(parentSnapshotId);
+ // if no snapshot is found, so that roll backing to it avoids conflicts,
Review Comment:
this sentence does not make much sense to me... can you rephrase?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1120901810
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -615,4 +633,47 @@ private static void updateTotal(
}
}
}
+
+ private Long snapshotIdToRollbackToOnConflict(Long parentSnapshotId) {
Review Comment:
can we add a javdoc for this to explain the algorithm instead of embedding all the comments inline? Also the method name feels like just getting the ID, but in fact you are already performing the rollback. So maybe an actionable method name like just `rollbackReplaceOnConflictToParent(Long originalParent)`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1260376892
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -615,4 +633,47 @@ private static void updateTotal(
}
}
}
+
+ private Long snapshotIdToRollbackToOnConflict(Long parentSnapshotId) {
Review Comment:
we are not actually doing a rollback here, but rather trying to see which parent snapshot id we can rollback to, once we figure that out we apply the current on this parent snapshot id we found and commit to the table so the actual rollback is happening in `apply()` hence named this method as it is, please let me know your thoughts, happy to make the changes accordingly
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1116602078
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -284,6 +284,10 @@ private TableProperties() {}
public static final String WRITE_AUDIT_PUBLISH_ENABLED = "write.wap.enabled";
public static final String WRITE_AUDIT_PUBLISH_ENABLED_DEFAULT = "false";
+ public static final String ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED =
Review Comment:
I think the name feels a bit awkward to me, first it's a commit time feature, so I think it's better to make it start with `commit.`. Secondly this is to rollback a REPLACE operation, so feels to me it's better to call it something like `commit.rollback-replace-on-conflict-enabled`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r984319484
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -479,6 +480,49 @@ private void applyUpdates(TableOperations underlyingOps) {
// Cannot pass even with retry due to conflicting metadata changes. So, break the
// retry-loop.
throw new PendingUpdateFailedException(e);
+
+ } catch (ValidationException e) {
+ if (PropertyUtil.propertyAsBoolean(
+ base.properties(),
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED,
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED_DEFAULT)) {
Review Comment:
> We are not just rolling back, but we are also reapplying the changes after rolling back. Maybe something like REAPPLY_REPLACE_ON_CONFLICTS_ENABLED? Any other suggestions for the name?
makes sense, how about something like `RETRY_WITH_ROLLBACK_REPLACE_ON_CONFLICTS_ENABLED`, it gives an impression that we are retrying applying the changes with roll-backing replace operations snapshot.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1116602078
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -284,6 +284,10 @@ private TableProperties() {}
public static final String WRITE_AUDIT_PUBLISH_ENABLED = "write.wap.enabled";
public static final String WRITE_AUDIT_PUBLISH_ENABLED_DEFAULT = "false";
+ public static final String ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED =
Review Comment:
I think the name feels a bit awkward to me, first it's a write side feature, so I think it's better to make it start with `write.`. Secondly this is to rollback a REPLACE operation, so feels to me it's better to call it something like `write.rollback-replace-on-conflict-enabled`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1120534618
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -615,4 +630,49 @@ private static void updateTotal(
}
}
}
+
+ private Long snapshotIdToRollbackToOnConflict(Long parentSnapshotId) {
+ Long newParentSnapshotId = parentSnapshotId;
+ if (shouldRollbackReplaceOnConflict()) {
+ // add a set snapshot op on top of base to roll back to parent snapshot
+ boolean isCommitSuccessfullyApplied = false;
+ // Update parentSnapshot to it's grandParent
+ // provided parentSnapshot is of type replace and never rollback beyond startingSnapshotId
+ while (newParentSnapshotId != null
+ && DataOperations.REPLACE.equals(base.snapshot(newParentSnapshotId).operation())
+ && !newParentSnapshotId.equals(startingSnapshotId())) {
+ // create a tempTableOperation to pass base with rollback to validate of update
+ TableOperations tempTableOps = ops.temp(base);
+ newParentSnapshotId = base.snapshot(newParentSnapshotId).parentId();
+ if (newParentSnapshotId == null) {
+ return null;
+ }
+ Snapshot parentSnapshot = base.snapshot(newParentSnapshotId);
+
+ SetSnapshotOperation setSnapshotOp = new SetSnapshotOperation(tempTableOps);
+ setSnapshotOp.rollbackTo(newParentSnapshotId).commit();
+ try {
+ validate(tempTableOps.current(), parentSnapshot);
+ isCommitSuccessfullyApplied = true;
+ } catch (ValidationException validationException) {
+ // swallow the exception for re-trying
+ }
+ }
+
+ if (!isCommitSuccessfullyApplied) {
+ return null;
+ }
+
+ return newParentSnapshotId;
+ } else {
+ return null;
+ }
+ }
+
+ private Boolean shouldRollbackReplaceOnConflict() {
+ return PropertyUtil.propertyAsBoolean(
+ base.properties(),
+ TableProperties.COMMIT_ROLLBACK_REPLACE_ON_CONFLICT_ENABLED,
Review Comment:
I am a bit not sure if this should be a table level property. I would imagine this to enabled only on certain sessions, but the majority of writers still do not rollback. Is there any other option to make this a runtime config instead?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1120542029
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -615,4 +630,49 @@ private static void updateTotal(
}
}
}
+
+ private Long snapshotIdToRollbackToOnConflict(Long parentSnapshotId) {
+ Long newParentSnapshotId = parentSnapshotId;
+ if (shouldRollbackReplaceOnConflict()) {
+ // add a set snapshot op on top of base to roll back to parent snapshot
+ boolean isCommitSuccessfullyApplied = false;
+ // Update parentSnapshot to it's grandParent
+ // provided parentSnapshot is of type replace and never rollback beyond startingSnapshotId
+ while (newParentSnapshotId != null
+ && DataOperations.REPLACE.equals(base.snapshot(newParentSnapshotId).operation())
+ && !newParentSnapshotId.equals(startingSnapshotId())) {
+ // create a tempTableOperation to pass base with rollback to validate of update
+ TableOperations tempTableOps = ops.temp(base);
+ newParentSnapshotId = base.snapshot(newParentSnapshotId).parentId();
+ if (newParentSnapshotId == null) {
+ return null;
+ }
+ Snapshot parentSnapshot = base.snapshot(newParentSnapshotId);
+
+ SetSnapshotOperation setSnapshotOp = new SetSnapshotOperation(tempTableOps);
+ setSnapshotOp.rollbackTo(newParentSnapshotId).commit();
+ try {
+ validate(tempTableOps.current(), parentSnapshot);
+ isCommitSuccessfullyApplied = true;
+ } catch (ValidationException validationException) {
+ // swallow the exception for re-trying
Review Comment:
can we add a log message here, something like "Rollback to %s and commit still failed, ...."
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1120543836
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -615,4 +630,49 @@ private static void updateTotal(
}
}
}
+
+ private Long snapshotIdToRollbackToOnConflict(Long parentSnapshotId) {
+ Long newParentSnapshotId = parentSnapshotId;
+ if (shouldRollbackReplaceOnConflict()) {
+ // add a set snapshot op on top of base to roll back to parent snapshot
+ boolean isCommitSuccessfullyApplied = false;
+ // Update parentSnapshot to it's grandParent
+ // provided parentSnapshot is of type replace and never rollback beyond startingSnapshotId
+ while (newParentSnapshotId != null
+ && DataOperations.REPLACE.equals(base.snapshot(newParentSnapshotId).operation())
+ && !newParentSnapshotId.equals(startingSnapshotId())) {
+ // create a tempTableOperation to pass base with rollback to validate of update
+ TableOperations tempTableOps = ops.temp(base);
+ newParentSnapshotId = base.snapshot(newParentSnapshotId).parentId();
+ if (newParentSnapshotId == null) {
+ return null;
+ }
+ Snapshot parentSnapshot = base.snapshot(newParentSnapshotId);
+
+ SetSnapshotOperation setSnapshotOp = new SetSnapshotOperation(tempTableOps);
+ setSnapshotOp.rollbackTo(newParentSnapshotId).commit();
Review Comment:
this is not within a transaction, wouldn't this actually commit the table?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r984039784
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -479,6 +480,49 @@ private void applyUpdates(TableOperations underlyingOps) {
// Cannot pass even with retry due to conflicting metadata changes. So, break the
// retry-loop.
throw new PendingUpdateFailedException(e);
+
+ } catch (ValidationException e) {
+ if (PropertyUtil.propertyAsBoolean(
+ base.properties(),
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED,
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED_DEFAULT)) {
Review Comment:
also the term rollback seems to be not accurate. We are not just rolling back, but we are also reapplying the changes after rolling back. Maybe something like `REAPPLY_REPLACE_SNAPSHOT_ON_CONFLICTS_ENABLED`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1131789195
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -615,4 +630,49 @@ private static void updateTotal(
}
}
}
+
+ private Long snapshotIdToRollbackToOnConflict(Long parentSnapshotId) {
+ Long newParentSnapshotId = parentSnapshotId;
+ if (shouldRollbackReplaceOnConflict()) {
+ // add a set snapshot op on top of base to roll back to parent snapshot
+ boolean isCommitSuccessfullyApplied = false;
+ // Update parentSnapshot to it's grandParent
+ // provided parentSnapshot is of type replace and never rollback beyond startingSnapshotId
+ while (newParentSnapshotId != null
+ && DataOperations.REPLACE.equals(base.snapshot(newParentSnapshotId).operation())
+ && !newParentSnapshotId.equals(startingSnapshotId())) {
+ // create a tempTableOperation to pass base with rollback to validate of update
+ TableOperations tempTableOps = ops.temp(base);
+ newParentSnapshotId = base.snapshot(newParentSnapshotId).parentId();
+ if (newParentSnapshotId == null) {
+ return null;
+ }
+ Snapshot parentSnapshot = base.snapshot(newParentSnapshotId);
+
+ SetSnapshotOperation setSnapshotOp = new SetSnapshotOperation(tempTableOps);
+ setSnapshotOp.rollbackTo(newParentSnapshotId).commit();
+ try {
+ validate(tempTableOps.current(), parentSnapshot);
+ isCommitSuccessfullyApplied = true;
+ } catch (ValidationException validationException) {
+ // swallow the exception for re-trying
+ }
+ }
+
+ if (!isCommitSuccessfullyApplied) {
+ return null;
+ }
+
+ return newParentSnapshotId;
+ } else {
+ return null;
+ }
+ }
+
+ private Boolean shouldRollbackReplaceOnConflict() {
+ return PropertyUtil.propertyAsBoolean(
+ base.properties(),
+ TableProperties.COMMIT_ROLLBACK_REPLACE_ON_CONFLICT_ENABLED,
Review Comment:
we could potentially make this more generic, for example just add this to `SnapshotUpdate<T>`
```
T autoRollback(List<String> operations);
```
Although I don't know what would be the use case of the other kinds of rollback, but seems like a good generalization.
@rdblue do you have any thoughts and further comments regarding this PR?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1120865320
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -615,4 +630,49 @@ private static void updateTotal(
}
}
}
+
+ private Long snapshotIdToRollbackToOnConflict(Long parentSnapshotId) {
+ Long newParentSnapshotId = parentSnapshotId;
+ if (shouldRollbackReplaceOnConflict()) {
+ // add a set snapshot op on top of base to roll back to parent snapshot
+ boolean isCommitSuccessfullyApplied = false;
+ // Update parentSnapshot to it's grandParent
Review Comment:
yup
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -215,7 +219,18 @@ public Snapshot apply() {
long sequenceNumber = base.nextSequenceNumber();
Long parentSnapshotId = parentSnapshot == null ? null : parentSnapshot.snapshotId();
- validate(base, parentSnapshot);
+ try {
+ validate(base, parentSnapshot);
+ } catch (ValidationException ve) {
+ parentSnapshotId = snapshotIdToRollbackToOnConflict(parentSnapshotId);
Review Comment:
sounds good, made the changes
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -615,4 +630,49 @@ private static void updateTotal(
}
}
}
+
+ private Long snapshotIdToRollbackToOnConflict(Long parentSnapshotId) {
+ Long newParentSnapshotId = parentSnapshotId;
+ if (shouldRollbackReplaceOnConflict()) {
+ // add a set snapshot op on top of base to roll back to parent snapshot
+ boolean isCommitSuccessfullyApplied = false;
+ // Update parentSnapshot to it's grandParent
+ // provided parentSnapshot is of type replace and never rollback beyond startingSnapshotId
+ while (newParentSnapshotId != null
+ && DataOperations.REPLACE.equals(base.snapshot(newParentSnapshotId).operation())
+ && !newParentSnapshotId.equals(startingSnapshotId())) {
+ // create a tempTableOperation to pass base with rollback to validate of update
+ TableOperations tempTableOps = ops.temp(base);
+ newParentSnapshotId = base.snapshot(newParentSnapshotId).parentId();
+ if (newParentSnapshotId == null) {
+ return null;
+ }
+ Snapshot parentSnapshot = base.snapshot(newParentSnapshotId);
+
+ SetSnapshotOperation setSnapshotOp = new SetSnapshotOperation(tempTableOps);
+ setSnapshotOp.rollbackTo(newParentSnapshotId).commit();
Review Comment:
As per my understanding, since this is based on `TableOperations tempTableOps = ops.temp(base);` so it should not commit, we use this for validation to create a new temp metadata which will act as an uncommitted table metadata similar to what's done in transaction.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1120526081
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -73,6 +73,7 @@ enum TransactionType {
private TableMetadata current;
private boolean hasLastOpCommitted;
private final MetricsReporter reporter;
+ private boolean shouldRollbackCompaction;
Review Comment:
this name should be updated based on the config name
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -73,6 +73,7 @@ enum TransactionType {
private TableMetadata current;
private boolean hasLastOpCommitted;
private final MetricsReporter reporter;
+ private boolean shouldRollbackCompaction;
Review Comment:
and seems like it's not used?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1116600119
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -615,4 +630,45 @@ private static void updateTotal(
}
}
}
+
+ private Long snapshotIdToRollbackToOnConflict(Long parentSnapshotId) {
+ Long newParentSnapshotId = parentSnapshotId;
+ if (PropertyUtil.propertyAsBoolean(
Review Comment:
can we move this property check outside this method? So we don't have such a long if block.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r984038524
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -479,6 +480,49 @@ private void applyUpdates(TableOperations underlyingOps) {
// Cannot pass even with retry due to conflicting metadata changes. So, break the
// retry-loop.
throw new PendingUpdateFailedException(e);
+
+ } catch (ValidationException e) {
+ if (PropertyUtil.propertyAsBoolean(
+ base.properties(),
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED,
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED_DEFAULT)) {
Review Comment:
thanks for the update! In that case should we call this something like `ROLLBACK_REPLACE_ON_CONFLICTS_ENABLED`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r984038524
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -479,6 +480,49 @@ private void applyUpdates(TableOperations underlyingOps) {
// Cannot pass even with retry due to conflicting metadata changes. So, break the
// retry-loop.
throw new PendingUpdateFailedException(e);
+
+ } catch (ValidationException e) {
+ if (PropertyUtil.propertyAsBoolean(
+ base.properties(),
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED,
+ TableProperties.ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED_DEFAULT)) {
Review Comment:
thanks for the update! In that case should we call this something like `ROLLBACK_REWRITE_ON_CONFLICTS_ENABLED`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1120901810
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -615,4 +633,47 @@ private static void updateTotal(
}
}
}
+
+ private Long snapshotIdToRollbackToOnConflict(Long parentSnapshotId) {
Review Comment:
can we add a javdoc for this to explain the algorithm instead of embedding all the comments inline? Also the method name feels like just getting the ID, but in fact you are already performing the rollback. So maybe an actionable method name like just `rollbackReplaceOnConflict(Long originalParent)`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1116602078
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -284,6 +284,10 @@ private TableProperties() {}
public static final String WRITE_AUDIT_PUBLISH_ENABLED = "write.wap.enabled";
public static final String WRITE_AUDIT_PUBLISH_ENABLED_DEFAULT = "false";
+ public static final String ROLLBACK_COMPACTION_ON_CONFLICTS_ENABLED =
Review Comment:
I think the name feels a bit awkward to me, first it's a write side feature, so I think it's better to make it start with `write.`. Secondly this is to rollback a REPLACE operation, so feels to me it's better to call it something like `write.rollback-rewrite-enabled`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1131789195
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -615,4 +630,49 @@ private static void updateTotal(
}
}
}
+
+ private Long snapshotIdToRollbackToOnConflict(Long parentSnapshotId) {
+ Long newParentSnapshotId = parentSnapshotId;
+ if (shouldRollbackReplaceOnConflict()) {
+ // add a set snapshot op on top of base to roll back to parent snapshot
+ boolean isCommitSuccessfullyApplied = false;
+ // Update parentSnapshot to it's grandParent
+ // provided parentSnapshot is of type replace and never rollback beyond startingSnapshotId
+ while (newParentSnapshotId != null
+ && DataOperations.REPLACE.equals(base.snapshot(newParentSnapshotId).operation())
+ && !newParentSnapshotId.equals(startingSnapshotId())) {
+ // create a tempTableOperation to pass base with rollback to validate of update
+ TableOperations tempTableOps = ops.temp(base);
+ newParentSnapshotId = base.snapshot(newParentSnapshotId).parentId();
+ if (newParentSnapshotId == null) {
+ return null;
+ }
+ Snapshot parentSnapshot = base.snapshot(newParentSnapshotId);
+
+ SetSnapshotOperation setSnapshotOp = new SetSnapshotOperation(tempTableOps);
+ setSnapshotOp.rollbackTo(newParentSnapshotId).commit();
+ try {
+ validate(tempTableOps.current(), parentSnapshot);
+ isCommitSuccessfullyApplied = true;
+ } catch (ValidationException validationException) {
+ // swallow the exception for re-trying
+ }
+ }
+
+ if (!isCommitSuccessfullyApplied) {
+ return null;
+ }
+
+ return newParentSnapshotId;
+ } else {
+ return null;
+ }
+ }
+
+ private Boolean shouldRollbackReplaceOnConflict() {
+ return PropertyUtil.propertyAsBoolean(
+ base.properties(),
+ TableProperties.COMMIT_ROLLBACK_REPLACE_ON_CONFLICT_ENABLED,
Review Comment:
we could potentially make this more generic, for example just add this to `SnapshotUpdate<T>`
```
T rollback(List<String> operations);
```
Although I don't know what would be the use case of the other kinds of rollback, but seems like a good generalization.
@rdblue do you have any thoughts and further comments regarding this PR?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org