leaves12138 commented on code in PR #8450:
URL: https://github.com/apache/paimon/pull/8450#discussion_r3520153198
##########
paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/procedure/RollbackToAsLatestProcedure.java:
##########
@@ -80,17 +85,19 @@ Row[] call(ProcedureContext procedureContext, String
tableId, String tagName, Lo
Preconditions.checkArgument(
hasTag != hasSnapshot, "Must specify exactly one of tag and
snapshot_id.");
+ TagManager tagManager = store.newTagManager();
Snapshot targetSnapshot;
if (hasTag) {
- targetSnapshot =
store.newTagManager().getOrThrow(tagName).trimToSnapshot();
+ targetSnapshot = tagManager.getOrThrow(tagName).trimToSnapshot();
} else {
- targetSnapshot = findSnapshot(store, snapshotId);
+ targetSnapshot = findSnapshot(store, tagManager, snapshotId);
}
+ Tag targetTag = ensureNonExpiringRollbackTag(tagManager,
targetSnapshot);
Review Comment:
This creates the non-expiring protection tag before the rollback commit has
passed pre-commit callbacks and `commitSnapshotImpl`. If the rollback is
rejected (for example the chain-table pre-callback case covered by
`testRollbackToAsLatestRejectedWhenDroppingAnchorSnapshotPartition`) or the
snapshot commit returns false because of a concurrent commit, the rollback
snapshot is not created but the new `rollback-to-as-latest-*` tag remains
forever and pins the target snapshot/files. Could we track whether this
procedure created a new protection tag and remove it when the rollback did not
actually create the new latest snapshot (being careful to keep it if the
snapshot was committed but a later callback threw), or otherwise create the tag
only after the rollback is known to have succeeded?
##########
paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/procedure/RollbackToAsLatestProcedure.java:
##########
@@ -108,13 +115,30 @@ Row[] call(ProcedureContext procedureContext, String
tableId, String tagName, Lo
};
}
- private Snapshot findSnapshot(FileStore<?> store, long snapshotId) {
+ private Tag ensureNonExpiringRollbackTag(TagManager tagManager, Snapshot
targetSnapshot) {
+ for (Pair<Tag, String> tag : tagManager.tagObjects()) {
+ Tag tagObject = tag.getLeft();
+ if (tagObject.id() == targetSnapshot.id() &&
tagObject.getTagTimeRetained() == null) {
Review Comment:
Reusing any non-expiring tag still makes the rolled-back latest snapshot
depend on an unrelated user tag. If that user tag is deleted later after the
target snapshot has expired, `TagManager.deleteTag` can clean files from the
target snapshot even though they are still referenced by the rollback snapshot.
To make the dependency explicit and avoid accidental deletion, I think this
should only reuse an existing `rollback-to-as-latest-*` protection tag;
otherwise it should create a dedicated rollback protection tag even when
another non-expiring tag already exists on the target 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
