feat: add option to disable bulk delete for aws#734
Conversation
|
What do you think about @PsiACE suggestion here: #731 (comment)
Adding an option to disable bulk delete does seem reasonable however |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @hengfeiyang -- this code looks good to me -- it is clearly a compatible and opt-in option for the API
Two issues in my mind:
- I had a question about one more test
- @PsiACE's question
| Ok(location) | ||
| } | ||
| }) | ||
| .buffered(20) |
There was a problem hiding this comment.
I wondered how you picked 20 as the concurrency but it seems to mirror the buffered(20) below
There was a problem hiding this comment.
Yes. it is from bellow.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_disable_bulk_delete_uses_single_object_delete() { |
There was a problem hiding this comment.
is there an existing test that shows that bulk_delete actually uses the POST /delete API? I didn't see one immediately
If not I think we should add one to document in test the different behaviors
There was a problem hiding this comment.
let me check this test case.
Address review feedback from apache#734: add a test that demonstrates the default `delete_stream` path uses the bulk `DeleteObjects` API (`POST /?delete`), complementing the existing test for the new `disable_bulk_delete` single-object path. While here, fix the assertion pattern used by these tests. `MockServer` swallows panics inside response closures (the connection resets and the S3 retry logic can still surface a Ok result), so assertions placed inside the closure were silently ignored. Capture request data into shared state and assert in the main test body so violations genuinely fail the test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
hengfeiyang
force-pushed
the
disable-bulk-delete
branch
from
655a486 to
a5c9a86
Compare
|
@alamb i added 4 new unit tests, the key point is different option will generate different result:
Unit tests
|
|
thanks @hengfeiyang and @tustvold |
3 participants
Which issue does this PR close?
Rationale for this change
We discussed in #731 and agree to add an option to disable
bulk_deletefor AWS.What changes are included in this PR?
Add a new option for AWS s3 client
with_disable_bulk_delete.Are there any user-facing changes?
User can use
client.with_disable_bulk_delete(true)to disable bulk delete if his provider doesn't compatible fors3 bulk delete API.