New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core: Fix for deleting files when commiting transactions with multiple branches #6634
Conversation
5c18843
to
4cf2ced
Compare
e383f32
to
870de44
Compare
870de44
to
3eb6f3c
Compare
@amogh-jahagirdar, this looks great! I think it's about ready to merge but there are a couple of naming nits. |
3eb6f3c
to
6186cf5
Compare
Thanks @rdblue for the review! since the focus of this PR was properly determining the set of committed files when committing a transaction I think we should be good. Regardless I think we probably want some more coverage on transactions spanning multiple branches, I can raise that in a separate PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, thanks for the fix!
Thanks, @amogh-jahagirdar! |
When a snapshot is expired as part of a transaction, the snapshot file(s) should be deleted when the transaction commits. A recent change (apache#6634) ensured that files are not deleted when they have also been committed as part of a transaction, but this breaks the simple case where no new files are committed. Fix this be not skipping deletion when the list of committed files is empty. TESTING: Extended a unit test to ensure that snapshot files are deleted. Ran the test without the fix on a branch where apache#6634 was reverted to show that this is a regression.
When a snapshot is expired as part of a transaction, the manifest list file(s) should be deleted when the transaction commits. A recent change (apache#6634) ensured that these files are not deleted when they have also been committed as part of a transaction, but this breaks the simple case where no new files are committed. Fix this by not skipping deletion when the list of committed files is empty. TESTING: Extended a unit test to ensure that manifest list files are deleted. Ran the test without the fix on a branch where apache#6634 was reverted to show that this is a regression.
When a snapshot is expired as part of a transaction, the manifest list file(s) should be deleted when the transaction commits. A recent change (apache#6634) ensured that these files are not deleted when they have also been committed as part of a transaction, but this breaks the simple case where no new files are committed. Fix this by not skipping deletion when the list of committed files is empty. TESTING: Extended a unit test to ensure that manifest list files are deleted. Ran the test without the fix on a branch where apache#6634 was reverted to show that this is a regression.
When a snapshot is expired as part of a transaction, the manifest list file(s) should be deleted when the transaction commits. A recent change (#6634) ensured that these files are not deleted when they have also been committed as part of a transaction, but this breaks the simple case where no new files are committed. Fix this by not skipping deletion when the list of committed files is empty. TESTING: Extended a unit test to ensure that manifest list files are deleted. Ran the test without the fix on a branch where #6634 was reverted to show that this is a regression.
…che#9183) When a snapshot is expired as part of a transaction, the manifest list file(s) should be deleted when the transaction commits. A recent change (apache#6634) ensured that these files are not deleted when they have also been committed as part of a transaction, but this breaks the simple case where no new files are committed. Fix this by not skipping deletion when the list of committed files is empty. TESTING: Extended a unit test to ensure that manifest list files are deleted. Ran the test without the fix on a branch where apache#6634 was reverted to show that this is a regression.
…ed (#9223) * Core: Expired Snapshot files in a transaction should be deleted. (#9183) When a snapshot is expired as part of a transaction, the manifest list file(s) should be deleted when the transaction commits. A recent change (#6634) ensured that these files are not deleted when they have also been committed as part of a transaction, but this breaks the simple case where no new files are committed. Fix this by not skipping deletion when the list of committed files is empty. * Core: Fix logic for determining set of committed files in BaseTransaction when there are no new snapshots (#9221) --------- Co-authored-by: Amogh Jahagirdar <amogh@tabular.io>
…che#9183) When a snapshot is expired as part of a transaction, the manifest list file(s) should be deleted when the transaction commits. A recent change (apache#6634) ensured that these files are not deleted when they have also been committed as part of a transaction, but this breaks the simple case where no new files are committed. Fix this by not skipping deletion when the list of committed files is empty. TESTING: Extended a unit test to ensure that manifest list files are deleted. Ran the test without the fix on a branch where apache#6634 was reverted to show that this is a regression.
…che#9183) When a snapshot is expired as part of a transaction, the manifest list file(s) should be deleted when the transaction commits. A recent change (apache#6634) ensured that these files are not deleted when they have also been committed as part of a transaction, but this breaks the simple case where no new files are committed. Fix this by not skipping deletion when the list of committed files is empty. TESTING: Extended a unit test to ensure that manifest list files are deleted. Ran the test without the fix on a branch where apache#6634 was reverted to show that this is a regression.
Fix for #6632
The way intermediate snapshots in transactions are tracked is incorrect in the presence of branching.
Moving to draft as I read more of the code to make sure this handles different failure cases properly and will add tests if determined this is the right way to fix this issue.