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

Support reading from secondary storage on retries #7622

Merged
merged 5 commits into from
Sep 21, 2019

Conversation

JoshLove-msft
Copy link
Member

@JoshLove-msft JoshLove-msft commented Sep 14, 2019

Fixes #6890

Adds a settable property on BlobClientOptions and QueueClientOptions where users can specify the GeoRedundantSecondaryUri. This should only be set if the account is configured to have Read-access geo-redundant storage (RA-GRS), i.e. accounts having geo replication without read access would not use this feature. Note Files do not support RA-GRS. If the property is set, in the event of retriable responses or exceptions for either HEAD or GET requests, we will retry against the secondary storage. If that retry fails, we will toggle back to primary storage and continue on in this way for subsequent failures for as many retries are configured in the RetryOptions.

This is different than the Track 1 C# implementation which has a property called LocationMode with the following options: PrimaryOnly (default), PrimaryThenSecondary, SecondaryOnly, SecondaryThenPrimary. What we are implementing is essentially the same as the PrimaryThenSecondary option from Track 1. SecondaryOnly and SecondaryThenPrimary are not supported in this Track 2 implementation. These two options rely on the client being able to add a custom RetryPolicy in which they add an implementation for the Evaluate method (see https://docs.microsoft.com/en-us/azure/storage/common/storage-designing-ha-apps-with-ragrs?toc=%2fazure%2fstorage%2fblobs%2ftoc.json). In this method, they could determine, based on information within RetryContext and OperationContext whether future retries should only occur against Secondary Storage (SecondaryOnly), or against Secondary and then Primary (SecondaryThenPrimary). For now, we would like to leave this level of configurability out of scope and align with the Java implementations for Track 1 and Track 2 which do the same toggling between Primary and Secondary storage that we are adding here. In the future, if we have a need for the more advanced configuration, we could expose our GeoRedundantReadPolicy class so that customers can override to have more flexibility in where retries are routed to.

@JoshLove-msft JoshLove-msft marked this pull request as ready for review September 16, 2019 19:13
@pakrym
Copy link
Contributor

pakrym commented Sep 16, 2019

Having an isolated mocked test of GeoRedundantReadPolicy and custom ResponseClassifier might be nice. Recorded tests verify that things work well together but it's hard to figure out what went wrong when individual component doesn't work correctly.

Having a separate policy test also makes sure that you call correct sync/async methods when appropriate.

Sample policy test https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/Azure.Core/tests/BufferResponsePolicyTests.cs

@JoshLove-msft
Copy link
Member Author

Having an isolated mocked test of GeoRedundantReadPolicy and custom ResponseClassifier might be nice. Recorded tests verify that things work well together but it's hard to figure out what went wrong when individual component doesn't work correctly.

Having a separate policy test also makes sure that you call correct sync/async methods when appropriate.

Sample policy test https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/Azure.Core/tests/BufferResponsePolicyTests.cs

Updated to add new tests for both classes.

@JoshLove-msft
Copy link
Member Author

/azp run net - storage - ci

Copy link
Member

@seanmcc-msft seanmcc-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 would probably be best for @tg-msft to take another look, too.

Copy link
Member

@tg-msft tg-msft left a comment

Choose a reason for hiding this comment

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

Nice! Please be sure to do one last test pass since a lot of big PRs have gone in today.

@JoshLove-msft JoshLove-msft merged commit 9edc1bc into Azure:master Sep 21, 2019
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.

Storage: Add a RetryPolicy that reads from a secondary region if the account supports it
4 participants