amogh-jahagirdar commented on code in PR #14923:
URL: https://github.com/apache/iceberg/pull/14923#discussion_r2644162150
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -991,60 +985,42 @@ public Object updateEvent() {
return new CreateSnapshotEvent(tableName, operation(), snapshotId,
sequenceNumber, summary);
}
- @SuppressWarnings("checkstyle:CyclomaticComplexity")
- private void cleanUncommittedAppends(Set<ManifestFile> committed) {
- if (!cachedNewDataManifests.isEmpty()) {
- boolean hasDeletes = false;
- for (ManifestFile manifest : cachedNewDataManifests) {
- if (!committed.contains(manifest)) {
- deleteFile(manifest.path());
- hasDeletes = true;
- }
- }
-
- if (hasDeletes) {
- this.cachedNewDataManifests.clear();
- }
- }
-
- boolean hasDeleteDeletes = false;
- for (ManifestFile cachedNewDeleteManifest : cachedNewDeleteManifests) {
- if (!committed.contains(cachedNewDeleteManifest)) {
- deleteFile(cachedNewDeleteManifest.path());
- hasDeleteDeletes = true;
- }
- }
-
- if (hasDeleteDeletes) {
- this.cachedNewDeleteManifests.clear();
- }
+ @Override
+ protected void cleanUncommitted(Set<ManifestFile> committed) {
+ mergeManager.cleanUncommitted(committed);
+ filterManager.cleanUncommitted(committed);
+ deleteMergeManager.cleanUncommitted(committed);
+ deleteFilterManager.cleanUncommitted(committed);
+ cleanUncommittedAppends(committed);
+ }
+ private void cleanUncommittedAppends(Set<ManifestFile> committed) {
+ deleteUncommitted(cachedNewDataManifests, committed, true /* clear
manifests */);
+ deleteUncommitted(cachedNewDeleteManifests, committed, true /* clear
manifests */);
// rewritten manifests are always owned by the table
- for (ManifestFile manifest : rewrittenAppendManifests) {
- if (!committed.contains(manifest)) {
- deleteFile(manifest.path());
- }
- }
+ deleteUncommitted(rewrittenAppendManifests, committed, false);
// manifests that are not rewritten are only owned by the table if the
commit succeeded
if (!committed.isEmpty()) {
// the commit succeeded if at least one manifest was committed
// the table now owns appendManifests; clean up any that are not used
- for (ManifestFile manifest : appendManifests) {
- if (!committed.contains(manifest)) {
- deleteFile(manifest.path());
- }
- }
+ deleteUncommitted(appendManifests, committed, false);
}
}
- @Override
- protected void cleanUncommitted(Set<ManifestFile> committed) {
- mergeManager.cleanUncommitted(committed);
- filterManager.cleanUncommitted(committed);
- deleteMergeManager.cleanUncommitted(committed);
- deleteFilterManager.cleanUncommitted(committed);
- cleanUncommittedAppends(committed);
+ private void deleteUncommitted(
Review Comment:
great find, yeah I think we can further clean this up!
--
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]
