Use randomized content ID for Azure multipart uploads #6869

avarnon · 2024-12-11T13:35:21Z

Which issue does this PR close?

Closes #6868.

Rationale for this change

Explained in the issue

What changes are included in this PR?

In object_store's AzureClient.put_block(), use a random, u128 for part_idx instead of the counter based part_idx parameter.

Are there any user-facing changes?

No

tustvold

Could we get a test showing concurrent uploads not interfering with each other?

object_store/src/azure/client.rs

Co-authored-by: Raphael Taylor-Davies <[email protected]>

avarnon · 2024-12-11T18:30:26Z

Could we get a test showing concurrent uploads not interfering with each other?

@tustvold, I have added an integration test (azure_parallel_put_multipart_test) to src/azure/mod.rs which tries to interleave blocks from 2 separate streams. The test fails without the changes to /home/avarnon/code/github/apache/arrow-rs/object_store/src/azure/client.rs because the last block for each block ID is committed.

tustvold · 2024-12-12T20:41:30Z

object_store/src/azure/mod.rs

+            .unwrap();
+
+        multipart_upload_1.complete().await.unwrap();
+        let err = multipart_upload_2.complete().await.unwrap_err();


Perhaps I have misunderstood something about this, but I would have expected the last writer to win? This would be consistent with other stores, e.g. S3 / GCS?

Without the changes to object_store/src/azure/client.rs, the final file shows it has parts from both streams. With the changes, the file is completely from the first stream to complete.

if you look at the test setup, you can see that I am alternating which stream is writing a block first/second

Right I get that, my point is that racing multipart uploads shouldn't fail, they should both succeed with the last writer winning. That is the expected semantic, making me think this is an incomplete fix.

It would be ideal if this integration test could be lifted into the generic test suite, to ensure all the various platforms behave consistently in this regard.

the problem is that Azure Storage returns InvalidBlockList on the second complete no matter what. I don't think Azure is allowing the second complete to succeed since it's already shipped off the final file.

This seems to be at odds with the documentation? https://learn.microsoft.com/en-us/rest/api/storageservices/put-block-list?tabs=microsoft-entra-id

In particular

Any uncommitted blocks are garbage collected if there are no successful calls to Put Block or Put Block List on the blob within a week following the last successful Put Block operation. If Put Blob is called on the blob, any uncommitted blocks are garbage collected.

Have you tried this against Azure proper, or just azurite?

I tried this against a real Azure storage account

Whelp, this wouldn't be the first time Azure copied AWS's homework badly... I guess an error is better than silently interleaving the bytes

I wonder if calling Put Block List is equivalent to Put Blob and is triggering garbage collection? I agree that consistency with an error is better than interleaving.

tustvold · 2024-12-12T20:43:41Z

object_store/src/azure/mod.rs

@@ -392,4 +396,99 @@ mod tests {
            azure_storage_token
        );
    }
+
+    #[tokio::test]


This will need to be folded into azure_blob_test above as otherwise it will race

Looks to still be here?

Apologies. I have pushed an update

tustvold · 2024-12-12T21:19:47Z

object_store/src/integration.rs

@@ -1109,3 +1111,67 @@ async fn delete_fixtures(storage: &DynObjectStore) {
        .await
        .unwrap();
 }
+
+/// Tests a race condition where 2 threads are performing multipart writes to the same path
+pub async fn multipart_race_condition(storage: &dyn ObjectStore) {


If we're going to make this a top-level integration test can we add an argument last_writer_wins: bool, hook this up to allow a non-error return, and then add this to the AWS and GCS test suites?

Sure! I don't have access to an AWS or GCS account to test those live

Feel free to leave that to the CI

tustvold · 2024-12-12T21:24:30Z

object_store/src/integration.rs


-    assert!(matches!(err, crate::Error::Generic { .. }), "{err}");
+    if last_writer_wins {


I suspect this should also yield a different final result

You are correct. I missed that. Pushed

avarnon · 2024-12-12T21:35:05Z

Pushed a fixup to satisfy AWS S3's minimum allowed upload size of 5,242,880

tustvold · 2024-12-12T21:44:02Z

The limit is the other way round, all bar the last part must be 5MiB or more. Feel free to leave for now if you prefer

avarnon · 2024-12-12T21:51:40Z

I have pushed a change which should make each block big enough for S3

tustvold · 2024-12-12T22:59:04Z

Thanks for this

Use randomized content ID for Azure multipart uploads

3e22cbc

github-actions bot added the object-store Object Store Interface label Dec 11, 2024

tustvold reviewed Dec 11, 2024

View reviewed changes

object_store/src/azure/client.rs Outdated Show resolved Hide resolved

avarnon and others added 2 commits December 11, 2024 09:45

Update object_store/src/azure/client.rs

4181cde

Co-authored-by: Raphael Taylor-Davies <[email protected]>

fixup! Use randomized content ID for Azure multipart uploads

c2839a5

fixup! Use randomized content ID for Azure multipart uploads

a4e8628

tustvold reviewed Dec 12, 2024

View reviewed changes

fixup! Use randomized content ID for Azure multipart uploads

d11eb21

tustvold reviewed Dec 12, 2024

View reviewed changes

fixup! Use randomized content ID for Azure multipart uploads

5edc040

avarnon force-pushed the avarnon-random-multipart-part-idx branch from f4139db to 5edc040 Compare December 12, 2024 21:21

fixup! Use randomized content ID for Azure multipart uploads

7ca0643

tustvold reviewed Dec 12, 2024

View reviewed changes

avarnon added 2 commits December 12, 2024 16:25

fixup! Use randomized content ID for Azure multipart uploads

b4fd825

fixup! Use randomized content ID for Azure multipart uploads

fae706b

avarnon force-pushed the avarnon-random-multipart-part-idx branch from 3ebef59 to fae706b Compare December 12, 2024 21:34

fixup! Use randomized content ID for Azure multipart uploads

e6a4c74

tustvold merged commit 84dba34 into apache:main Dec 12, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use randomized content ID for Azure multipart uploads #6869

Use randomized content ID for Azure multipart uploads #6869

avarnon commented Dec 11, 2024 •

edited

Loading

tustvold left a comment

avarnon commented Dec 11, 2024 •

edited

Loading

tustvold Dec 12, 2024

avarnon Dec 12, 2024

avarnon Dec 12, 2024

tustvold Dec 12, 2024 •

edited

Loading

avarnon Dec 12, 2024

tustvold Dec 12, 2024

avarnon Dec 12, 2024

tustvold Dec 12, 2024

avarnon Dec 12, 2024

tustvold Dec 12, 2024

avarnon Dec 12, 2024

tustvold Dec 12, 2024

avarnon Dec 12, 2024

tustvold Dec 12, 2024

avarnon Dec 12, 2024

tustvold Dec 12, 2024

avarnon Dec 12, 2024

tustvold Dec 12, 2024

avarnon Dec 12, 2024

avarnon commented Dec 12, 2024

tustvold commented Dec 12, 2024

avarnon commented Dec 12, 2024

tustvold commented Dec 12, 2024


		assert!(matches!(err, crate::Error::Generic { .. }), "{err}");
		if last_writer_wins {

Use randomized content ID for Azure multipart uploads #6869

Use randomized content ID for Azure multipart uploads #6869

Conversation

avarnon commented Dec 11, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold left a comment

Choose a reason for hiding this comment

avarnon commented Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Dec 12, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avarnon commented Dec 12, 2024

tustvold commented Dec 12, 2024

avarnon commented Dec 12, 2024

tustvold commented Dec 12, 2024

avarnon commented Dec 11, 2024 •

edited

Loading

avarnon commented Dec 11, 2024 •

edited

Loading

tustvold Dec 12, 2024 •

edited

Loading