smiroslav commented on code in PR #1748: URL: https://github.com/apache/jackrabbit-oak/pull/1748#discussion_r1837831232
########## oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/v8/AzureSegmentStoreServiceV8.java: ########## @@ -0,0 +1,155 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.jackrabbit.oak.segment.azure.v8; + +import com.azure.storage.blob.BlobContainerClient; +import com.azure.storage.blob.BlobContainerClientBuilder; +import com.microsoft.azure.storage.CloudStorageAccount; +import com.microsoft.azure.storage.LocationMode; +import com.microsoft.azure.storage.StorageCredentials; +import com.microsoft.azure.storage.StorageException; +import com.microsoft.azure.storage.blob.BlobRequestOptions; +import com.microsoft.azure.storage.blob.CloudBlobClient; +import com.microsoft.azure.storage.blob.CloudBlobContainer; +import org.apache.commons.lang3.StringUtils; +import org.apache.jackrabbit.oak.segment.azure.Configuration; +import org.jetbrains.annotations.NotNull; +import org.osgi.framework.ServiceRegistration; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.ConfigurationPolicy; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.net.URISyntaxException; +import java.security.InvalidKeyException; + +@Component( Review Comment: It seems this class is not used as an OSGi component, and annotation can be removed. ########## oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/v8/AzureSegmentStoreServiceV8.java: ########## @@ -0,0 +1,155 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.jackrabbit.oak.segment.azure.v8; + +import com.azure.storage.blob.BlobContainerClient; +import com.azure.storage.blob.BlobContainerClientBuilder; +import com.microsoft.azure.storage.CloudStorageAccount; +import com.microsoft.azure.storage.LocationMode; +import com.microsoft.azure.storage.StorageCredentials; +import com.microsoft.azure.storage.StorageException; +import com.microsoft.azure.storage.blob.BlobRequestOptions; +import com.microsoft.azure.storage.blob.CloudBlobClient; +import com.microsoft.azure.storage.blob.CloudBlobContainer; +import org.apache.commons.lang3.StringUtils; +import org.apache.jackrabbit.oak.segment.azure.Configuration; +import org.jetbrains.annotations.NotNull; +import org.osgi.framework.ServiceRegistration; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.ConfigurationPolicy; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.net.URISyntaxException; +import java.security.InvalidKeyException; + +@Component( + configurationPolicy = ConfigurationPolicy.REQUIRE, + configurationPid = {Configuration.PID}) +public class AzureSegmentStoreServiceV8 { + + private static final Logger log = LoggerFactory.getLogger(AzureSegmentStoreServiceV8.class); + + public static final String DEFAULT_CONTAINER_NAME = "oak"; + + public static final String DEFAULT_ROOT_PATH = "/oak"; Review Comment: unused constant ########## oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzureSegmentStoreService.java: ########## @@ -57,21 +48,35 @@ public class AzureSegmentStoreService { public static final String DEFAULT_ROOT_PATH = "/oak"; public static final boolean DEFAULT_ENABLE_SECONDARY_LOCATION = false; - public static final String DEFAULT_ENDPOINT_SUFFIX = "core.windows.net"; private ServiceRegistration registration; - private static AzureStorageCredentialManager azureStorageCredentialManager; + + private final boolean useAzureSdkV12 = Boolean.getBoolean("segment.azure.v12.enabled"); + @Activate public void activate(ComponentContext context, Configuration config) throws IOException { - AzurePersistence persistence = createAzurePersistenceFrom(config); - registration = context.getBundleContext() - .registerService(SegmentNodeStorePersistence.class, persistence, new Hashtable<String, Object>() {{ - put(SERVICE_PID, String.format("%s(%s, %s)", AzurePersistence.class.getName(), config.accountName(), config.rootPath())); - if (!Objects.equals(config.role(), "")) { - put("role", config.role()); - } - }}); + if (useAzureSdkV12) { + log.info("Starting nodestore using Azure SDK 12"); Review Comment: ```suggestion log.info("Starting node store using Azure SDK 12"); ``` ########## oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/v8/AzureSegmentStoreServiceV8.java: ########## @@ -0,0 +1,155 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.jackrabbit.oak.segment.azure.v8; + +import com.azure.storage.blob.BlobContainerClient; +import com.azure.storage.blob.BlobContainerClientBuilder; +import com.microsoft.azure.storage.CloudStorageAccount; +import com.microsoft.azure.storage.LocationMode; +import com.microsoft.azure.storage.StorageCredentials; +import com.microsoft.azure.storage.StorageException; +import com.microsoft.azure.storage.blob.BlobRequestOptions; +import com.microsoft.azure.storage.blob.CloudBlobClient; +import com.microsoft.azure.storage.blob.CloudBlobContainer; +import org.apache.commons.lang3.StringUtils; +import org.apache.jackrabbit.oak.segment.azure.Configuration; +import org.jetbrains.annotations.NotNull; +import org.osgi.framework.ServiceRegistration; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.ConfigurationPolicy; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.net.URISyntaxException; +import java.security.InvalidKeyException; + +@Component( + configurationPolicy = ConfigurationPolicy.REQUIRE, + configurationPid = {Configuration.PID}) +public class AzureSegmentStoreServiceV8 { Review Comment: I would remove ```Service``` from the class name ########## oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzureRepositoryLock.java: ########## @@ -144,14 +142,14 @@ private void refreshLease() { writeAccessController.disableWriting(); } - if (e instanceof StorageException) { - StorageException storageException = (StorageException) e; - if (Set.of(StorageErrorCodeStrings.OPERATION_TIMED_OUT - , StorageErrorCode.SERVICE_INTERNAL_ERROR - , StorageErrorCodeStrings.SERVER_BUSY - , StorageErrorCodeStrings.INTERNAL_ERROR).contains(storageException.getErrorCode())) { + if (e instanceof BlobStorageException) { + BlobStorageException storageException = (BlobStorageException) e; + if (Set.of(BlobErrorCode.OPERATION_TIMED_OUT, + BlobErrorCode.SERVER_BUSY, + BlobErrorCode.INTERNAL_ERROR).contains(storageException.getErrorCode())) { log.warn("Could not renew the lease due to the operation timeout or service unavailability. Retry in progress ...", e); - } else if (storageException.getHttpStatusCode() == Constants.HeaderConstants.HTTP_UNUSED_306) { + //TODO: ierandra Review Comment: TODO to be addressed ########## oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/tool/AzureCheck.java: ########## @@ -18,8 +18,8 @@ import com.google.common.io.Files; import com.microsoft.azure.storage.blob.CloudBlobDirectory; -import org.apache.jackrabbit.oak.segment.azure.AzurePersistence; -import org.apache.jackrabbit.oak.segment.azure.AzureStorageCredentialManager; +import org.apache.jackrabbit.oak.segment.azure.v8.AzurePersistenceV8; Review Comment: Do we have this class also upgraded to Azure SDK v12 somewhere? ########## oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/v8/AzureSegmentStoreServiceV8.java: ########## @@ -0,0 +1,155 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.jackrabbit.oak.segment.azure.v8; + +import com.azure.storage.blob.BlobContainerClient; +import com.azure.storage.blob.BlobContainerClientBuilder; +import com.microsoft.azure.storage.CloudStorageAccount; +import com.microsoft.azure.storage.LocationMode; +import com.microsoft.azure.storage.StorageCredentials; +import com.microsoft.azure.storage.StorageException; +import com.microsoft.azure.storage.blob.BlobRequestOptions; +import com.microsoft.azure.storage.blob.CloudBlobClient; +import com.microsoft.azure.storage.blob.CloudBlobContainer; +import org.apache.commons.lang3.StringUtils; +import org.apache.jackrabbit.oak.segment.azure.Configuration; +import org.jetbrains.annotations.NotNull; +import org.osgi.framework.ServiceRegistration; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.ConfigurationPolicy; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.net.URISyntaxException; +import java.security.InvalidKeyException; + +@Component( + configurationPolicy = ConfigurationPolicy.REQUIRE, + configurationPid = {Configuration.PID}) +public class AzureSegmentStoreServiceV8 { + + private static final Logger log = LoggerFactory.getLogger(AzureSegmentStoreServiceV8.class); + + public static final String DEFAULT_CONTAINER_NAME = "oak"; + + public static final String DEFAULT_ROOT_PATH = "/oak"; + + public static final boolean DEFAULT_ENABLE_SECONDARY_LOCATION = false; Review Comment: unused constant ########## oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/v8/AzureSegmentStoreServiceV8.java: ########## @@ -0,0 +1,155 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.jackrabbit.oak.segment.azure.v8; + +import com.azure.storage.blob.BlobContainerClient; +import com.azure.storage.blob.BlobContainerClientBuilder; +import com.microsoft.azure.storage.CloudStorageAccount; +import com.microsoft.azure.storage.LocationMode; +import com.microsoft.azure.storage.StorageCredentials; +import com.microsoft.azure.storage.StorageException; +import com.microsoft.azure.storage.blob.BlobRequestOptions; +import com.microsoft.azure.storage.blob.CloudBlobClient; +import com.microsoft.azure.storage.blob.CloudBlobContainer; +import org.apache.commons.lang3.StringUtils; +import org.apache.jackrabbit.oak.segment.azure.Configuration; +import org.jetbrains.annotations.NotNull; +import org.osgi.framework.ServiceRegistration; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.ConfigurationPolicy; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.net.URISyntaxException; +import java.security.InvalidKeyException; + +@Component( + configurationPolicy = ConfigurationPolicy.REQUIRE, + configurationPid = {Configuration.PID}) +public class AzureSegmentStoreServiceV8 { + + private static final Logger log = LoggerFactory.getLogger(AzureSegmentStoreServiceV8.class); + + public static final String DEFAULT_CONTAINER_NAME = "oak"; + + public static final String DEFAULT_ROOT_PATH = "/oak"; + + public static final boolean DEFAULT_ENABLE_SECONDARY_LOCATION = false; + public static final String DEFAULT_ENDPOINT_SUFFIX = "core.windows.net"; + + private ServiceRegistration registration; Review Comment: unused property ########## oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzureSegmentStoreService.java: ########## @@ -57,21 +48,35 @@ public class AzureSegmentStoreService { public static final String DEFAULT_ROOT_PATH = "/oak"; public static final boolean DEFAULT_ENABLE_SECONDARY_LOCATION = false; - public static final String DEFAULT_ENDPOINT_SUFFIX = "core.windows.net"; private ServiceRegistration registration; - private static AzureStorageCredentialManager azureStorageCredentialManager; + + private final boolean useAzureSdkV12 = Boolean.getBoolean("segment.azure.v12.enabled"); + @Activate public void activate(ComponentContext context, Configuration config) throws IOException { - AzurePersistence persistence = createAzurePersistenceFrom(config); - registration = context.getBundleContext() - .registerService(SegmentNodeStorePersistence.class, persistence, new Hashtable<String, Object>() {{ - put(SERVICE_PID, String.format("%s(%s, %s)", AzurePersistence.class.getName(), config.accountName(), config.rootPath())); - if (!Objects.equals(config.role(), "")) { - put("role", config.role()); - } - }}); + if (useAzureSdkV12) { + log.info("Starting nodestore using Azure SDK 12"); + AzurePersistence persistence = AzurePersistenceManager.createAzurePersistenceFrom(config); + registration = context.getBundleContext() + .registerService(SegmentNodeStorePersistence.class, persistence, new Hashtable<String, Object>() {{ + put(SERVICE_PID, String.format("%s(%s, %s)", AzurePersistence.class.getName(), config.accountName(), config.rootPath())); + if (!Objects.equals(config.role(), "")) { + put("role", config.role()); + } + }}); + } else { + log.info("Starting nodestore using Azure SDK 8"); Review Comment: ```suggestion log.info("Starting node store using Azure SDK 8"); ``` ########## oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/v8/AzureSegmentStoreServiceV8.java: ########## @@ -0,0 +1,155 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.jackrabbit.oak.segment.azure.v8; + +import com.azure.storage.blob.BlobContainerClient; +import com.azure.storage.blob.BlobContainerClientBuilder; +import com.microsoft.azure.storage.CloudStorageAccount; +import com.microsoft.azure.storage.LocationMode; +import com.microsoft.azure.storage.StorageCredentials; +import com.microsoft.azure.storage.StorageException; +import com.microsoft.azure.storage.blob.BlobRequestOptions; +import com.microsoft.azure.storage.blob.CloudBlobClient; +import com.microsoft.azure.storage.blob.CloudBlobContainer; +import org.apache.commons.lang3.StringUtils; +import org.apache.jackrabbit.oak.segment.azure.Configuration; +import org.jetbrains.annotations.NotNull; +import org.osgi.framework.ServiceRegistration; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.ConfigurationPolicy; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.net.URISyntaxException; +import java.security.InvalidKeyException; + +@Component( + configurationPolicy = ConfigurationPolicy.REQUIRE, + configurationPid = {Configuration.PID}) +public class AzureSegmentStoreServiceV8 { + + private static final Logger log = LoggerFactory.getLogger(AzureSegmentStoreServiceV8.class); + + public static final String DEFAULT_CONTAINER_NAME = "oak"; Review Comment: unused constant ########## oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzurePersistenceManager.java: ########## @@ -0,0 +1,199 @@ +package org.apache.jackrabbit.oak.segment.azure; Review Comment: The license header is missing. ########## oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzurePersistenceManager.java: ########## @@ -0,0 +1,199 @@ +package org.apache.jackrabbit.oak.segment.azure; + +import com.azure.identity.ClientSecretCredential; +import com.azure.identity.ClientSecretCredentialBuilder; +import com.azure.storage.blob.BlobContainerClient; +import com.azure.storage.blob.BlobServiceClient; +import com.azure.storage.blob.BlobServiceClientBuilder; +import com.azure.storage.blob.models.BlobStorageException; +import com.azure.storage.common.policy.RequestRetryOptions; +import org.apache.commons.lang3.StringUtils; +import org.apache.jackrabbit.oak.segment.azure.util.AzureRequestOptions; +import org.apache.jackrabbit.oak.segment.azure.util.Environment; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; + +import static org.apache.jackrabbit.oak.segment.azure.AzureUtilities.*; + +public class AzurePersistenceManager { + + private static final Logger log = LoggerFactory.getLogger(AzurePersistenceManager.class); + + private AzurePersistenceManager() { + } + + public static AzurePersistence createAzurePersistenceFrom(@NotNull String accountName, @NotNull String containerName, @NotNull String rootPrefix, @NotNull Environment environment) throws IOException { Review Comment: unused method ########## oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzureRepositoryLock.java: ########## @@ -128,11 +130,7 @@ private void refreshLease() { long timeSinceLastUpdate = (System.currentTimeMillis() - lastUpdate) / 1000; try { if (timeSinceLastUpdate > renewalInterval) { - - BlobRequestOptions requestOptions = new BlobRequestOptions(); - requestOptions.setMaximumExecutionTimeInMs(LEASE_RENEWAL_TIMEOUT_MS); - requestOptions.setRetryPolicyFactory(new RetryNoRetry()); - blob.renewLease(AccessCondition.generateLeaseCondition(leaseId), requestOptions, null); + leaseId = leaseClient.renewLeaseWithResponse((RequestConditions) null, Duration.ofMillis(LEASE_RENEWAL_TIMEOUT_MS), Context.NONE).getValue(); Review Comment: The previous implementation configured request options with ```RetryNoRetry```. How is it achieved here? ########## oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzureHttpRequestLoggingPolicy.java: ########## @@ -0,0 +1,50 @@ +package org.apache.jackrabbit.oak.segment.azure; Review Comment: The license header is missing -- 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]
