You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Fokko (via GitHub)" <gi...@apache.org> on 2023/12/20 13:24:03 UTC
[PR] Core: Fix missing delete files from transaction [iceberg]
Fokko opened a new pull request, #9354:
URL: https://github.com/apache/iceberg/pull/9354
With retries with conflicting manifest merges.
@rdblue pointed out that this might also occur with the deletes. However, I was unable to replicate this with a test. I've added the test that should uncover this issue when merging DELETE manifests, and deleting the old one before the transaction is successfully committed.
--
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: Fix missing delete files from transaction [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9354:
URL: https://github.com/apache/iceberg/pull/9354#discussion_r1433360860
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -892,15 +892,18 @@ private void cleanUncommittedAppends(Set<ManifestFile> committed) {
}
}
- ListIterator<ManifestFile> deleteManifestsIterator = cachedNewDeleteManifests.listIterator();
- while (deleteManifestsIterator.hasNext()) {
- ManifestFile deleteManifest = deleteManifestsIterator.next();
- if (!committed.contains(deleteManifest)) {
- deleteFile(deleteManifest.path());
- deleteManifestsIterator.remove();
+ boolean hasDeleteDeletes = false;
+ for (ManifestFile cachedNewDeleteManifest : cachedNewDeleteManifests) {
+ if (!committed.contains(cachedNewDeleteManifest)) {
+ deleteFile(cachedNewDeleteManifest.path());
+ hasDeleteDeletes = true;
Review Comment:
Nit: I think a better name would be clearCachedDeleteManifests
--
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: Fix missing delete files from transaction [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #9354:
URL: https://github.com/apache/iceberg/pull/9354
--
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: Fix missing delete files from transaction [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #9354:
URL: https://github.com/apache/iceberg/pull/9354#discussion_r1434520886
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -892,15 +892,18 @@ private void cleanUncommittedAppends(Set<ManifestFile> committed) {
}
}
- ListIterator<ManifestFile> deleteManifestsIterator = cachedNewDeleteManifests.listIterator();
- while (deleteManifestsIterator.hasNext()) {
- ManifestFile deleteManifest = deleteManifestsIterator.next();
- if (!committed.contains(deleteManifest)) {
- deleteFile(deleteManifest.path());
- deleteManifestsIterator.remove();
+ boolean hasDeleteDeletes = false;
+ for (ManifestFile cachedNewDeleteManifest : cachedNewDeleteManifests) {
+ if (!committed.contains(cachedNewDeleteManifest)) {
+ deleteFile(cachedNewDeleteManifest.path());
+ hasDeleteDeletes = true;
Review Comment:
I also looked at that. I think it is for performance reasons since `writer.toManifestFiles();` returns a `list`. For the deletes, we do a `addAll` which is in `O(n)`. I'm a bit torn, I like the performance optimization, but in practice, I don't think that we write that many manifests, so `n` is rather small. Therefore I prefer avoiding nulling it out to make the code easier to read.
--
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: Fix missing delete files from transaction [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #9354:
URL: https://github.com/apache/iceberg/pull/9354#issuecomment-1866627868
Thanks for getting this in @nastra and @Fokko!
--
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: Fix missing delete files from transaction [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9354:
URL: https://github.com/apache/iceberg/pull/9354#discussion_r1433651142
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -892,15 +892,18 @@ private void cleanUncommittedAppends(Set<ManifestFile> committed) {
}
}
- ListIterator<ManifestFile> deleteManifestsIterator = cachedNewDeleteManifests.listIterator();
- while (deleteManifestsIterator.hasNext()) {
- ManifestFile deleteManifest = deleteManifestsIterator.next();
- if (!committed.contains(deleteManifest)) {
- deleteFile(deleteManifest.path());
- deleteManifestsIterator.remove();
+ boolean hasDeleteDeletes = false;
+ for (ManifestFile cachedNewDeleteManifest : cachedNewDeleteManifests) {
+ if (!committed.contains(cachedNewDeleteManifest)) {
+ deleteFile(cachedNewDeleteManifest.path());
+ hasDeleteDeletes = true;
Review Comment:
we can't null out `cachedNewDeleteManifests` because it's `final`. So the only thing that's being done in the code is to clear that collection
--
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: Fix missing delete files from transaction [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9354:
URL: https://github.com/apache/iceberg/pull/9354#discussion_r1433360860
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -892,15 +892,18 @@ private void cleanUncommittedAppends(Set<ManifestFile> committed) {
}
}
- ListIterator<ManifestFile> deleteManifestsIterator = cachedNewDeleteManifests.listIterator();
- while (deleteManifestsIterator.hasNext()) {
- ManifestFile deleteManifest = deleteManifestsIterator.next();
- if (!committed.contains(deleteManifest)) {
- deleteFile(deleteManifest.path());
- deleteManifestsIterator.remove();
+ boolean hasDeleteDeletes = false;
+ for (ManifestFile cachedNewDeleteManifest : cachedNewDeleteManifests) {
+ if (!committed.contains(cachedNewDeleteManifest)) {
+ deleteFile(cachedNewDeleteManifest.path());
+ hasDeleteDeletes = true;
Review Comment:
Nit: I think a better name would be clearCachedDeleteManifests (but I see this was just following the pattern for data file manifests)
--
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: Fix missing delete files from transaction [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9354:
URL: https://github.com/apache/iceberg/pull/9354#discussion_r1433548763
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -892,15 +892,18 @@ private void cleanUncommittedAppends(Set<ManifestFile> committed) {
}
}
- ListIterator<ManifestFile> deleteManifestsIterator = cachedNewDeleteManifests.listIterator();
- while (deleteManifestsIterator.hasNext()) {
- ManifestFile deleteManifest = deleteManifestsIterator.next();
- if (!committed.contains(deleteManifest)) {
- deleteFile(deleteManifest.path());
- deleteManifestsIterator.remove();
+ boolean hasDeleteDeletes = false;
+ for (ManifestFile cachedNewDeleteManifest : cachedNewDeleteManifests) {
+ if (!committed.contains(cachedNewDeleteManifest)) {
+ deleteFile(cachedNewDeleteManifest.path());
+ hasDeleteDeletes = true;
Review Comment:
Also another nit: I'd probably the same pattern we did for the data manifest case where we just null it out and don't exercise the loop if it's null, but I see that the logic is the same.
--
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: Fix missing delete files from transaction [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9354:
URL: https://github.com/apache/iceberg/pull/9354#discussion_r1433548763
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -892,15 +892,18 @@ private void cleanUncommittedAppends(Set<ManifestFile> committed) {
}
}
- ListIterator<ManifestFile> deleteManifestsIterator = cachedNewDeleteManifests.listIterator();
- while (deleteManifestsIterator.hasNext()) {
- ManifestFile deleteManifest = deleteManifestsIterator.next();
- if (!committed.contains(deleteManifest)) {
- deleteFile(deleteManifest.path());
- deleteManifestsIterator.remove();
+ boolean hasDeleteDeletes = false;
+ for (ManifestFile cachedNewDeleteManifest : cachedNewDeleteManifests) {
+ if (!committed.contains(cachedNewDeleteManifest)) {
+ deleteFile(cachedNewDeleteManifest.path());
+ hasDeleteDeletes = true;
Review Comment:
Also another nit: I'd probably use the same pattern we did for the data manifest case where we just null it out and don't exercise the loop if it's null, but I see that the logic is the same with clearing the cached manifests and the loop.
--
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