-
Notifications
You must be signed in to change notification settings - Fork 10
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
Fix azure bugs, expand logging #2890
Conversation
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.
Self review
|
||
public AzureApiImpl(ActorSystem system, AzureLeaseSettings settings) | ||
{ | ||
_settings = settings; | ||
_log = Logging.GetLogger(system, GetType()); | ||
|
||
_containerClient = new Lazy<BlobContainerClient>(() => |
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.
Actual fix, removed Lazy<T>
because it is caching container client value.
private async Task<BlobContainerClient> ContainerClient() | ||
{ | ||
var serviceClient = _settings.AzureCredential != null && _settings.ServiceEndpoint != null | ||
? new BlobServiceClient(_settings.ServiceEndpoint, _settings.AzureCredential, _settings.BlobClientOptions) | ||
: new BlobServiceClient(_settings.ConnectionString); | ||
|
||
var client = serviceClient.GetBlobContainerClient(_settings.ContainerName); | ||
|
||
// Make sure that `CreateIfNotExistsAsync()` only get called once for every AzureApi instance | ||
if (!_initialized) | ||
{ | ||
await client.CreateIfNotExistsAsync(); | ||
_initialized = true; | ||
} | ||
|
||
return client; | ||
} |
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.
Actual fix, instead of using Lazy<T>
, make sure that a new container client is instantiated every time a container client is requested, but only try to initialize the azure container once for every instance of AzureApiImpl
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
Related to akkadotnet/akka.net#7379
Changes
LeaseActor
from/user
to/system
Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):