-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Core: Extend ResolvingFileIO to support prefix-based operations #8334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| /* | ||
| * 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.io; | ||
|
|
||
| /** | ||
| * This interface is intended as an extension for FileIO implementations that support being a | ||
| * delegate target. | ||
| */ | ||
| public interface DelegateFileIO extends FileIO, SupportsPrefixOperations, SupportsBulkOperations {} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,18 +31,23 @@ | |
| import org.apache.iceberg.hadoop.SerializableConfiguration; | ||
| import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting; | ||
| import org.apache.iceberg.relocated.com.google.common.base.Joiner; | ||
| import org.apache.iceberg.relocated.com.google.common.base.Preconditions; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.Iterators; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.Lists; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.Maps; | ||
| import org.apache.iceberg.util.SerializableMap; | ||
| import org.apache.iceberg.util.SerializableSupplier; | ||
| import org.apache.iceberg.util.Tasks; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** FileIO implementation that uses location scheme to choose the correct FileIO implementation. */ | ||
| public class ResolvingFileIO implements FileIO, HadoopConfigurable, SupportsBulkOperations { | ||
| /** | ||
| * FileIO implementation that uses location scheme to choose the correct FileIO implementation. | ||
| * Delegate FileIO implementations must implement the {@link DelegateFileIO} mixin interface, | ||
| * otherwise initialization will fail. | ||
| */ | ||
| public class ResolvingFileIO | ||
| implements FileIO, HadoopConfigurable, SupportsBulkOperations, SupportsPrefixOperations { | ||
|
nastra marked this conversation as resolved.
Outdated
|
||
| private static final Logger LOG = LoggerFactory.getLogger(ResolvingFileIO.class); | ||
| private static final int BATCH_SIZE = 100_000; | ||
| private static final String FALLBACK_IMPL = "org.apache.iceberg.hadoop.HadoopFileIO"; | ||
|
|
@@ -55,7 +60,7 @@ public class ResolvingFileIO implements FileIO, HadoopConfigurable, SupportsBulk | |
| "s3n", S3_FILE_IO_IMPL, | ||
| "gs", GCS_FILE_IO_IMPL); | ||
|
|
||
| private final Map<String, FileIO> ioInstances = Maps.newConcurrentMap(); | ||
| private final Map<String, DelegateFileIO> ioInstances = Maps.newConcurrentMap(); | ||
| private final AtomicBoolean isClosed = new AtomicBoolean(false); | ||
| private final transient StackTraceElement[] createStack; | ||
| private SerializableMap<String, String> properties; | ||
|
|
@@ -95,25 +100,12 @@ public void deleteFiles(Iterable<String> pathsToDelete) throws BulkDeletionFailu | |
| Iterators.partition(pathsToDelete.iterator(), BATCH_SIZE) | ||
| .forEachRemaining( | ||
| partitioned -> { | ||
| Map<FileIO, List<String>> pathByFileIO = | ||
| Map<DelegateFileIO, List<String>> pathByFileIO = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can just call the delegate directly now, I don't believe there is a need for this logic anymore
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you think we should still batch here or would we want to essentially use the same implementation you had? To me it seems like batching should probably be handled by the underlying FileIO implementation, but just wanted to see what others think?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, it is probably better to keep what you have as there may be multiple FileIO types involved, and the peek method wasn’t ideal. |
||
| partitioned.stream().collect(Collectors.groupingBy(this::io)); | ||
| for (Map.Entry<FileIO, List<String>> entries : pathByFileIO.entrySet()) { | ||
| FileIO io = entries.getKey(); | ||
| for (Map.Entry<DelegateFileIO, List<String>> entries : pathByFileIO.entrySet()) { | ||
| DelegateFileIO io = entries.getKey(); | ||
| List<String> filePaths = entries.getValue(); | ||
| if (io instanceof SupportsBulkOperations) { | ||
| ((SupportsBulkOperations) io).deleteFiles(filePaths); | ||
| } else { | ||
| LOG.warn( | ||
| "IO {} does not support bulk operations. Using non-bulk deletes.", | ||
| io.getClass().getName()); | ||
| Tasks.Builder<String> deleteTasks = | ||
| Tasks.foreach(filePaths) | ||
| .noRetry() | ||
| .suppressFailureWhenFinished() | ||
| .onFailure( | ||
| (file, exc) -> LOG.warn("Failed to delete file: {}", file, exc)); | ||
| deleteTasks.run(io::deleteFile); | ||
| } | ||
| io.deleteFiles(filePaths); | ||
| } | ||
| }); | ||
| } | ||
|
|
@@ -133,12 +125,12 @@ public void initialize(Map<String, String> newProperties) { | |
| @Override | ||
| public void close() { | ||
| if (isClosed.compareAndSet(false, true)) { | ||
| List<FileIO> instances = Lists.newArrayList(); | ||
| List<DelegateFileIO> instances = Lists.newArrayList(); | ||
|
|
||
| instances.addAll(ioInstances.values()); | ||
| ioInstances.clear(); | ||
|
|
||
| for (FileIO io : instances) { | ||
| for (DelegateFileIO io : instances) { | ||
| io.close(); | ||
| } | ||
| } | ||
|
|
@@ -161,9 +153,9 @@ public Configuration getConf() { | |
| } | ||
|
|
||
| @VisibleForTesting | ||
| FileIO io(String location) { | ||
| DelegateFileIO io(String location) { | ||
| String impl = implFromLocation(location); | ||
| FileIO io = ioInstances.get(impl); | ||
| DelegateFileIO io = ioInstances.get(impl); | ||
| if (io != null) { | ||
| if (io instanceof HadoopConfigurable && ((HadoopConfigurable) io).getConf() == null) { | ||
| synchronized (io) { | ||
|
|
@@ -181,13 +173,14 @@ FileIO io(String location) { | |
| impl, | ||
| key -> { | ||
| Configuration conf = hadoopConf.get(); | ||
| FileIO fileIO; | ||
|
|
||
| try { | ||
| Map<String, String> props = Maps.newHashMap(properties); | ||
| // ResolvingFileIO is keeping track of the creation stacktrace, so no need to do the | ||
| // same in S3FileIO. | ||
| props.put("init-creation-stacktrace", "false"); | ||
| return CatalogUtil.loadFileIO(key, props, conf); | ||
| fileIO = CatalogUtil.loadFileIO(key, props, conf); | ||
| } catch (IllegalArgumentException e) { | ||
| if (key.equals(FALLBACK_IMPL)) { | ||
| // no implementation to fall back to, throw the exception | ||
|
|
@@ -200,7 +193,7 @@ FileIO io(String location) { | |
| FALLBACK_IMPL, | ||
| e); | ||
| try { | ||
| return CatalogUtil.loadFileIO(FALLBACK_IMPL, properties, conf); | ||
| fileIO = CatalogUtil.loadFileIO(FALLBACK_IMPL, properties, conf); | ||
| } catch (IllegalArgumentException suppressed) { | ||
| LOG.warn( | ||
| "Failed to load FileIO implementation: {} (fallback)", | ||
|
|
@@ -213,10 +206,17 @@ FileIO io(String location) { | |
| } | ||
| } | ||
| } | ||
|
|
||
| Preconditions.checkState( | ||
|
nastra marked this conversation as resolved.
|
||
| fileIO instanceof DelegateFileIO, | ||
| "FileIO does not implement DelegateFileIO: " + fileIO.getClass().getName()); | ||
|
|
||
| return (DelegateFileIO) fileIO; | ||
| }); | ||
| } | ||
|
|
||
| private static String implFromLocation(String location) { | ||
| @VisibleForTesting | ||
| String implFromLocation(String location) { | ||
| return SCHEME_TO_FILE_IO.getOrDefault(scheme(location), FALLBACK_IMPL); | ||
| } | ||
|
|
||
|
|
@@ -252,4 +252,14 @@ protected void finalize() throws Throwable { | |
| } | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Iterable<FileInfo> listPrefix(String prefix) { | ||
| return io(prefix).listPrefix(prefix); | ||
| } | ||
|
|
||
| @Override | ||
| public void deletePrefix(String prefix) { | ||
| io(prefix).deletePrefix(prefix); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.