georgew5656 commented on code in PR #15630:
URL: https://github.com/apache/druid/pull/15630#discussion_r1443499305
##########
extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureClientFactory.java:
##########
@@ -46,31 +48,20 @@ public AzureClientFactory(AzureAccountConfig config)
}
// It's okay to store clients in a map here because all the configs for
specifying azure retries are static, and there are only 2 of them.
- // The 2 configs are AzureAccountConfig.maxTries and
AzureOutputConfig.maxRetrr.
- // We will only ever have at most 2 clients in cachedBlobServiceClients.
- public BlobServiceClient getBlobServiceClient(Integer retryCount)
+ // The 2 configs are AzureAccountConfig.maxTries and
AzureOutputConfig.maxRetry.
+ // We will only ever have at most 2 clients in cachedBlobServiceClients per
storage account.
+ public BlobServiceClient getBlobServiceClient(@Nullable Integer retryCount,
String storageAccount)
{
- if (!cachedBlobServiceClients.containsKey(retryCount)) {
- BlobServiceClientBuilder clientBuilder =
getAuthenticatedBlobServiceClientBuilder()
- .retryOptions(new RetryOptions(
- new ExponentialBackoffOptions()
- .setMaxRetries(retryCount != null ? retryCount :
config.getMaxTries())
- .setBaseDelay(Duration.ofMillis(1000))
- .setMaxDelay(Duration.ofMillis(60000))
- ));
- cachedBlobServiceClients.put(retryCount, clientBuilder.buildClient());
- }
-
- return cachedBlobServiceClients.get(retryCount);
+ return cachedBlobServiceClients.computeIfAbsent(Pair.of(storageAccount,
retryCount != null ? retryCount : config.getMaxTries()), key ->
buildNewClient(key.rhs, key.lhs));
Review Comment:
Keeping all the clients used for ingestion around in this map feels a little
weird to me, but technically the regular AzureClientFactory will only ever use
a single storage account and the AzureIngestClientFactory's can get GC"s as
soon as the ingestion is done/only run on peons anyways.
The other option was to have the AzureClientFactory be a more
straightforward dumb factory that just creates new clients based on its auth
configs and then do the client caching in AzureStorage which is storage account
aware
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
