-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Core: Extend ResolvingFileIO to support prefix-based operations #8334
Core: Extend ResolvingFileIO to support prefix-based operations #8334
Conversation
f88ae6a
to
0e52578
Compare
@@ -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 = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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?
// peek at the first element to determine the type of FileIO
Iterator<String> originalIterator = pathsToDelete.iterator();
if (!originalIterator.hasNext()) {
return;
}
PeekingIterator<String> iterator = Iterators.peekingIterator(originalIterator);
DelegateFileIO fileIO = io(iterator.peek());
fileIO.deleteFiles(() -> iterator);
To me it seems like batching should probably be handled by the underlying FileIO implementation, but just wanted to see what others think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.
767280b
to
a3c0ece
Compare
@nastra @RussellSpitzer Should ADLSFileIO implement DelgateFileIO as GCSFileIO to make code clean and consistently? |
@leoluan2009 yes it should, good catch. I've opened #8563 to address that |
No description provided.