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?
--
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]
