Core, API: Support cleanupMode in snapshot expiration #14287
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.apache.iceberg; | ||
|
|
||
| /** An enum representing possible clean up mode used in snapshot expiration. */ | ||
| public enum CleanupMode { | ||
| NONE(0), | ||
| METADATA_ONLY(1), | ||
| ALL(2); | ||
|
|
||
| CleanupMode(int id) { | ||
| this.id = id; | ||
| } | ||
|
|
||
| private final int id; | ||
|
|
||
| public int id() { | ||
| return id; | ||
| } | ||
|
|
||
| public boolean requireCleanup() { | ||
|
dramaticlly marked this conversation as resolved.
Outdated
|
||
| return this.id > 0; | ||
| } | ||
|
|
||
| public static CleanupMode fromId(int id) { | ||
| switch (id) { | ||
| case 0: | ||
| return NONE; | ||
| case 1: | ||
| return METADATA_ONLY; | ||
| case 2: | ||
| return ALL; | ||
| } | ||
| throw new IllegalArgumentException("Unknown cleanup mode: " + id); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,7 +69,7 @@ class RemoveSnapshots implements ExpireSnapshots { | |
| private final Set<Long> idsToRemove = Sets.newHashSet(); | ||
| private final long now; | ||
| private final long defaultMaxRefAgeMs; | ||
| private final CleanupMode defaultCleanupMode = CleanupMode.ALL; | ||
| private TableMetadata base; | ||
| private long defaultExpireOlderThan; | ||
| private int defaultMinNumSnapshots; | ||
|
|
@@ -79,6 +79,7 @@ class RemoveSnapshots implements ExpireSnapshots { | |
| private Boolean incrementalCleanup; | ||
| private boolean specifiedSnapshotId = false; | ||
| private boolean cleanExpiredMetadata = false; | ||
| private CleanupMode cleanupMode = defaultCleanupMode; | ||
|
|
||
| RemoveSnapshots(TableOperations ops) { | ||
| this.ops = ops; | ||
|
|
@@ -103,7 +104,12 @@ class RemoveSnapshots implements ExpireSnapshots { | |
|
|
||
| @Override | ||
| public ExpireSnapshots cleanExpiredFiles(boolean clean) { | ||
| LOG.warn("cleanExpiredFiles(boolean) is deprecated. Use cleanMode(CleanupMode) instead."); | ||
|
dramaticlly marked this conversation as resolved.
Outdated
|
||
| Preconditions.checkArgument( | ||
|
Contributor
There was a problem hiding this comment. I'm not sure about throwing here, why not just override whatever the cleanup mode was set to?
Contributor
Author
There was a problem hiding this comment. Thanks Eduard, I added such validation mainly want to prevent double intention where one set both the cleanExpired files as well as cleanupMode, so intention is not clear and override might be risky, this could lead to some unexpected results so throw early maybe helpful. This can be found more in my unit test named
Contributor
There was a problem hiding this comment. it seems ok to throw an exception here because we don't want to users to call both setters. only one should be used.
Contributor
There was a problem hiding this comment. but the condition may not be precise/perfect. e.g., if the client called the other way is to default maybe the complexity is not worth it. I am wondering if it is simpler to just go with what @nastra suggested. just rely on API deprecation to move users away from the old API.
Contributor
Author
There was a problem hiding this comment. I think there's some nuance here and value to keep: Take what I included in the unit test for an example here, use preconditions check here prevent the case where both Although unlikely, for the corner case discussed here when
Contributor
There was a problem hiding this 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. |
||
| cleanupMode == defaultCleanupMode, | ||
| "Cannot set cleanExpiredFiles when cleanMode has already been set to: %s", | ||
| cleanupMode); | ||
| this.cleanupMode = clean ? CleanupMode.ALL : CleanupMode.NONE; | ||
| return this; | ||
| } | ||
|
|
||
|
|
@@ -167,6 +173,17 @@ public ExpireSnapshots cleanExpiredMetadata(boolean clean) { | |
| return this; | ||
| } | ||
|
|
||
| @Override | ||
| public ExpireSnapshots cleanMode(CleanupMode mode) { | ||
| Preconditions.checkNotNull(mode, "CleanupMode cannot be null"); | ||
|
dramaticlly marked this conversation as resolved.
Outdated
|
||
| Preconditions.checkArgument( | ||
|
Contributor
There was a problem hiding this comment. same as mentioned earlier about throwing
Contributor
There was a problem hiding this 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 ?
Contributor
Author
There was a problem hiding this comment. I added this mostly want to avoid ambiguous intention when user set both options together regardless of which option is chained first. i.e both shall fail with exception to ask user for further clarification table.expireSnapshots()
.cleanupLevel(ExpireSnapshots.CleanupLevel.METADATA_ONLY)
.cleanExpiredFiles(false)and table
.expireSnapshots()
.cleanExpiredFiles(false)
.cleanupLevel(ExpireSnapshots.CleanupLevel.METADATA_ONLY)Updated preconditions to better assertion condition and message |
||
| cleanupMode == defaultCleanupMode, | ||
| "Cannot set cleanMode when it has already been set to: %s", | ||
| cleanupMode); | ||
| this.cleanupMode = mode; | ||
| return this; | ||
| } | ||
|
|
||
| @Override | ||
| public List<Snapshot> apply() { | ||
| TableMetadata updated = internalApply(); | ||
|
|
@@ -184,6 +201,8 @@ private TableMetadata internalApply() { | |
| return base; | ||
| } | ||
|
|
||
| LOG.debug("Using cleanup mode: {}", cleanupMode); | ||
|
|
||
| Set<Long> idsToRetain = Sets.newHashSet(); | ||
| // Identify refs that should be removed | ||
| Map<String, SnapshotRef> retainedRefs = computeRetainedRefs(base.refs()); | ||
|
|
@@ -352,8 +371,8 @@ public void commit() { | |
| }); | ||
| LOG.info("Committed snapshot changes"); | ||
|
|
||
|
dramaticlly marked this conversation as resolved.
|
||
| if (cleanupMode.requireCleanup() && !base.snapshots().isEmpty()) { | ||
| cleanExpiredSnapshots(cleanupMode); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -362,7 +381,7 @@ ExpireSnapshots withIncrementalCleanup(boolean useIncrementalCleanup) { | |
| return this; | ||
| } | ||
|
|
||
| private void cleanExpiredSnapshots(CleanupMode mode) { | ||
| TableMetadata current = ops.refresh(); | ||
|
|
||
| if (Boolean.TRUE.equals(incrementalCleanup)) { | ||
|
|
@@ -380,9 +399,9 @@ private void cleanExpiredSnapshots() { | |
| FileCleanupStrategy cleanupStrategy = | ||
| incrementalCleanup | ||
| ? new IncrementalFileCleanup( | ||
| ops.io(), deleteExecutorService, planExecutorService(), deleteFunc, mode) | ||
| : new ReachableFileCleanup( | ||
| ops.io(), deleteExecutorService, planExecutorService(), deleteFunc, mode); | ||
|
|
||
| cleanupStrategy.cleanFiles(base, current); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.