amogh-jahagirdar commented on code in PR #14287:
URL: https://github.com/apache/iceberg/pull/14287#discussion_r2547097113
##########
core/src/main/java/org/apache/iceberg/ReachableFileCleanup.java:
##########
@@ -48,8 +48,17 @@ class ReachableFileCleanup extends FileCleanupStrategy {
super(fileIO, deleteExecutorService, planExecutorService, deleteFunc);
}
+ /** {@inheritDoc} */
Review Comment:
Nit: Is this required? My understanding is that this is for subclasses so it
inherits the parent docs and then adding additional context for the child class
afterwards. I thought by default, if we just want the parent javadocs, we'll
automatically inherit those? i.e. I thought only having inheritDoc by itself
isn't required. It's useful when it's followed by some additional child class
docs.
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -350,9 +367,11 @@ public void commit() {
TableMetadata updated = internalApply();
ops.commit(base, updated);
});
- LOG.info("Committed snapshot changes");
+ LOG.info(
+ "Committed snapshot changes and prepare to clean up files at level={}",
+ cleanupLevel.name());
- if (cleanExpiredFiles && !base.snapshots().isEmpty()) {
Review Comment:
Minor: I would've preferred for this to just change to cleanupLevel !=
CleanupLevel.NONE instead of nesting the logic further inside
cleanExpiredSnapshots. Now someone has to read cleanExpiredSnapshots() to see
it gets skipped when it's none, whereas I feel like it's better if
cleanExpiredSnapshots just does cleanup.
--
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]
