saxenapranav commented on code in PR #6846: URL: https://github.com/apache/hadoop/pull/6846#discussion_r1617193731
########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/AbfsServiceType.java: ########## @@ -0,0 +1,31 @@ +/** + * 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.hadoop.fs.azurebfs.constants; + +/** + * Azure Storage Offers two sets of Rest APIs for interacting with the storage account. + * <ol> + * <li>Blob Rest API: <a href = https://learn.microsoft.com/en-us/rest/api/storageservices/blob-service-rest-api></a></li> + * <li>Data Lake Rest API: <a href = https://learn.microsoft.com/en-us/rest/api/storageservices/datalakestoragegen2/operation-groups></a></li> + * </ol> + */ +public enum AbfsServiceType { + DFS, + BLOB Review Comment: lets have field as endpointDnsSuffix, which would have something like .dfs.core.windows.net or .blob.core.windows.net. Reason being, some account can have word 'blob' or 'dfs' as part of the accountName. Also, can it happen that fsUri have some other suffix also (https://learn.microsoft.com/en-us/rest/api/storageservices/datalakestoragegen2/filesystem/create?view=rest-storageservices-datalakestoragegen2-2019-12-12 doesnt mention any particular suffix) ? ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/contracts/services/AppendRequestParameters.java: ########## @@ -52,6 +57,31 @@ public AppendRequestParameters(final long position, this.leaseId = leaseId; this.isExpectHeaderEnabled = isExpectHeaderEnabled; this.isRetryDueToExpect = false; + this.blockId = null; + this.eTag = null; + } + + // Constructor to be used for interacting with AbfsBlobClient + @SuppressWarnings("checkstyle:ParameterNumber") Review Comment: we can add this rule in checkstyle-suppressions.xml ########## hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsBlobClient.java: ########## @@ -0,0 +1,151 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.hadoop.fs.azurebfs; + +import java.net.URI; +import java.util.UUID; + +import org.assertj.core.api.Assertions; +import org.junit.Assume; +import org.junit.Test; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.azurebfs.constants.FSOperationType; +import org.apache.hadoop.fs.azurebfs.constants.FileSystemUriSchemes; +import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AzureBlobFileSystemException; +import org.apache.hadoop.fs.azurebfs.services.AbfsBlobClient; +import org.apache.hadoop.fs.azurebfs.services.AbfsClient; +import org.apache.hadoop.fs.azurebfs.services.AuthType; +import org.apache.hadoop.fs.azurebfs.utils.Base64; +import org.apache.hadoop.fs.azurebfs.utils.TracingContext; +import org.apache.hadoop.fs.azurebfs.utils.TracingHeaderFormat; + +import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY; +import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.AZURE_CREATE_REMOTE_FILESYSTEM_DURING_INITIALIZATION; +import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_ACCOUNT_AUTH_TYPE_PROPERTY_NAME; +import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_APPEND_BLOB_KEY; +import static org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.FS_AZURE_ABFS_ACCOUNT_NAME; +import static org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.FS_AZURE_ACCOUNT_NAME; +import static org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.FS_AZURE_CONTRACT_TEST_URI; +import static org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.FS_AZURE_TEST_APPENDBLOB_ENABLED; +import static org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.FS_AZURE_TEST_NAMESPACE_ENABLED_ACCOUNT; +import static org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.TEST_CONFIGURATION_FILE_NAME; +import static org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.TEST_CONTAINER_PREFIX; + +/** + * Test class to test AbfsBlobClient APIs. + * Todo: [FnsOverBlob] - Add more tests to cover all APIs once they are ready + */ +public class ITestAbfsBlobClient { + + @Test + public void testAbfsBlobClient() throws Exception { + try (AzureBlobFileSystem fs = getBlobFileSystem()) { + AbfsClient client = fs.getAbfsStore().getClient(); + Assertions.assertThat(client).isInstanceOf(AbfsBlobClient.class); + // Make sure all client.REST_API_CALLS succeed with right parameters + testClientAPIs(client, getTestTracingContext(fs)); + } catch (AzureBlobFileSystemException ex) { + // Todo: [FnsOverBlob] - Remove this block once all Blob Endpoint Support is ready. + Assertions.assertThat(ex.getMessage()).contains("Blob Endpoint Support is not yet implemented"); + } + } + + private void testClientAPIs(AbfsClient client, TracingContext tracingContext) throws Exception { + // 1. Set File System Properties + String val1 = Base64.encode("value1".getBytes()); + String val2 = Base64.encode("value2".getBytes()); + String properties = "key1=" + val1 + ",key2=" + val2; + client.setFilesystemProperties(properties, tracingContext); + + // 2. Get File System Properties + client.getFilesystemProperties(tracingContext); + + // 3. Create Path + client.createPath("/test", true, true, null, false, null, null, tracingContext); + client.createPath("/dir", false, true, null, false, null, null, tracingContext); + client.createPath("/dir/test", true, true, null, false, null, null, tracingContext); + + // 4. List Path + client.listPath("/", false, 5, null, tracingContext); + + // 5. Acquire lease + client.acquireLease("/dir/test", 5, tracingContext); + + // 6. Set Path Properties + client.setPathProperties("/test", properties, tracingContext, null); + + // 7. Get Path Status + client.getPathStatus("/test", true, tracingContext, null); + + // N. Delete File System + client.deleteFilesystem(tracingContext); + } + + private AzureBlobFileSystem getBlobFileSystem() throws Exception { + Configuration rawConfig = new Configuration(); + rawConfig.addResource(TEST_CONFIGURATION_FILE_NAME); + + String fileSystemName = TEST_CONTAINER_PREFIX + UUID.randomUUID().toString(); + String accountName = rawConfig.get(FS_AZURE_ACCOUNT_NAME, ""); + if (accountName.isEmpty()) { + // check if accountName is set using different config key + accountName = rawConfig.get(FS_AZURE_ABFS_ACCOUNT_NAME, ""); + } + Assume.assumeFalse("Skipping test as account name is not provided", accountName.isEmpty()); + + Assume.assumeFalse("Blob Endpoint Works only with FNS Accounts", + rawConfig.getBoolean(FS_AZURE_TEST_NAMESPACE_ENABLED_ACCOUNT, true)); + accountName = setBlobEndpoint(accountName); + + AbfsConfiguration abfsConfig = new AbfsConfiguration(rawConfig, accountName); + AuthType authType = abfsConfig.getEnum(FS_AZURE_ACCOUNT_AUTH_TYPE_PROPERTY_NAME, AuthType.SharedKey); + String abfsScheme = authType == AuthType.SharedKey ? FileSystemUriSchemes.ABFS_SCHEME + : FileSystemUriSchemes.ABFS_SECURE_SCHEME; + final String abfsUrl = fileSystemName + "@" + accountName; + URI defaultUri = null; + + try { + defaultUri = new URI(abfsScheme, abfsUrl, null, null, null); + } catch (Exception ex) { + throw new AssertionError(ex); + } + + String testUrl = defaultUri.toString(); + abfsConfig.set(FS_DEFAULT_NAME_KEY, defaultUri.toString()); Review Comment: We can inherit `AbstractAbfsIntegrationTest`, which would give us all the boilerplate for running the test. What we can do, is get the configuration form the getFileSystem().getConf(); then, make changes to the config, and then return back filesystem created with FileSystem.newInstance().. What you feel? ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClientHandler.java: ########## @@ -0,0 +1,68 @@ +/** + * 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.hadoop.fs.azurebfs.services; + +import org.apache.hadoop.fs.azurebfs.constants.AbfsServiceType; +import org.apache.hadoop.util.Preconditions; + +/** + * AbfsClientHandler is a class that provides a way to get the AbfsClient + * based on the service type. + */ +public class AbfsClientHandler { + private AbfsServiceType defaultServiceType; + private AbfsClient dfsAbfsClient; + private AbfsClient blobAbfsClient; Review Comment: lets have them final. ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClientHandler.java: ########## @@ -0,0 +1,68 @@ +/** + * 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.hadoop.fs.azurebfs.services; + +import org.apache.hadoop.fs.azurebfs.constants.AbfsServiceType; +import org.apache.hadoop.util.Preconditions; + +/** + * AbfsClientHandler is a class that provides a way to get the AbfsClient + * based on the service type. + */ +public class AbfsClientHandler { + private AbfsServiceType defaultServiceType; + private AbfsClient dfsAbfsClient; + private AbfsClient blobAbfsClient; + + public AbfsClientHandler(AbfsServiceType defaultServiceType, + AbfsClient dfsAbfsClient, AbfsClient blobAbfsClient) { + this.blobAbfsClient = blobAbfsClient; + this.dfsAbfsClient = dfsAbfsClient; + this.defaultServiceType = defaultServiceType; + } + + public AbfsClient getClient() { + if (defaultServiceType == AbfsServiceType.DFS) { + Preconditions.checkNotNull(dfsAbfsClient, "DFS client is not initialized"); Review Comment: what if we remove these preconditions, but add them in the constructor. As if this method gets called multiple times, prechecks would have multiple times which is not required. ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java: ########## @@ -170,6 +176,8 @@ public class AzureBlobFileSystemStore implements Closeable, ListingSupport { private static final Logger LOG = LoggerFactory.getLogger(AzureBlobFileSystemStore.class); private AbfsClient client; + private AbfsClientHandler clientHandler; + private AbfsServiceType defaultServiceType; Review Comment: Can this be made final. ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/contracts/exceptions/UnsupportedAbfsOperationException.java: ########## @@ -0,0 +1,31 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.hadoop.fs.azurebfs.contracts.exceptions; + +/** + * Abfs Exception to be thrown when operation is not supported. + */ +public class UnsupportedAbfsOperationException extends AzureBlobFileSystemException { Review Comment: What if PathIOException own this behavior. What you say? -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
