rdblue commented on code in PR #5981:
URL: https://github.com/apache/iceberg/pull/5981#discussion_r996532450
##########
core/src/main/java/org/apache/iceberg/ReachableFileCleanup.java:
##########
@@ -84,19 +77,74 @@ public void cleanFiles(TableMetadata beforeExpiration,
TableMetadata afterExpira
deleteFiles(manifestListsToDelete, "manifest list");
}
- private Set<ManifestFile> readManifests(Set<Snapshot> snapshots) {
- Set<ManifestFile> manifestFiles = Sets.newHashSet();
- for (Snapshot snapshot : snapshots) {
- try (CloseableIterable<ManifestFile> manifestFilesForSnapshot =
readManifestFiles(snapshot)) {
- for (ManifestFile manifestFile : manifestFilesForSnapshot) {
- manifestFiles.add(manifestFile.copy());
- }
- } catch (IOException e) {
- throw new RuntimeIOException(
- e, "Failed to close manifest list: %s",
snapshot.manifestListLocation());
- }
+ // Returns the current set of manifests while pruning the manifests to delete
+ // If all candidate manifests are pruned the returned set of current
manifests will be empty
+ private Set<ManifestFile> findCurrentManifestsAndPruneCandidates(
Review Comment:
I'm not a big fan of doing multiple things in the same helper method. In
this case, I'll admit that it makes sense because we don't want to read all the
manifest lists twice. Still, I find this a bit awkward. The name is long and it
modifies an argument (calling `deletionCandidates.remove`). I'd probably return
both sets to avoid modifying the one passed in, but that's just me. I think
this is okay.
--
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]
