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 2020/02/16 22:17:16 UTC
[GitHub] [incubator-iceberg] rdblue opened a new pull request #802: Update
RemoveSnapshots to protect cherry-picked data files
rdblue opened a new pull request #802: Update RemoveSnapshots to protect cherry-picked data files
URL: https://github.com/apache/incubator-iceberg/pull/802
This updates `RemoveSnapshots` to detect cherry-picked commits and skip cleanup.
This doesn't handle complex cases where snapshots are explicitly expired using `ExpireSnapshots.expireSnapshotId`, or where the current snapshot is directly modified. We will want a more thorough follow-up for those cases, tracked by #744.
This does handle cases where a change is picked into the current table state and the original commit expires. Cleanup for picked changes is done when the commit in the current snapshot's ancestors expires.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on issue #802: Update
RemoveSnapshots to protect cherry-picked data files
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #802: Update RemoveSnapshots to protect cherry-picked data files
URL: https://github.com/apache/incubator-iceberg/pull/802#issuecomment-588353399
@rominparekh, I agree that I need to add tests for this before we get it in. I just wanted to get this to you guys early, since it at least protects data in most cases and I know you're eager to use the cherry-pick feature.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#802: Update RemoveSnapshots to protect cherry-picked data files
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #802: Update RemoveSnapshots to protect cherry-picked data files
URL: https://github.com/apache/incubator-iceberg/pull/802#discussion_r381444502
##########
File path: core/src/main/java/org/apache/iceberg/RemoveSnapshots.java
##########
@@ -188,16 +189,33 @@ private void cleanExpiredFiles(List<Snapshot> snapshots, Set<Long> validIds, Set
// physically deleting files that were logically deleted in a commit that was rolled back.
Set<Long> ancestorIds = Sets.newHashSet(SnapshotUtil.ancestorIds(base.currentSnapshot(), base::snapshot));
Review comment:
The current set of changes makes some assumptions that generally hold:
* Cherry-picked snapshots are newer than the original snapshot -- C comes after B in time
* Expiration happens in order -- B is expired before C or in the same commit
I'm not yet sure how to handle more complex cases, like expiring a specific snapshot. I think maybe we should just add a warning or deprecate that method. I don't know of any current uses.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue edited a comment on issue #802: Update
RemoveSnapshots to protect cherry-picked data files
Posted by GitBox <gi...@apache.org>.
rdblue edited a comment on issue #802: Update RemoveSnapshots to protect cherry-picked data files
URL: https://github.com/apache/incubator-iceberg/pull/802#issuecomment-588353399
@rominparekh, I agree that I need to add tests for this before we get it in. I just wanted to get this to you guys as early as I could, since it at least protects data in most cases and I know you're eager to use the cherry-pick feature.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] mehtaashish23 commented on issue #802: Update
RemoveSnapshots to protect cherry-picked data files
Posted by GitBox <gi...@apache.org>.
mehtaashish23 commented on issue #802: Update RemoveSnapshots to protect cherry-picked data files
URL: https://github.com/apache/incubator-iceberg/pull/802#issuecomment-588885590
@rdblue Here is the PR on top of your branch, to add UTs, and confirms the expected behavior of this code. https://github.com/rdblue/iceberg/pull/1/files
Considering that we are in conflict with latest master and also one of the newly written UT in above PR fails (as we haven't picked a fix from latest master), I have raised another PR https://github.com/apache/incubator-iceberg/pull/814 which is rebased + your changes + UTs + Marking `expireSnapshotId` as Deprecated.
Feel free to follow, whatever is simple.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on issue #802: Update
RemoveSnapshots to protect cherry-picked data files
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #802: Update RemoveSnapshots to protect cherry-picked data files
URL: https://github.com/apache/incubator-iceberg/pull/802#issuecomment-596824075
Closing this because #814 replaces it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] mehtaashish23 commented on a change in pull
request #802: Update RemoveSnapshots to protect cherry-picked data files
Posted by GitBox <gi...@apache.org>.
mehtaashish23 commented on a change in pull request #802: Update RemoveSnapshots to protect cherry-picked data files
URL: https://github.com/apache/incubator-iceberg/pull/802#discussion_r381564069
##########
File path: core/src/main/java/org/apache/iceberg/RemoveSnapshots.java
##########
@@ -188,16 +189,33 @@ private void cleanExpiredFiles(List<Snapshot> snapshots, Set<Long> validIds, Set
// physically deleting files that were logically deleted in a commit that was rolled back.
Set<Long> ancestorIds = Sets.newHashSet(SnapshotUtil.ancestorIds(base.currentSnapshot(), base::snapshot));
Review comment:
@rdblue +1, I am too in the opinion, that we should deprecate the older API of expiring a particular snapshotId, supporting it with cherry-pick (or more complex linking) is going to be tricky.
Apologies I didn't followed on this issue, I was only stuck, since I also didn't had any solution for supporting expiry of a particular snapshot. I can help with the UTs though, I attempted to add some with my PR https://github.com/rominparekh/incubator-iceberg/pull/3/files
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on issue #802: Update
RemoveSnapshots to protect cherry-picked data files
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #802: Update RemoveSnapshots to protect cherry-picked data files
URL: https://github.com/apache/incubator-iceberg/pull/802#issuecomment-586761049
@prodeezy, @mehtaashish23, can you take a look at this? This adds some basic protection for cherry-picked data files. This is what we are currently using when we clean up snapshots to avoid deleting data files that have been picked into the current table state.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] mehtaashish23 commented on a change in pull
request #802: Update RemoveSnapshots to protect cherry-picked data files
Posted by GitBox <gi...@apache.org>.
mehtaashish23 commented on a change in pull request #802: Update RemoveSnapshots to protect cherry-picked data files
URL: https://github.com/apache/incubator-iceberg/pull/802#discussion_r381139425
##########
File path: core/src/main/java/org/apache/iceberg/RemoveSnapshots.java
##########
@@ -188,16 +189,33 @@ private void cleanExpiredFiles(List<Snapshot> snapshots, Set<Long> validIds, Set
// physically deleting files that were logically deleted in a commit that was rolled back.
Set<Long> ancestorIds = Sets.newHashSet(SnapshotUtil.ancestorIds(base.currentSnapshot(), base::snapshot));
Review comment:
@rdblue I can see that this PR handles almost all scenarios in the current/first iteration (when you are expiring some or any snapshot), but I am trying to understand the run following the current iteration, what is the output of this (SnapshotUtil.ancestorIds) API (on subsequent runs), after we expire one of the ancestorIds in the current run?
Based on what I can understand, it will return the ancestors based of off current snapshot, till the time we can find the parent snapshot in snapshot list, and since we have expired a particular ancestor (in previous run) the list will be incomplete. Right? If yes, then I suppose, we can't support APIs like expireSnapshotId, for selectively expiring snapshots, as it breaks lineage.
I gave a thought, and found out, that we can't support id based expiring, instead we can only do time-based expiry. That will elevate the issue, where in (See below example) I can expire C (cherrypicked from B), and then later decide to expire B, which might delete underlying dataFiles of B referred by C, which is expired in previous run. Let me know, how this PR solves that?
//A -- C (B) -- D
// `- B
Correct me if I am wrong, I am still trying to understand nuances of snapshot list maintenance in Iceberg.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue closed pull request #802: Update
RemoveSnapshots to protect cherry-picked data files
Posted by GitBox <gi...@apache.org>.
rdblue closed pull request #802: Update RemoveSnapshots to protect cherry-picked data files
URL: https://github.com/apache/incubator-iceberg/pull/802
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org