Skip to content

Added Seal Append Blob#11262

Merged
seanmcc-msft merged 7 commits intoAzure:feature/storage/stg73basefrom
seanmcc-msft:feature/storage/appendBlobSeal
Apr 15, 2020
Merged

Added Seal Append Blob#11262
seanmcc-msft merged 7 commits intoAzure:feature/storage/stg73basefrom
seanmcc-msft:feature/storage/appendBlobSeal

Conversation

@seanmcc-msft
Copy link
Copy Markdown
Member

No description provided.

{
try
{
Response<AppendBlobSealInternal> response = await BlobRestClient.AppendBlob.SealAsync(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing AppendBlobSealInternal defined in this PR. Are there any additional interesting properties outside of what's provided by BlobInfo on it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it's just ETag, LastModified, and a boolean IsSealed.

@seanmcc-msft
Copy link
Copy Markdown
Member Author

The spec was out of date, we don't need to add x-ms-blob-seal to AppendBlobClient.Append() or AppendBlobClient.AppendFromUri(), but we do need to add it to BlobBaseClient.StartCopyFromUri()

@seanmcc-msft
Copy link
Copy Markdown
Member Author

@tg-msft, I had to add a new parameter to BlobBaseClient.StartCopyFromUri().

StandardZrs = 3,
PremiumLrs = 4,
}
public partial class StartCopyFromUriOptions
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be CopyFromUriOptions? The Start prefix is only to signify that it's an LRO and not part of the operation name. I'll ping @KrzysztofCwalina to see if he has a preference and get back to you ASAP.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd vote for CopyBlobFromUriOptions, it's less generic.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Krzysztof is in favor of shorter/simpler names when we can make sure we're not going to end up with conflicts down the road. I'm not worried about adding a CopyFromUriAsync that would take different parameters so that's fine.

I think you bring up a really good point that we may possibly end up adding other copy operations in the future. How about BlobCopyFromUriOptions since other copy operations would probably be operating on blobs and the pattern we've been going with is [Operation]Options when possible or [FooClient - Client][Operation]Options? This would leave us room for (BlobContainer/Service/????)CopyFromUriOptions if ever needed.

Does that sound reasonable? Ping me if you want to chat more to hopefully unblock this faster.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think BlobCopyFromUriOptions sounds good.

public Azure.Storage.Blobs.Models.BlobRequestConditions DestinationConditions { get { throw null; } set { } }
public System.Collections.Generic.IDictionary<string, string> Metadata { get { throw null; } set { } }
public Azure.Storage.Blobs.Models.RehydratePriority? RehydratePriority { get { throw null; } set { } }
public bool? SealBlob { get { throw null; } set { } }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make this IsSealed to match the other uses of the property?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that seems reasonable.

@seanmcc-msft
Copy link
Copy Markdown
Member Author

@tg-msft, let me know the resolution for StartCopyFromUriOptions, and I'll do one more iteration.

@seanmcc-msft seanmcc-msft merged commit 0369f07 into Azure:feature/storage/stg73base Apr 15, 2020
@seanmcc-msft seanmcc-msft deleted the feature/storage/appendBlobSeal branch April 15, 2020 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants