amogh-jahagirdar commented on code in PR #14287:
URL: https://github.com/apache/iceberg/pull/14287#discussion_r2510399050
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -103,7 +104,12 @@ class RemoveSnapshots implements ExpireSnapshots {
@Override
public ExpireSnapshots cleanExpiredFiles(boolean clean) {
- this.cleanExpiredFiles = clean;
+ LOG.warn("cleanExpiredFiles(boolean) is deprecated. Use
cleanMode(CleanupMode) instead.");
+ Preconditions.checkArgument(
Review Comment:
I think the current impleemntation is reasonable so long as there's no
breakages when someone upgrades to 1.11 and using the deprecated API, which it
looks like there's not since the condition is based on whether someone
additionally set the new API. I also prefer the approach of failing if a
non-default cleanup level is set and the old one is also set because it's
pretty unlikely a user intended to do that and it forces them to resolve that
ambiguity that @dramaticlly mentioned.
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -362,9 +380,14 @@ ExpireSnapshots withIncrementalCleanup(boolean
useIncrementalCleanup) {
return this;
}
- private void cleanExpiredSnapshots() {
+ private void cleanExpiredSnapshots(CleanupLevel level) {
Review Comment:
Do we actually need to pass through level here? It's already part of the
local state
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -69,7 +69,7 @@ class RemoveSnapshots implements ExpireSnapshots {
private final Set<Long> idsToRemove = Sets.newHashSet();
private final long now;
private final long defaultMaxRefAgeMs;
- private boolean cleanExpiredFiles = true;
+ private final CleanupLevel defaultCleanupLevel = CleanupLevel.ALL;
Review Comment:
It's somewhat minor to me, but I agree with @stevenzwu here. Generally in
the project the pattern we follow is if there's a default for a given operation
just set the value to that rather than have a separate value. If you see below,
for `cleanExpiredMetadata` we just set the defaults to false.
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -167,6 +173,17 @@ public ExpireSnapshots cleanExpiredMetadata(boolean clean)
{
return this;
}
+ @Override
+ public ExpireSnapshots cleanMode(CleanupMode mode) {
+ Preconditions.checkNotNull(mode, "CleanupMode cannot be null");
+ Preconditions.checkArgument(
Review Comment:
Unresolving this one, I do think this is a case where we should just be
consistent with what's done in the other options in this API, which is just
override what was previously set. e.g. we can set cleanExpiredFiles multiple
times, and it'll just take the last. Any reason why this particular one should
be different and throw @dramaticlly ?
--
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]
