Azure Blob Cache and Test refactor. #112
Conversation
Codecov Report
@@ Coverage Diff @@
## master #112 +/- ##
==========================================
+ Coverage 82.67% 84.01% +1.33%
==========================================
Files 44 47 +3
Lines 1068 1126 +58
Branches 157 161 +4
==========================================
+ Hits 883 946 +63
+ Misses 137 128 -9
- Partials 48 52 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
JimBobSquarePants
changed the title
| AzureBlobStorageCacheOptions options = cacheOptions.Value; | ||
|
|
||
| this.container = new BlobContainerClient(options.ConnectionString, options.ContainerName); | ||
| this.container.CreateIfNotExistsAsync(PublicAccessType.None); |
There was a problem hiding this comment.
should we really be creating this if it doesn't exists? or should we require application to do this during setup, I see this the same as EF etc I wouldn't expect my ORM to create the database for me unless I configured it so. We should however error if it doesn't exist.
Also shouldn't really be doing potentially expensive operations inside the constructor. There should probably be a private BlobContainerClient GetClientAsync() that is called to get the client or set it up if it hasn't yet been instantiated.
There was a problem hiding this comment.
We could make it optional I suppose. This changes the behavior compared to the physical cache though and our own default behavior of creating directories when we save to a destination path.
That constructor is only called once, the cache is registered as a singleton.
There was a problem hiding this comment.
I feel file system is different you aren't dealing with potentially expensive operations on a remote server, you are dealing with local resources and can assume that is fast.
I think my main issue is that we really shouldn't be being called async/remote APIs during construction.
Even having a static async factory method would be fine its just that network call in the middle of the constructor that bothers me the most.
There was a problem hiding this comment.
I'll move it to a factory then attached to the cache and allow passing the view enum as well then.
Prerequisites
Description
This has grown arms and legs. Started off as a simple Azure Blob Storage cache implementation turned into a realization that we're loading up the cache stream for 304s and that all our integration tests are super janky once the perf jumped. (We were generating multiple server instances that were hitting the same cache file).
No breaking changes thankfully, old cache files containing no content length will simply be reprocessed and cached.