feat: support bulk deletes in object_store #4060

wjones127 · 2023-04-12T02:23:11Z

Which issue does this PR close?

Closes #2615.

Rationale for this change

Provides methods for quickly deleting large numbers of objects, such as when dropping a Parquet table.

What changes are included in this PR?

Introduces two new methods on ObjectStore, each with a default implementation. One provides the bulk deletion method. Another provides the number of objects that can be deleted in one underlying call. The latter can be used if the user wants to control the parallelism themselves or if they want to implement progress tracking.

Are there any user-facing changes?

Adds new APIs, with inline documentation.

tustvold · 2023-04-12T05:42:20Z

object_store/src/lib.rs

+    /// this method to customize the parallelism or provide a progress indicator.
+    ///
+    /// This method may create multiple threads to perform the deletions in parallel.
+    async fn delete_all(&self, locations: Vec<Path>) -> Result<()> {


What do you think about making this instead something like

Suggested change

async fn delete_all(&self, locations: Vec<Path>) -> Result<()> {

async fn delete_all(&self, locations: Vec<Path>) -> Result<BoxStream<'_, Result<()>>> {

This would allow for granular error and progress reporting? One could even go so far as to make the input also a BoxStream 🤔

wjones127 · 2023-04-24T00:03:05Z

I think to implement Azure blob store and GCS, we'll need to do work upstream:

seanmonstar/reqwest#1241

They both used the mixed/multipart request type and to do proper error handling we'll want to have robust implementations of it. So I'm going to wait on those two.

wjones127 · 2023-05-21T03:06:21Z

✅ I've run the integration tests against AWS S3 and Cloudflare R2. Both pass 👍 (although Cloudflare fails the get_opts test right now).

tustvold

Thank you for this, I left some comments

tustvold · 2023-05-22T09:19:35Z

object_store/src/aws/client.rs

@@ -129,6 +153,43 @@ struct MultipartPart {
    part_number: usize,
 }

+#[derive(Deserialize)]
+#[serde(rename_all = "PascalCase", rename = "DeleteResult")]
+struct BatchDeleteResponse {


Given we seem to have enabled a feature to get this to deserialize correctly, could we perhaps get a basic test of this deserialization logic - i.e. given a payload with mixed success and failures, it deserialized correctly?

tustvold · 2023-05-22T09:20:38Z

object_store/src/aws/client.rs

+
+        let inner_body = paths
+            .iter()
+            .map(|path| format!("<Object><Key>{}</Key></Object>", path))


I think this might run into weirdness due to XML escaping, could we use quick-xml to serialize this payload instead of string formatting?

I think Path allows '&' for example, a test of this would be superb...

I've added a test with characters special to XML

object_store/src/lib.rs

object_store/src/util.rs

tustvold · 2023-05-22T09:26:34Z

object_store/src/util.rs

@@ -170,6 +178,22 @@ fn merge_ranges(
    ret
 }

+/// Common implementation for delete_all
+#[allow(dead_code)]
+pub(crate) fn delete_all_helper<'a>(


I wonder if we could follow the pattern in #4220 of using extension traits for this

TBH, I think the helper turned out not to be necessary, so I removed it entirely.

tustvold · 2023-05-22T09:31:39Z

object_store/src/aws/mod.rs

+    fn delete_stream<'a>(
+        &'a self,
+        locations: BoxStream<'a, Path>,
+    ) -> BoxStream<'a, BoxFuture<'a, Result<Vec<Result<Path>>>>> {


This signature whilst it does provide the maximum flexibility to the upstreams, is kind of obnoxious to use

What do you think of returning BoxStream<'a, Result<Path>> and letting the individual stores control the concurrency, much like we do for coalesce_ranges? This would have the advantage of letting them choose an appropriate value

It would also avoid overheads for stores that don't support bulk deletes, FWIW

I think this signature is fine, but how would I implement limit store with it?

Neither signature would really let you do this meaningfully, as neither exposes the granularity of the requests. I personally think this is fine, I suspect we may add request limiting to ClientOptions eventually to handle this

object_store/src/limit.rs

tustvold · 2023-05-23T09:47:46Z

object_store/src/lib.rs

        let paths = flatten_list_stream(storage, None).await.unwrap();
-
-        for f in &paths {
-            storage.delete(f).await.unwrap();
-        }
+        storage
+            .delete_stream(futures::stream::iter(paths).boxed())


Thinking about this use-case, I wonder if the input should be a fallible stream, what do you think? You could then feed the output of the list operation directly into the bulk delete API

It makes the API a little funky but that might be fine. In most of my downstream use cases, I'm probably passing in a Vec or iterator, so I'm already going to wrap in future::stream::iter. Having to add a .map(Ok) seems fine.

I mean the alternative would be to just pass in Vec<Path>, but I think if we're going to go the path of providing a streaming interface we should at least make it usable. I would be happy with either tbh, Vec<Path> does have the advantage of being simpler... Up to you 😄

tustvold

Looking really nice, thank you for sticking with this. I think this is good to go, but left one final thought on the signature that I'd be interested to here your take on

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

tustvold · 2023-05-23T15:08:53Z

object_store/src/aws/mod.rs

    ) -> BoxStream<'a, Result<Path>> {
        locations
            .chunks(1_000)
            .map(move |locations| async {
+                let locations: Vec<Path> =
+                    locations.into_iter().collect::<Result<_>>()?;


I think it would be better to use try_chunks, as it will short-circuit on the first error

tustvold · 2023-05-23T15:09:16Z

object_store/src/lib.rs

@@ -1578,9 +1578,15 @@ mod tests {
    }

    async fn delete_fixtures(storage: &DynObjectStore) {
-        let paths = flatten_list_stream(storage, None).await.unwrap();
+        // let paths = flatten_list_stream(storage, None).await.unwrap();


Suggested change

// let paths = flatten_list_stream(storage, None).await.unwrap();

tustvold

Looking nice 😄

alamb · 2023-05-25T18:13:32Z

Shall we merge it?

wjones127 · 2023-05-25T18:29:56Z

I'll do some final clean ups later today, then it should be ready.

github-actions bot added the object-store Object Store Interface label Apr 12, 2023

tustvold reviewed Apr 12, 2023

View reviewed changes

roeap mentioned this pull request May 16, 2023

vacuum is very slow on Cloudflare R2 delta-io/delta-rs#1366

Closed

feat: support bulk deletes

3ad50bf

wjones127 force-pushed the batch-delete branch from 32cc1ac to 3ad50bf Compare May 20, 2023 21:46

wjones127 added 5 commits May 20, 2023 16:01

fix: make NotFound reporting consistent

3eac131

fix http store

8bbb4a9

fix aws support

ce72902

remove unnecessary flag

b768c9b

fix: make AWS S3 compatible

ed81cee

wjones127 marked this pull request as ready for review May 21, 2023 03:05

wjones127 requested a review from tustvold May 21, 2023 03:23

tustvold reviewed May 22, 2023

View reviewed changes

wjones127 added 2 commits May 22, 2023 20:19

pr feedback: use simpler API

dfa8e45

pr feedback: test paths and ordering

2396364

tustvold reviewed May 23, 2023

View reviewed changes

object_store/src/limit.rs Outdated Show resolved Hide resolved

tustvold reviewed May 23, 2023

View reviewed changes

tustvold approved these changes May 23, 2023

View reviewed changes

wjones127 and others added 2 commits May 23, 2023 07:47

Update object_store/src/limit.rs

a4de050

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

take fallible stream

74f8feb

tustvold reviewed May 23, 2023

View reviewed changes

tustvold approved these changes May 23, 2023

View reviewed changes

final pr feedback

189f204

wjones127 requested a review from tustvold May 27, 2023 01:34

tustvold merged commit 770e241 into apache:master May 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support bulk deletes in object_store #4060

feat: support bulk deletes in object_store #4060

wjones127 commented Apr 12, 2023

tustvold Apr 12, 2023

wjones127 commented Apr 24, 2023

wjones127 commented May 21, 2023

tustvold left a comment

tustvold May 22, 2023

wjones127 May 23, 2023

tustvold May 22, 2023

wjones127 May 23, 2023

tustvold May 22, 2023

wjones127 May 23, 2023

tustvold May 22, 2023 •

edited

Loading

wjones127 May 23, 2023

tustvold May 23, 2023

tustvold May 23, 2023 •

edited

Loading

wjones127 May 23, 2023

tustvold May 23, 2023 •

edited

Loading

tustvold left a comment

tustvold May 23, 2023

tustvold May 23, 2023

tustvold left a comment

alamb commented May 25, 2023

wjones127 commented May 25, 2023

	async fn delete_all(&self, locations: Vec<Path>) -> Result<()> {
	async fn delete_all(&self, locations: Vec<Path>) -> Result<BoxStream<'_, Result<()>>> {

feat: support bulk deletes in object_store #4060

feat: support bulk deletes in object_store #4060

Conversation

wjones127 commented Apr 12, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

wjones127 commented Apr 24, 2023

wjones127 commented May 21, 2023

tustvold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold May 23, 2023 • edited Loading

Choose a reason for hiding this comment

tustvold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold left a comment

Choose a reason for hiding this comment

alamb commented May 25, 2023

wjones127 commented May 25, 2023

tustvold May 22, 2023 •

edited

Loading

tustvold May 23, 2023 •

edited

Loading

tustvold May 23, 2023 •

edited

Loading