RussellSpitzer commented on a change in pull request #2985:
URL: https://github.com/apache/iceberg/pull/2985#discussion_r693215532
##########
File path: core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
##########
@@ -458,10 +461,12 @@ private void cleanUncommittedAppends(Set<ManifestFile>
committed) {
this.cachedNewManifest = null;
}
- if (cachedNewDeleteManifest != null &&
!committed.contains(cachedNewDeleteManifest)) {
- deleteFile(cachedNewDeleteManifest.path());
- this.cachedNewDeleteManifest = null;
+ for (ManifestFile cachedNewDeleteManifest : cachedNewDeleteManifests) {
+ if (!committed.contains(cachedNewDeleteManifest)) {
+ deleteFile(cachedNewDeleteManifest.path());
+ }
}
+ this.cachedNewDeleteManifests.clear();
Review comment:
This logic seems a little different than it was previously?
Before
```
if committed doesn't contain cachedNewDeleteManifest
deleteFile()
clear cachedNewDeleteManifest
```
```
for any cachedNewDeleteManifest
if commited doesn't contain cachedNewDeleteManifest
deleteFile
clear all cachedDeleteManifests
```
I'm still trying to understand the check here but it seems like we will
clear out all manifests even if some of them are committed?
Seems like the equivalent would be something like
```
for (ManifestFile cachedNewDeleteManifest : cachedNewDeleteManifests) {
if (!committed.contains(cachedNewDeleteManifest)) {
deleteFile(cachedNewDeleteManifest.path());
this.cachedNewDeleteManifests.remove(cachedNewDeleteManifests) //
Although this would be modifying the list as we iterated through it but you get
the idea
}
}
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
