HADOOP-18679. Add API for bulk/paged object deletion #6494
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
steveloughran
force-pushed
the
s3/HADOOP-18679-bulk-delete-api-lean
branch
from
1774c5b to
5afb659
Compare
|
💔 -1 overall
This message was automatically generated. |
|
+add a FileUtils method to assist deletion here, with why so? Makes reflection binding straighforward: no new types; just two methods. |
steveloughran
force-pushed
the
s3/HADOOP-18679-bulk-delete-api-lean
branch
from
5afb659 to
0823d3f
Compare
|
💔 -1 overall
This message was automatically generated. |
steveloughran
force-pushed
the
s3/HADOOP-18679-bulk-delete-api-lean
branch
from
0823d3f to
030ab9f
Compare
4 tasks
A more minimal design that is easier to use and implement. Caller creates a BulkOperation; they get the page size of it and then submit batches to delete of less than that size. The outcome of each call contains a list of failures. S3A implementation to show how straightforward it is. Even with the single entry page size, it is still more efficient to use this as it doesn't try to recreate a parent dir or perform any probes to see if it is a directory: it maps straight to a DELETE call. Change-Id: Ibe8737e7933fe03d39070e7138a710b50c3d60c2
Add methods in FileUtil to take an FS, cast to a BulkDeleteSource then perform the pageSize/bulkDelete operations. This is to make reflection based access straightforward: no new interfaces or types to work with, just two new methods with type-erased lists. Change-Id: I2d7b1bf8198422de635253fc087ca551a8bc6609
Change-Id: Ib098c07cc1f7747ed1a3131b252656c96c520a75
Using this PR to start with the initial design, implementation and services offered by having lower-level interaction with S3 pushed down into an S3AStore class, with interface/impl split. The bulk delete callbacks now to talk to the store, *not* s3afs, with some minor changes in behaviour (IllegalArgumentException is raised if root paths / are to be deleted) Mock tests are failing; I expected that: they are always brittle. What next? get this in and then move lower level fs ops over a method calling s3client at a time, or in groups, as appropriate. The metric of success are: * all callback classes created in S3A FS can work through the store * no s3client direct invocation in S3AFS Change-Id: Ib5bc58991533fd5d13e78426e88b884b6ae5205c
steveloughran
force-pushed
the
s3/HADOOP-18679-bulk-delete-api-lean
branch
from
030ab9f to
ea19f43
Compare
Changing results of method calls, using Tuples.pair() to return Map.Entry() instances as immutable tuples. Change-Id: Ibdd5a5b11fe0a57b293b9cb623272e272c8bab69
| // multi object delete flag | ||
| // this is always true, even if multi object | ||
| // delete is disabled -the page size is simply reduced to 1. | ||
| case CommonPathCapabilities.BULK_DELETE: |
There was a problem hiding this comment.
nit: won't this be a bit misleading?
There was a problem hiding this comment.
it means the API is present and some of the semantics "parent dir existence not guaranteed". For that reason, it will always be faster than before: one DELETE; no LIST/HEAD etc
| @Retries.RetryTranslated | ||
| private List<Map.Entry<String, String>> deleteSingleObject(final String key) throws IOException { | ||
| try { | ||
| once("bulkDelete", path, () -> |
There was a problem hiding this comment.
nit : this is a single object delete,
...hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/BulkDeleteOperationCallbacksImpl.java
Show resolved
Hide resolved
ahmarsuhail
requested changes
Mar 28, 2024
|
|
||
| If multi-object delete is enabled (`fs.s3a.multiobjectdelete.enable` = true), as | ||
| it is by default, then the page size is limited to that defined in | ||
| `fs.s3a.bulk.delete.page.size`, which MUST be less than or equal to1000. |
| public static List<Map.Entry<Path, String>> bulkDelete(FileSystem fs, Path base, List<Path> paths) | ||
| ``` | ||
|
|
||
| ## S3A Implementation |
There was a problem hiding this comment.
Can we move this section to S3A docs instead, and link here? feel like S3A docs in hadoop-common are hard to find
| @@ -46,6 +46,9 @@ public final class StoreStatisticNames { | |||
| /** {@value}. */ | |||
| public static final String OP_APPEND = "op_append"; | |||
|
|
|||
| /** {@value}. */ | |||
| public static final String OP_BULK_DELETE = "op_bulk-delete"; | |||
There was a problem hiding this comment.
super nit: change to op_bulk_delete to match how other OPs are named
|
FYI i want to pull the rate limiter API of #6596 in here too; we'd have a rate limiter in s3a store which if enabled would limit #of deletes which can be issued on a bucket. Ideally it'd be at 3000 on s3 standard, off for s3 express and third party stores, so reduce load this call can generate. |
|
In #6686 I'm creating a new utils class for reflection access, nothing else. And proposing that all tests of the API use reflection to be really confident it works and that there's no accidental changes which break reflection |
4 tasks
| /** | ||
| * Delete a list of files/objects. | ||
| * <ul> | ||
| * <li>Files must be under the path provided in {@link #basePath()}.</li> |
There was a problem hiding this comment.
writing contract tests for this locally., can't find the implementation of this in S3A.
| * The maximum number of objects/files to delete in a single request. | ||
| * @return a number greater than or equal to zero. | ||
| */ | ||
| int pageSize(); |
There was a problem hiding this comment.
shouldn't this be greater than 0?
equal to 0 doesn't make sense. also we have the check in S3A impl.
...hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/BulkDeleteOperationCallbacksImpl.java
Show resolved
Hide resolved
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
HADOOP-18679.
A more minimal design that is easier to use and implement than #5993
Caller creates a BulkOperation; they get the page size of it and then submit batches to delete of less than that size.
The outcome of each call contains a list of failures.
S3A implementation to show how straightforward it is.
Even with the single entry page size, it is still more efficient to use this as it doesn't try to recreate a parent dir or perform any probes to see if it is a directory: it maps straight to a DELETE call.
How was this patch tested?
If the design looks good, I'll write some contract tests as well as a filesystem api
specification.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?