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
[Lease.Azure] Async task and options code cleanup #1256
[Lease.Azure] Async task and options code cleanup #1256
Conversation
builder.AddHocon(sb.ToString(), HoconAddMode.Prepend); | ||
|
||
if(AzureCredential is null && ServiceEndpoint is null) | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code cleanup, shortcut Apply
to not inject a Setup
class if there is no need for it
var setup = new AzureLeaseSetup | ||
{ | ||
AzureCredential = AzureCredential, | ||
ServiceEndpoint = ServiceEndpoint, | ||
BlobClientOptions = BlobClientOptions, | ||
ApiServiceRequestTimeout = ApiServiceRequestTimeout, | ||
ConnectionString = ConnectionString, | ||
ContainerName = ContainerName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code cleanup, remove non-needed information from Setup
class
@@ -167,6 +167,7 @@ public async Task<LeaseResource> ReadOrCreateLeaseResource(string name) | |||
{ | |||
switch ((HttpStatusCode)e.Status) | |||
{ | |||
case HttpStatusCode.PreconditionFailed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug fix, we did not use any pre-condition in our code but the driver implicitly added a pre-condition internally, need to capture this exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -167,6 +167,7 @@ public async Task<LeaseResource> ReadOrCreateLeaseResource(string name) | |||
{ | |||
switch ((HttpStatusCode)e.Status) | |||
{ | |||
case HttpStatusCode.PreconditionFailed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch
Possible fix for #1251
Still unsure that this is the actual cause of the failure, problem was not reproduced.