OAK-12219-Upgrade Azure SDK V8 to V12 for oak-blob-azure - rework (#2…#2982
OAK-12219-Upgrade Azure SDK V8 to V12 for oak-blob-azure - rework (#2…#2982reschke wants to merge 1 commit into
Conversation
Commit-Check ❌ |
| */ | ||
| static boolean getUseV12Value(Map<String, Object> config) { | ||
| if (System.getProperty(JVM_PROPERTY_V12_ENABLED) != null) { | ||
| boolean useV12 = Boolean.getBoolean(JVM_PROPERTY_V12_ENABLED); |
There was a problem hiding this comment.
use SystemPropertyProvider
|
Q: the default is still V8? |
|
joerghoh
left a comment
joerghoh
left a comment
There was a problem hiding this comment.
a series of questions and suggestions. Nothing really critical, but worth to review.
| if (StringUtils.isNotBlank(customBlobEndpoint)) { | ||
| // Use custom endpoint (e.g., for private endpoints) | ||
| // Ensure it starts with https:// if not already present | ||
| if (!customBlobEndpoint.startsWith("http://") && !customBlobEndpoint.startsWith("https://")) { |
There was a problem hiding this comment.
Wouldn't this allow also a http (non-encypted) url to be created?
| .retryOptions(retryOptions) | ||
| .addPolicy(loggingPolicy); | ||
|
|
||
| HttpClient httpClient = new NettyAsyncHttpClientBuilder() |
There was a problem hiding this comment.
What is the overhead of creating this httpClient? Is this httpClient designed to be reused (like the Apache HttpClient?)
| String proxyHost = properties.getProperty(AzureConstantsV12.PROXY_HOST); | ||
| String proxyPort = properties.getProperty(AzureConstantsV12.PROXY_PORT); | ||
|
|
||
| if (!(Objects.toString(proxyHost, "").isEmpty() || Objects.toString(proxyPort, "").isEmpty())) { |
There was a problem hiding this comment.
I think that the logic is wrong; it should be AND instead of OR.
|
|
||
| private static final Logger log = LoggerFactory.getLogger(AzureDataStoreWrapper.class); | ||
|
|
||
| public static final String NAME = "org.apache.jackrabbit.oak.plugins.blob.datastore.AzureDataStore"; |
There was a problem hiding this comment.
Is there any special reason not to use the full qualified class name?
(just found the answer to this in a comment below; you should move that comment here.)
| private StatisticsProvider statisticsProvider; | ||
| private ServiceRegistration<?> delegateReg; | ||
|
|
||
| static ServiceRegistration<?> registerService(ComponentContext context, AbstractSharedCachingDataStore service) { |
There was a problem hiding this comment.
The method name is a bit too generic.
| static ServiceRegistration<?> registerService(ComponentContext context, AbstractSharedCachingDataStore service) { | |
| static ServiceRegistration<?> registerDataStoreService(ComponentContext context, AbstractSharedCachingDataStore service) { |
| log.info("Azure SDK v12 flag: OSGi config {}={}", OSGI_CONFIG_V12_ENABLED, useV12); | ||
| return useV12; | ||
| } | ||
| log.info("Azure SDK v12 flag: not configured, using default (false)"); |
There was a problem hiding this comment.
| log.info("Azure SDK v12 flag: not configured, using default (false)"); | |
| log.info("Azure SDK v12 flag: not configured, falling back to v8 "); |
|
|
||
| @Override | ||
| protected String[] getDescription() { | ||
| return new String[]{"type=AzureBlob"}; |
There was a problem hiding this comment.
This is used in the context of the mbeans, is it visible there as well, what SDK version is active?
3 participants
…941)
Ai-Assisted-By: claude
EDIT: please ignore the osgi-it changes, I started with the wrong commit.
EDIT2: who can review this from an OSGi point of view?