Open Write #13344

seanmcc-msft · 2020-07-09T17:54:35Z

Continued from #12779 and #12662

seanmcc-msft · 2020-07-09T18:04:22Z

@PaulVrugt, we are continuing Open Write here.

PaulVrugt · 2020-07-09T18:05:30Z

Cool, thanks for the heads up!

jaschrep-msft

Mostly nitpicking, but there are some potential bugs to be addressed. Also some questions on tiny design elements (nothing major).
Will review tests when they are completed.

sdk/storage/Azure.Storage.Blobs/api/Azure.Storage.Blobs.netstandard2.0.cs

sdk/storage/Azure.Storage.Blobs/src/AppendBlobClient.cs

sdk/storage/Azure.Storage.Blobs/src/AppendBlobWriteStream.cs

sdk/storage/Azure.Storage.Files.Shares/src/ShareFileClient.cs

sdk/storage/Azure.Storage.Files.Shares/src/ShareFileWriteStream.cs

seanmcc-msft · 2020-08-03T16:35:53Z

We should test StorageWriteStream on its own in Azure.Storage.Common.Tests, mocking out the abstract behavior. I'm particularly interested in testing edge cases on combining Stream.WriteAsync and Stream.FlushAsync calls, observing when/how they trigger AppendInternal calls that actually send data to the service.

A lot of this logic is handled in the implementation streams, I'm not sure how valuable having tests just for StorageWriteStream will be.

sdk/storage/Azure.Storage.Common/tests/StorageWriteStreamTests.cs

tg-msft

Does the Java implementation share the exact semantics around which conditions are used, updated, and cleared at various points in the life cycle? What about Track 1?

sdk/storage/Azure.Storage.Blobs/api/Azure.Storage.Blobs.netstandard2.0.cs

sdk/storage/Azure.Storage.Common/src/Shared/Constants.cs

sdk/storage/Azure.Storage.Common/src/Shared/PooledMemoryStream.cs

sdk/storage/Azure.Storage.Common/src/Shared/StorageWriteStream.cs

sdk/storage/Azure.Storage.Blobs/src/PageBlobClient.cs

sdk/storage/Azure.Storage.Blobs/src/PageBlobWriteStream.cs

sdk/storage/Azure.Storage.Blobs/src/AppendBlobClient.cs

sdk/storage/Azure.Storage.Blobs/src/PageBlobClient.cs

sdk/storage/Azure.Storage.Files.DataLake/src/DataLakeFileClient.cs

sdk/storage/Azure.Storage.Files.Shares/src/ShareFileClient.cs

seanmcc-msft · 2020-08-08T00:22:35Z

sdk/storage/Azure.Storage.Blobs/src/PageBlobClient.cs

+#pragma warning restore AZC0015 // Unexpected client method return type.
+            bool overwrite,
+            long position,
+            long? size = default,


I chose to add an optional size parameter, and mentioned in the xml comments it is required if overwrite is set to true, or the Page Blob is being created for the first time.

We should move at least size and maybe position (if it can default to 0 when creating a new page blob) into PageBlobOpenWriteOptions. Having optional params in both places ends up being a little confusing.

Moved Size to PageBlobOpenWriteOptions. I'd like to keep position a required parameter to prevent users from overwriting the data at the beginning of a Page Blob by default.

seanmcc-msft · 2020-08-08T00:22:57Z

sdk/storage/Azure.Storage.Files.Shares/src/ShareFileClient.cs

+        private async Task<Stream> OpenWriteInternal(
+            bool overwrite,
+            long position,
+            long? maxSize,


I chose to add an optional maxSize parameter, and mentioned in the xml comments it is required if overwrite is set to true, or the File is being created for the first time.

Makes sense, but I think we should still move it to the options

Fixed in next iteration.

tg-msft

This is awesome progress - I think there are just a couple of other spots we need to update the etag.

tg-msft · 2020-08-11T18:12:21Z

sdk/storage/Azure.Storage.Blobs/src/AppendBlobWriteStream.cs

+                _conditions.IfAppendPositionEqual = null;
+                _conditions.IfMaxSizeLessThanOrEqual = null;


I would expect these lines get removed now that we're splitting the concepts of OpenConditions and WriteConditions?

tg-msft · 2020-08-11T19:06:13Z

sdk/storage/Azure.Storage.Blobs/src/PageBlobClient.cs

+#pragma warning restore AZC0015 // Unexpected client method return type.
+            bool overwrite,
+            long position,
+            long? size = default,


We should move at least size and maybe position (if it can default to 0 when creating a new page blob) into PageBlobOpenWriteOptions. Having optional params in both places ends up being a little confusing.

sdk/storage/Azure.Storage.Blobs/src/PageBlobWriteStream.cs

tg-msft · 2020-08-11T19:39:43Z

sdk/storage/Azure.Storage.Blobs/src/PageBlobWriteStream.cs

+                // We need a multiple of 512 to flush.
+                if (_buffer.Length % Constants.Blob.Page.PageSizeBytes != 0)
+                {
+                    int bytesToWrite = (int)(Constants.Blob.Page.PageSizeBytes - _buffer.Length % Constants.Blob.Page.PageSizeBytes);
+                    await WriteToBufferInternal(buffer, offset, bytesToWrite, async, cancellationToken).ConfigureAwait(false);
+                    remaining -= bytesToWrite;
+                    offset += bytesToWrite;
+                }
+
+                // Flush the buffer.
+                await AppendInternal(async, cancellationToken).ConfigureAwait(false);
+
+                while (remaining > 0)
+                {
+                    await WriteToBufferInternal(
+                        buffer,
+                        offset,
+                        (int)Math.Min(remaining, _bufferSize),
+                        async,
+                        cancellationToken).ConfigureAwait(false);
+
+                    // Remaining bytes won't fit in buffer.
+                    if (remaining > _bufferSize)
+                    {
+                        await AppendInternal(async, cancellationToken).ConfigureAwait(false);
+                        remaining -= (int)_bufferSize;
+                        offset += (int)_bufferSize;
+                    }
+
+                    // Remaining bytes will fit in buffer.
+                    else
+                    {
+                        remaining = 0;
+                    }


Don't you need the same 512 alignment on the Write/Append calls in the loop?

No, because the number of bytes we are writing to the buffer is (int)Math.Min(remaining, _bufferSize),. In the case remaining > _bufferSize, we just wrote _bufferSize to the buffer, which is an increment of 512.

tg-msft · 2020-08-11T19:45:55Z

sdk/storage/Azure.Storage.Blobs/src/BlockBlobClient.cs

+
+                if (!overwrite)
+                {
+                    throw new ArgumentException($"{nameof(BlockBlobClient)}.{nameof(BlockBlobClient.OpenWrite)} only supports overwrite.");


We should include the overwrite name as another param to ArgEx. We should also maybe change the message to "only supports overwriting" to make it slightly clearer it's the specific value of the param and not just using the param with any value.

Changed to "only supports overwriting".

sdk/storage/Azure.Storage.Blobs/src/BlockBlobWriteStream.cs

tg-msft · 2020-08-11T19:49:06Z

sdk/storage/Azure.Storage.Blobs/src/BlockBlobWriteStream.cs

+                async: async,
+                cancellationToken: cancellationToken)
+                .ConfigureAwait(false);
+        }


https://docs.microsoft.com/en-us/rest/api/storageservices/put-block-list says:

You can call Put Block List to update a blob by uploading only those blocks that have changed, then committing the new and existing blocks together. You can do this by specifying whether to commit a block from the committed block list or from the uncommitted block list, or to commit the most recently uploaded version of the block, whichever list it may belong to.

sdk/storage/Azure.Storage.Common/src/Shared/StorageWriteStream.cs

tg-msft · 2020-08-11T19:56:19Z

sdk/storage/Azure.Storage.Files.Shares/src/ShareFileClient.cs

+        private async Task<Stream> OpenWriteInternal(
+            bool overwrite,
+            long position,
+            long? maxSize,


Makes sense, but I think we should still move it to the options

tg-msft · 2020-08-11T20:03:56Z

sdk/storage/Azure.Storage.Files.Shares/src/ShareFileWriteStream.cs

+                if (async)
+                {
+                    await _fileClient.UploadRangeAsync(
+                        range: httpRange,
+                        content: _buffer,
+                        progressHandler: _progressHandler,
+                        conditions: _conditions,
+                        cancellationToken: cancellationToken)
+                        .ConfigureAwait(false);
+                }
+                else
+                {
+                    _fileClient.UploadRange(
+                        range: httpRange,
+                        content: _buffer,
+                        progressHandler: _progressHandler,
+                        conditions: _conditions,
+                        cancellationToken: cancellationToken);
+                }


Don't these need to update the etag?

The Files Service doesn't support ETags.

seanmcc-msft · 2020-08-11T23:48:16Z

https://docs.microsoft.com/en-us/rest/api/storageservices/put-block-list says:

You can call Put Block List to update a blob by uploading only those blocks that have changed, then committing the new and existing blocks together. You can do this by specifying whether to commit a block from the committed block list or from the uncommitted block list, or to commit the most recently uploaded version of the block, whichever list it may belong to.

BlockBlobClient.CommitBlockList() always commits blocks in the block list as "Latest". When I was experimenting with this a few days ago, it appears that blocks not included in the block list are removed.

seanmcc-msft · 2020-08-11T23:49:18Z

It looks like we're still not pulling the updated etag after committing the block list? We should add a test with multiple "append, then flush, then append, then append some more, then flush" actions to verify that this works correctly for all blobs.

Fixed BlockBlobWriteStream to update IfMatch, and added a test for this case for all object types.

tg-msft

Looks great. Thanks Sean!

yahorsi · 2021-01-03T15:58:47Z

Guys stupid question, is this functionality released? I can't find the OpenWrite method on the BlobClient.
Please point me to the right direction

seanmcc-msft · 2021-01-04T16:32:56Z

Guys stupid question, is this functionality released? I can't find the OpenWrite method on the BlobClient.
Please point me to the right direction

It's not present on the BlobClient, it is on Azure.Storage.Blobs.Specialized.BlockBlobClient.

Open Write

a4db5c1

ghost added the Storage Storage Service (Queues, Blobs, Files) label Jul 9, 2020

seanmcc-msft marked this pull request as ready for review July 9, 2020 18:34

seanmcc-msft requested review from amishra-dev, amnguye, kasobol-msft, KrzysztofCwalina, pakrym and tg-msft as code owners July 9, 2020 18:34

seanmcc-msft mentioned this pull request Jul 13, 2020

[FEATURE REQ] Storage: Better Stream support #13392

Closed

Added unit test for PooledMemoryStream

18cd1d7

seanmcc-msft added the APIChange label Jul 15, 2020

jaschrep-msft reviewed Jul 24, 2020

View reviewed changes

seanmcc-msft added 15 commits July 26, 2020 16:48

Merge branch 'master' into feature/storage/openWrite

4250e4f

PR comments

3e1f59d

PR comments

6c33408

Added AppendBlobClient.OpenWrite() tests

4635e13

Added PageBlobClient.OpenWrite() tests

aef8481

Added ShareFileClient.OpenWrite() tests

fa69cb9

Added DataLakeFileClient.OpenWrite() tests

05c6191

Added BlockBlobClient.OpenWrite() tests

ea535d5

Ran Export-API.ps1

4187fdd

Recording tests

ea57ec0

More test recording

4affeeb

should pass CI now

8073695

Removed BlockBlobOpenWriteOptions.Overwrite

8962a4e

Merge branch 'master' into feature/storage/openWrite

24a5ed6

Hopefully fixed CI

8730556

PR comments

cde6c3b

seanmcc-msft added 2 commits August 3, 2020 15:26

Removed overwrite parameters

918355b

Added tests for StorageWriteStream

5cd8246

jaschrep-msft reviewed Aug 4, 2020

View reviewed changes

sdk/storage/Azure.Storage.Common/tests/StorageWriteStreamTests.cs Outdated Show resolved Hide resolved

PR comments

7e77c9f

kasobol-msft approved these changes Aug 4, 2020

View reviewed changes

tg-msft reviewed Aug 6, 2020

View reviewed changes

seanmcc-msft added 10 commits August 6, 2020 14:02

PR comments

f49d85b

PR comments

5196a94

Hopefully fixed CI

6a1faec

Updated Append Blob Open Write

151baed

Updated Page Blob Open Write

715ef28

Updated Block Blob Open Write

0a99f2f

Updated Data Lake File Client Open Write

240252a

Updated Share File Client Open Write

00ab151

Ran Export-API.ps1

5a74513

Recorded tests

13a2575

seanmcc-msft commented Aug 8, 2020

View reviewed changes

CI

0cee4ef

tg-msft reviewed Aug 11, 2020

View reviewed changes

PR comments

9126dc1

tg-msft approved these changes Aug 12, 2020

View reviewed changes

seanmcc-msft merged commit f0265cb into Azure:master Aug 12, 2020

seanmcc-msft deleted the feature/storage/openWrite branch August 17, 2021 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open Write #13344

Open Write #13344

seanmcc-msft commented Jul 9, 2020

seanmcc-msft commented Jul 9, 2020

PaulVrugt commented Jul 9, 2020

jaschrep-msft left a comment

seanmcc-msft commented Aug 3, 2020

tg-msft left a comment

seanmcc-msft Aug 8, 2020

tg-msft Aug 11, 2020

seanmcc-msft Aug 11, 2020 •

edited

Loading

seanmcc-msft Aug 8, 2020

tg-msft Aug 11, 2020

seanmcc-msft Aug 11, 2020

tg-msft left a comment

tg-msft Aug 11, 2020

tg-msft Aug 11, 2020

tg-msft Aug 11, 2020

seanmcc-msft Aug 11, 2020

tg-msft Aug 11, 2020

seanmcc-msft Aug 11, 2020

tg-msft Aug 11, 2020 •

edited by seanmcc-msft

Loading

tg-msft Aug 11, 2020

tg-msft Aug 11, 2020

seanmcc-msft Aug 11, 2020

seanmcc-msft commented Aug 11, 2020

seanmcc-msft commented Aug 11, 2020

tg-msft left a comment

yahorsi commented Jan 3, 2021

seanmcc-msft commented Jan 4, 2021

		_conditions.IfAppendPositionEqual = null;
		_conditions.IfMaxSizeLessThanOrEqual = null;

Open Write #13344

Open Write #13344

Conversation

seanmcc-msft commented Jul 9, 2020

seanmcc-msft commented Jul 9, 2020

PaulVrugt commented Jul 9, 2020

jaschrep-msft left a comment

Choose a reason for hiding this comment

seanmcc-msft commented Aug 3, 2020

tg-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmcc-msft Aug 11, 2020 • 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

tg-msft 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

tg-msft Aug 11, 2020 • edited by seanmcc-msft 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

seanmcc-msft commented Aug 11, 2020

seanmcc-msft commented Aug 11, 2020

tg-msft left a comment

Choose a reason for hiding this comment

yahorsi commented Jan 3, 2021

seanmcc-msft commented Jan 4, 2021

seanmcc-msft Aug 11, 2020 •

edited

Loading

tg-msft Aug 11, 2020 •

edited by seanmcc-msft

Loading