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

Report services are not using Blob storage SDK classes anymore. #8484

Merged
merged 6 commits into from Apr 10, 2021
Merged

Conversation

agr
Copy link
Contributor

@agr agr commented Apr 2, 2021

Report services were using Storage SDK classes directly instead of our wrappers around them. To centralize blob storage access code this change makes them use ICloudBlobClient.

Duplicated code that switches between the primary and alternate source for stats was removed from individual services and moved into DI container. A registration of ICloudBlobClient keyed as BindingKeys.FeatureFlaggedStatisticsKey resolves to a blob client that points to the proper storage account. Services are supposed to request Func<ICloudBlobClient> from the DI container that will provide proper client on each individual call.

A SimpleCloudBlobExtensions class was introduced to close the gap between CloudBlockBlobReference class provided by the storage SDK and CloudBlobWrapper in our code (specifically, our wrapper lacks equivalent of DownloadTextAsync).

Additionally, report services did a .ExistAsync() checks before accessing the blob contents, causing unnecessary network round trip. More helpers were added to SimpleCloudBlobExtensions to handle missing blobs gracefully on content retrieval instead, saving one HTTP request on each report content access.

Lastly, CloudDownloadCountService was made async to accommodate for async-only ISimpleCloudBlob.

Blob storage resolution to be used by report services was moved to the DI container.
Async refresh in CloudDownloadCountService.
Copy link
Contributor

@skofman1 skofman1 left a comment

Choose a reason for hiding this comment

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

:shipit:

@@ -19,7 +19,7 @@ public CloudDownloadCountServiceRefreshJob(TimeSpan interval, CloudDownloadCount

public override Task Execute()
{
return new Task(_downloadCountService.Refresh);
return _downloadCountService.Refresh();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make sure that all async method names have an Async suffix? Or would that be a huge change?

Also, please add an await here. See: https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#prefer-asyncawait-over-directly-returning-task

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, our NuGetJobCoordinator complains if that Execute method returns a started task, so we have to wrap it in a new Task(), updated the code.

private static Task CreateWrappingContinuingTask(Task originalTask, Action<Task> continuationTask, TaskContinuationOptions options)
{
var continuation = originalTask.ContinueWith(continuationTask, options);
var wrapper = new Task(() =>
{
if (originalTask.Status == TaskStatus.WaitingForActivation || originalTask.Status == TaskStatus.Created)
{
originalTask.Start();
}
continuation.Wait();
});
return wrapper;
}
}

(line 53)

Copy link
Contributor

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

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

This is awesome!! 🥇

@agr agr merged commit 92afc80 into dev Apr 10, 2021
@agr agr deleted the agr-blobs branch April 10, 2021 00:05
@agr agr restored the agr-blobs branch April 10, 2021 00:06
@agr agr mentioned this pull request Apr 10, 2021
@agr agr mentioned this pull request Apr 17, 2021
14 tasks
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.

None yet

4 participants