Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update geo-redundancy code sample and readme #8

Conversation

pauljewellmsft
Copy link
Contributor

Modified folder structure in the repo to split out the v11 sample from the newly added v12 sample. Updated readme to show different paths for v11 and v12.

@pauljewellmsft
Copy link
Contributor Author

pauljewellmsft commented Sep 6, 2022

@dotnet/docs

Requesting review for code sample changes. This repo is being updated to include a sample for the .NET v12 SDK (please see changes within the v12 folder). I'm open to any and all feedback you may have. Thanks so much!

@pauljewellmsft
Copy link
Contributor Author

@tamram @normesta If either of you have a chance to review in the next few days, it would be appreciated. This is the code sample piece - I put it in a separate PR since it's a different repo than the main docs changes (not sure if that's preferred).

Copy link
Member

@IEvangelist IEvangelist left a comment

Choose a reason for hiding this comment

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

A few nits, and suggestions.

Co-authored-by: David Pine <david.pine@microsoft.com>
Copy link

@jaschrep-msft jaschrep-msft left a comment

Choose a reason for hiding this comment

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

It seems odd that an update that went out of its way to replace instances of "circuit breaker pattern" with "geo-redundancy options" doesn't actually use any of the built-in geo-redundancy options in the v12 library. While v12's built-in options are certainly different than v11's, I'm not sure why we wouldn't be guiding customers towards them, and determining how these might be augmented if we feel they are currently lacking, rather than asking customers to write all this code on their own. It would be good to sync up on the purpose of the v12 sample.

@pauljewellmsft
Copy link
Contributor Author

It seems odd that an update that went out of its way to replace instances of "circuit breaker pattern" with "geo-redundancy options" doesn't actually use any of the built-in geo-redundancy options in the v12 library. While v12's built-in options are certainly different than v11's, I'm not sure why we wouldn't be guiding customers towards them, and determining how these might be augmented if we feel they are currently lacking, rather than asking customers to write all this code on their own. It would be good to sync up on the purpose of the v12 sample.

@jaschrep-msft I'm very glad to sync up with you to discuss. I was under the impression that one of the main additions in v12 was the ability to set GeoRedundantSecondaryUri within BlobClientOptions to redirect retries to secondary storage. I included that in the sample, but if I'm missing other options I'm happy to make changes.

@jaschrep-msft
Copy link

@pauljewellmsft I must have missed it. I saw the explicit secondary uri client and didnt' see the usual options at first glance. Would still love to discuss this sample with you though to get a better idea of what we are guiding customers towards and perhaps what work the library can do for them in this regard.

Copy link

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

Thanks for tagging us @pauljewellmsft

This looks great. I only found one small nit. Let's :shipit:

@pauljewellmsft
Copy link
Contributor Author

@jaschrep-msft Sample updated and streamlined per our conversation, if you don't mind taking a look.

@pauljewellmsft pauljewellmsft merged commit 6bed171 into Azure-Samples:master Sep 12, 2022
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.

4 participants