[Enhancement] Refactor Cleanup Task Handler #516
Conversation
danielhumanmod
requested review from
jbonofre,
ashvina,
RussellSpitzer,
snazy,
vvcephei,
takidau,
jackye1995,
flyrain,
eric-maynard,
collado-mike,
ebyhr and
adutra
as code owners
… refactor-cleanup-task
… refactor-cleanup-task
| .atWarn() | ||
| .addKeyValue("batchFiles", batchFiles.toString()) | ||
| .addKeyValue("tableId", tableId) | ||
| .log("File batch cleanup task scheduled, but the none of the file in batch exists"); |
There was a problem hiding this comment.
| .log("File batch cleanup task scheduled, but the none of the file in batch exists"); | |
| .log("File batch cleanup task scheduled, but none of the files in batch exists"); |
|
|
||
| @Override | ||
| public boolean canHandleTask(TaskEntity task) { | ||
| throw new UnsupportedOperationException("This method must be implemented by subclasses."); |
There was a problem hiding this comment.
Why not use an abstract class then?
| return LOGGER; | ||
| } | ||
|
|
||
| public static final class BatchFileCleanupTask { |
| allDeletes.join(); | ||
| } catch (Exception e) { | ||
| LOGGER.error("Exception detected during batch files deletion", e); | ||
| return false; |
There was a problem hiding this comment.
Imho this a bit too coarse-grained. The likelihood of having one failed delete will grow bigger with the number of files to delete. Shouldn't we return a more meaningful result, e.g. a statistics object reporting how many files were deleted, and how many failed? Not saying you should change this now though, because returning a boolean is imposed by TaskHandler API, so we need to look into improving the API itself.
There was a problem hiding this comment.
That’s a great point! Since this is the first time we’re introducing a batch files handler, it would be helpful to discuss how to refine the TaskHandler API for future scenarios. @collado-mike, do you have any suggestions or thoughts on this?
|
Thank you for the valuable feedback, @adutra ! Your comments were super helpful in improving the code quality. I’ve incorporated your suggestions and updated the code in the latest commit. |
| @Override | ||
| public abstract boolean handleTask(TaskEntity task); | ||
|
|
||
| public abstract Logger getLogger(); |
There was a problem hiding this comment.
This is imho not necessary, using this class' LOGGER is enough.
There was a problem hiding this comment.
This is imho not necessary, using this class'
LOGGERis enough.
Thanks for reminding, already fixed it!
| // Schedule the deletion for each file asynchronously | ||
| List<CompletableFuture<Void>> deleteFutures = | ||
| validFiles.stream() | ||
| .map(file -> super.tryDelete(tableId, authorizedFileIO, null, file, null, 1)) |
There was a problem hiding this comment.
Hm, I'd have expected that this class actually leverages batched deletes of the underlying object store, but seems it does not.
| List<String> batchFiles = cleanupTask.batchFiles(); | ||
| try (FileIO authorizedFileIO = fileIOSupplier.apply(task)) { | ||
| List<String> validFiles = | ||
| batchFiles.stream().filter(file -> TaskUtils.exists(file, authorizedFileIO)).toList(); |
There was a problem hiding this comment.
Here and in line 69 are exists-checks performed for each individual file, which is inefficient (slow) and also incurs unnecessary cost for cloud object storages.
I'd prefer an approach that
a) leverages object-storage batch requests
b) does not perform the same operation more than once
Description
This PR is created to refactor
ManifestFileCleanupTaskHandler.java. The key changes include:FileCleanupTaskHandler.javaBatchFileCleanupTaskHandler.javaManifestFileCleanupTaskHandler.javaFixes #515
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist:
Please delete options that are not relevant.