aokolnychyi commented on a change in pull request #2255: URL: https://github.com/apache/iceberg/pull/2255#discussion_r580744578
########## File path: api/src/main/java/org/apache/iceberg/actions/RemoveOrphanFiles.java ########## @@ -0,0 +1,79 @@ +/* + * 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.actions; + +import java.util.function.Consumer; + +/** + * An action that removes orphan files in a table. + * <p> + * A metadata or data file is considered orphan if it is not reachable by any valid snapshot. + * The set of actual files is built by listing the underlying storage which makes this operation + * expensive. + */ +public interface RemoveOrphanFiles extends Action<RemoveOrphanFiles.Result> { + /** + * Passes a location which should be scanned for orphan files. + * <p> + * If not set, the root table location will be scanned potentially removing both orphan data and + * metadata files. + * + * @param location the location where to look for orphan files + * @return this for method chaining + */ + RemoveOrphanFiles location(String location); + + /** + * Removes orphan files only if they are older than the given timestamp. + * <p> + * This is a safety measure to avoid removing files that are being added to the table. + * For example, there may be a concurrent operation adding new files while the action searches + * for orphan files. New files may not be referenced by the metadata yet but they are not orphan. + * <p> + * If not set, defaults to a timestamp 3 days ago. + * + * @param olderThanTimestamp a long timestamp, as returned by {@link System#currentTimeMillis()} + * @return this for method chaining + */ + RemoveOrphanFiles olderThan(long olderThanTimestamp); + + /** + * Passes an alternative delete implementation that will be used for orphan files. + * <p> + * This method allows to customize the delete func. For example, one may set a custom delete func + * and collect all orphan files into a set instead of physically removing them. + * <p> + * If not called, uses the table's {@link org.apache.iceberg.io.FileIO io} implementation. + * + * @param deleteFunc a function that will be called to delete files + * @return this for method chaining + */ + RemoveOrphanFiles deleteWith(Consumer<String> deleteFunc); + + /** + * The action result that contains a summary of the execution. + */ + interface Result { + /** + * Returns locations of orphan files. + */ + Iterable<String> orphanFileLocations(); Review comment: > Cases like Iterable<ManifestFile> rewrittenManifests() can also benefit from using CloseableIterable. I feel if we want to do it for one, we should do it for all, because we cannot just assume one result might need to close resource, the other does not. If an engine does it for one action, it is likely that it will have this behavior for all actions. Given that it adds complexity, I feel like this should be considered on a case by case basis. For orphan files, it may make sense as we can stream the result but I am not sure about cases like `rewrittenManifests`. Most likely, we would need to materialize the list of manifests to rewrite at some point as we need to process all entries in them. > Why not just use Iterable, and let Result be the object to implement close()? I think this is a better idea than using `ClosebleIterable`. In this case, we probably should make all action results closable for consistency. What does everybody on this thread think? ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
