From 12161d1616be217a5f7877925583299ec7314f7b Mon Sep 17 00:00:00 2001 From: Sneha Vijayarajan Date: Wed, 26 Aug 2020 00:31:35 +0530 Subject: [PATCH 01/11] Disable default create with overwrite=true --- .../hadoop/fs/azurebfs/AbfsConfiguration.java | 6 ++ .../azurebfs/constants/ConfigurationKeys.java | 4 + .../constants/FileSystemConfigurations.java | 1 + .../fs/azurebfs/services/AbfsClient.java | 70 ++++++++++++--- .../ITestAzureBlobFileSystemCreate.java | 85 +++++++++++++++++++ .../ITestAzureBlobFileSystemMkDir.java | 56 ++++++++++++ 6 files changed, 208 insertions(+), 14 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java index 66d485317c9ba..a5216eb958ce7 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java @@ -181,6 +181,10 @@ public class AbfsConfiguration{ DefaultValue = DEFAULT_FS_AZURE_ATOMIC_RENAME_DIRECTORIES) private String azureAtomicDirs; + @BooleanConfigurationValidatorAnnotation(ConfigurationKey = FS_AZURE_DISABLE_DEFAULT_CREATE_OVERWRITE, + DefaultValue = DEFAULT_FS_AZURE_DISABLE_DEFAULT_CREATE_OVERWRITE) + private boolean disableDefaultCreateOverwrite; + @StringConfigurationValidatorAnnotation(ConfigurationKey = FS_AZURE_APPEND_BLOB_KEY, DefaultValue = DEFAULT_FS_AZURE_APPEND_BLOB_DIRECTORIES) private String azureAppendBlobDirs; @@ -573,6 +577,8 @@ public String getAzureAtomicRenameDirs() { return this.azureAtomicDirs; } + public boolean isDefaultCreateOverwriteDisabled() { return this.disableDefaultCreateOverwrite; } + public String getAppendBlobDirs() { return this.azureAppendBlobDirs; } diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java index 681390c019873..a28e0579cf2b1 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java @@ -67,6 +67,10 @@ public final class ConfigurationKeys { public static final String FS_AZURE_ENABLE_AUTOTHROTTLING = "fs.azure.enable.autothrottling"; public static final String FS_AZURE_ALWAYS_USE_HTTPS = "fs.azure.always.use.https"; public static final String FS_AZURE_ATOMIC_RENAME_KEY = "fs.azure.atomic.rename.key"; + /** HDFS FS defaults overwrite behaviour to true which leads to race conditions at backend with parallel operations + * issued to same path. This config provides a means to override the default overwrite flag. + */ + public static final String FS_AZURE_DISABLE_DEFAULT_CREATE_OVERWRITE = "fs.azure.disable.default.create.overwrite"; /** Provides a config to provide comma separated path prefixes on which Appendblob based files are created * Default is empty. **/ public static final String FS_AZURE_APPEND_BLOB_KEY = "fs.azure.appendblob.directories"; diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/FileSystemConfigurations.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/FileSystemConfigurations.java index f70d102c1d905..a85727681c298 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/FileSystemConfigurations.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/FileSystemConfigurations.java @@ -70,6 +70,7 @@ public final class FileSystemConfigurations { public static final boolean DEFAULT_AZURE_SKIP_USER_GROUP_METADATA_DURING_INITIALIZATION = false; public static final String DEFAULT_FS_AZURE_ATOMIC_RENAME_DIRECTORIES = "/hbase"; + public static final boolean DEFAULT_FS_AZURE_DISABLE_DEFAULT_CREATE_OVERWRITE = true; public static final String DEFAULT_FS_AZURE_APPEND_BLOB_DIRECTORIES = ""; public static final int DEFAULT_READ_AHEAD_QUEUE_DEPTH = -1; diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java index 45c1948a0ec7a..13368d974a042 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java @@ -40,6 +40,7 @@ import org.slf4j.LoggerFactory; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AzureBlobFileSystemException; +import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AbfsRestOperationException; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.InvalidUriException; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.SASTokenProviderException; import org.apache.hadoop.fs.azurebfs.extensions.ExtensionHelper; @@ -263,10 +264,62 @@ public AbfsRestOperation deleteFilesystem() throws AzureBlobFileSystemException return op; } - public AbfsRestOperation createPath(final String path, final boolean isFile, final boolean overwrite, - final String permission, final String umask, - final boolean isAppendBlob) throws AzureBlobFileSystemException { + public AbfsRestOperation createPath(final String path, + final boolean isFile, + final boolean overwrite, + final String permission, + final String umask, + final boolean isAppendBlob) throws AzureBlobFileSystemException { + boolean isFirstAttemptToCreateWithoutOverwrite = false; + + String operation = isFile + ? SASTokenProvider.CREATE_FILE_OPERATION + : SASTokenProvider.CREATE_DIRECTORY_OPERATION; + + // attemptFileCreateWithoutOverwriteFirst + if (isFile && overwrite + && abfsConfiguration.isDefaultCreateOverwriteDisabled()) { + isFirstAttemptToCreateWithoutOverwrite = true; + } + + AbfsRestOperation op = null; + // Query builder + final AbfsUriQueryBuilder abfsUriQueryBuilder = createDefaultUriQueryBuilder(); + abfsUriQueryBuilder.addQuery(QUERY_PARAM_RESOURCE, + operation.equals(SASTokenProvider.CREATE_FILE_OPERATION) + ? FILE + : DIRECTORY); + if (isAppendBlob) { + abfsUriQueryBuilder.addQuery(QUERY_PARAM_BLOBTYPE, APPEND_BLOB_TYPE); + } + + appendSASTokenToQuery(path, operation, abfsUriQueryBuilder); + + try { + op = createPath(path, abfsUriQueryBuilder, + (isFirstAttemptToCreateWithoutOverwrite ? false : overwrite), + permission, umask); + } catch (AbfsRestOperationException e) { + if ((e.getStatusCode() == HttpURLConnection.HTTP_CONFLICT) + && isFirstAttemptToCreateWithoutOverwrite) { + // was a first attempt made to create without overwrite. Now try again + // with overwrite now. + op = createPath(path, abfsUriQueryBuilder, true, permission, umask); + } else { + throw e; + } + } + + return op; + } + + private AbfsRestOperation createPath(final String path, + AbfsUriQueryBuilder abfsUriQueryBuilder, + final boolean overwrite, + final String permission, + final String umask) throws AzureBlobFileSystemException { final List requestHeaders = createDefaultHeaders(); + if (!overwrite) { requestHeaders.add(new AbfsHttpHeader(IF_NONE_MATCH, AbfsHttpConstants.STAR)); } @@ -279,17 +332,6 @@ public AbfsRestOperation createPath(final String path, final boolean isFile, fin requestHeaders.add(new AbfsHttpHeader(HttpHeaderConfigurations.X_MS_UMASK, umask)); } - final AbfsUriQueryBuilder abfsUriQueryBuilder = createDefaultUriQueryBuilder(); - abfsUriQueryBuilder.addQuery(QUERY_PARAM_RESOURCE, isFile ? FILE : DIRECTORY); - if (isAppendBlob) { - abfsUriQueryBuilder.addQuery(QUERY_PARAM_BLOBTYPE, APPEND_BLOB_TYPE); - } - - String operation = isFile - ? SASTokenProvider.CREATE_FILE_OPERATION - : SASTokenProvider.CREATE_DIRECTORY_OPERATION; - appendSASTokenToQuery(path, operation, abfsUriQueryBuilder); - final URL url = createRequestUrl(path, abfsUriQueryBuilder.toString()); final AbfsRestOperation op = new AbfsRestOperation( AbfsRestOperationType.CreatePath, diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java index 4b8f071b998de..cf10a3404b863 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java @@ -22,10 +22,14 @@ import java.io.FilterOutputStream; import java.io.IOException; import java.util.EnumSet; +import java.util.UUID; import org.junit.Test; +import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.CreateFlag; +import org.apache.hadoop.fs.FileAlreadyExistsException; +import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.FSDataOutputStream; import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.permission.FsPermission; @@ -34,6 +38,8 @@ import static org.apache.hadoop.fs.contract.ContractTestUtils.assertIsFile; import static org.apache.hadoop.test.LambdaTestUtils.intercept; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CONNECTIONS_MADE; + /** * Test create operation. */ @@ -188,4 +194,83 @@ public void testFilterFSWriteAfterClose() throws Throwable { }); } + @Test + public void testDefaultCreateOverwriteFileTest() throws Throwable { + testCreateFileOverwrite(true); + testCreateFileOverwrite(false); + } + + public void testCreateFileOverwrite(boolean defaultDisableCreateOverwrite) + throws Throwable { + final AzureBlobFileSystem currentFs = getFileSystem(); + Configuration config = new Configuration(this.getRawConfiguration()); + config.set("fs.azure.disable.default.create.overwrite", + Boolean.toString(defaultDisableCreateOverwrite)); + + final AzureBlobFileSystem fs = + (AzureBlobFileSystem) FileSystem.newInstance(currentFs.getUri(), + config); + + long totalConnectionMadeBeforeTest = fs.getInstrumentationMap() + .get(CONNECTIONS_MADE.getStatName()); + + int createRequestCount = 0; + final Path nonOverwriteFile = new Path("/NonOverwriteTest_FileName_" + + UUID.randomUUID().toString()); + + // Case 1: Not Overwrite - File does not pre-exist + // create should be successful + fs.create(nonOverwriteFile, false); + + // One request to server to create path should be issued + createRequestCount++; + + assertAbfsStatistics( + CONNECTIONS_MADE, + totalConnectionMadeBeforeTest + createRequestCount, + fs.getInstrumentationMap()); + + // Case 2: Not Overwrite - File pre-exists + intercept(FileAlreadyExistsException.class, + () -> fs.create(nonOverwriteFile,false)); + + // One request to server to create path should be issued + createRequestCount++; + + assertAbfsStatistics( + CONNECTIONS_MADE, + totalConnectionMadeBeforeTest + createRequestCount, + fs.getInstrumentationMap()); + + final Path overwriteFilePath = new Path("/OverwriteTest_FileName_" + + UUID.randomUUID().toString()); + + // Case 3: Overwrite - File does not pre-exist + // create should be successful + fs.create(overwriteFilePath, true); + + // One request to server to create path should be issued + createRequestCount++; + + assertAbfsStatistics( + CONNECTIONS_MADE, + totalConnectionMadeBeforeTest + createRequestCount, + fs.getInstrumentationMap()); + + // Case 4: Overwrite - File pre-exists + fs.create(overwriteFilePath, true); + + if (defaultDisableCreateOverwrite) { + // Two requests will be sent to server to create path, one without overwrite + // and another with overwrite should be issued. + createRequestCount += 2; + } else { + createRequestCount++; + } + + assertAbfsStatistics( + CONNECTIONS_MADE, + totalConnectionMadeBeforeTest + createRequestCount, + fs.getInstrumentationMap()); + } } diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java index 382d3966485f1..bfb88643c9e71 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java @@ -18,12 +18,18 @@ package org.apache.hadoop.fs.azurebfs; +import java.util.UUID; + import org.junit.Test; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import static org.apache.hadoop.fs.contract.ContractTestUtils.assertMkdirs; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CONNECTIONS_MADE; + /** * Test mkdir operation. */ @@ -45,4 +51,54 @@ public void testCreateDirWithExistingDir() throws Exception { public void testCreateRoot() throws Exception { assertMkdirs(getFileSystem(), new Path("/")); } + + @Test + public void testDefaultCreateOverwriteDirTest() throws Throwable { + // the config fs.azure.disable.default.create.overwrite should have no + // effect on mkdirs + testCreateDirOverwrite(true); + testCreateDirOverwrite(false); + } + + public void testCreateDirOverwrite(boolean defaultDisableCreateOverwrite) + throws Throwable { + final AzureBlobFileSystem currentFs = getFileSystem(); + Configuration config = new Configuration(this.getRawConfiguration()); + config.set("fs.azure.disable.default.create.overwrite", + Boolean.toString(defaultDisableCreateOverwrite)); + + final AzureBlobFileSystem fs = + (AzureBlobFileSystem) FileSystem.newInstance(currentFs.getUri(), + config); + + long totalConnectionMadeBeforeTest = fs.getInstrumentationMap() + .get(CONNECTIONS_MADE.getStatName()); + + int mkdirRequestCount = 0; + final Path dirPath = new Path("/DirPath_" + + UUID.randomUUID().toString()); + + // Case 1: Dir does not pre-exist + fs.mkdirs(dirPath); + + // One request to server + mkdirRequestCount++; + + assertAbfsStatistics( + CONNECTIONS_MADE, + totalConnectionMadeBeforeTest + mkdirRequestCount, + fs.getInstrumentationMap()); + + // Case 2: Dir pre-exists + // Mkdir on existing Dir path will not lead to failure + fs.mkdirs(dirPath); + + // One request to server + mkdirRequestCount++; + + assertAbfsStatistics( + CONNECTIONS_MADE, + totalConnectionMadeBeforeTest + mkdirRequestCount, + fs.getInstrumentationMap()); + } } From 670ac1c129b43a47c9c54e333c8340c033f3dd82 Mon Sep 17 00:00:00 2001 From: Sneha Vijayarajan Date: Wed, 26 Aug 2020 10:26:04 +0530 Subject: [PATCH 02/11] Test fix --- .../azurebfs/ITestAbfsNetworkStatistics.java | 34 +++++++++++++------ 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java index f6ee7a90faa31..c55521cbac108 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java @@ -112,6 +112,14 @@ public void testAbfsHttpSendStatistics() throws IOException { try (AbfsOutputStream out = createAbfsOutputStreamWithFlushEnabled(fs, sendRequestPath)) { + boolean createOverwriteNeedsAdditionalRequest = false; + if (fs.getAbfsStore() + .getAbfsConfiguration() + .isDefaultCreateOverwriteDisabled()) { + // if the default overwrite=true behaviour is disabled by config, + // the re-create here will need 2 requests. + createOverwriteNeedsAdditionalRequest = true; + } for (int i = 0; i < LARGE_OPERATIONS; i++) { out.write(testNetworkStatsString.getBytes()); @@ -141,17 +149,22 @@ public void testAbfsHttpSendStatistics() throws IOException { * wrote each time). * */ + long expectedConnectionsMade = connectionsMade + 1 + + (createOverwriteNeedsAdditionalRequest ? 1 : 0); + long expectedSendRequests = requestsSent + 1 + + (createOverwriteNeedsAdditionalRequest ? 1 : 0); + if (fs.getAbfsStore().isAppendBlobKey(fs.makeQualified(sendRequestPath).toString())) { // no network calls are made for hflush in case of appendblob assertAbfsStatistics(CONNECTIONS_MADE, - connectionsMade + 1 + LARGE_OPERATIONS, metricMap); + expectedConnectionsMade + LARGE_OPERATIONS, metricMap); assertAbfsStatistics(SEND_REQUESTS, - requestsSent + 1 + LARGE_OPERATIONS, metricMap); + expectedSendRequests + LARGE_OPERATIONS, metricMap); } else { assertAbfsStatistics(CONNECTIONS_MADE, - connectionsMade + 1 + LARGE_OPERATIONS * 2, metricMap); + expectedConnectionsMade + LARGE_OPERATIONS * 2, metricMap); assertAbfsStatistics(SEND_REQUESTS, - requestsSent + 1 + LARGE_OPERATIONS * 2, metricMap); + expectedSendRequests + LARGE_OPERATIONS * 2, metricMap); } assertAbfsStatistics(AbfsStatistic.BYTES_SENT, bytesSent + LARGE_OPERATIONS * (testNetworkStatsString.getBytes().length), @@ -223,9 +236,9 @@ public void testAbfsHttpResponseStatistics() throws IOException { // Testing that bytes received is equal to bytes sent. long bytesSend = metricMap.get(AbfsStatistic.BYTES_SENT.getStatName()); - bytesReceived = assertAbfsStatistics(AbfsStatistic.BYTES_RECEIVED, - bytesSend, - metricMap); + //bytesReceived = assertAbfsStatistics(AbfsStatistic.BYTES_RECEIVED, + // bytesSend, + //metricMap); } finally { IOUtils.cleanupWithLogger(LOG, out, in); @@ -244,6 +257,7 @@ public void testAbfsHttpResponseStatistics() throws IOException { */ StringBuilder largeBuffer = new StringBuilder(); out = fs.create(getResponsePath); + for (int i = 0; i < LARGE_OPERATIONS; i++) { out.write(testResponseString.getBytes()); out.hflush(); @@ -275,9 +289,9 @@ public void testAbfsHttpResponseStatistics() throws IOException { * File). * */ - assertAbfsStatistics(AbfsStatistic.BYTES_RECEIVED, - bytesReceived + LARGE_OPERATIONS * (testResponseString.getBytes().length), - metricMap); + //assertAbfsStatistics(AbfsStatistic.BYTES_RECEIVED, + // bytesReceived + LARGE_OPERATIONS * (testResponseString.getBytes().length), + //metricMap); if (fs.getAbfsStore().isAppendBlobKey(fs.makeQualified(getResponsePath).toString())) { // no network calls are made for hflush in case of appendblob assertAbfsStatistics(AbfsStatistic.GET_RESPONSES, From a55898b8155af657afc4fc1f67439fa2406984a7 Mon Sep 17 00:00:00 2001 From: Sneha Vijayarajan Date: Wed, 26 Aug 2020 10:36:00 +0530 Subject: [PATCH 03/11] Undo testAbfsHttpResponseStatistics code comment --- .../fs/azurebfs/ITestAbfsNetworkStatistics.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java index c55521cbac108..d1e03394eae75 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java @@ -236,9 +236,9 @@ public void testAbfsHttpResponseStatistics() throws IOException { // Testing that bytes received is equal to bytes sent. long bytesSend = metricMap.get(AbfsStatistic.BYTES_SENT.getStatName()); - //bytesReceived = assertAbfsStatistics(AbfsStatistic.BYTES_RECEIVED, - // bytesSend, - //metricMap); + bytesReceived = assertAbfsStatistics(AbfsStatistic.BYTES_RECEIVED, + bytesSend, + metricMap); } finally { IOUtils.cleanupWithLogger(LOG, out, in); @@ -289,9 +289,9 @@ public void testAbfsHttpResponseStatistics() throws IOException { * File). * */ - //assertAbfsStatistics(AbfsStatistic.BYTES_RECEIVED, - // bytesReceived + LARGE_OPERATIONS * (testResponseString.getBytes().length), - //metricMap); + assertAbfsStatistics(AbfsStatistic.BYTES_RECEIVED, + bytesReceived + LARGE_OPERATIONS * (testResponseString.getBytes().length), + metricMap); if (fs.getAbfsStore().isAppendBlobKey(fs.makeQualified(getResponsePath).toString())) { // no network calls are made for hflush in case of appendblob assertAbfsStatistics(AbfsStatistic.GET_RESPONSES, From 0667067bff245db7509922bf9c355b816c659092 Mon Sep 17 00:00:00 2001 From: Sneha Vijayarajan Date: Wed, 26 Aug 2020 15:38:32 +0530 Subject: [PATCH 04/11] Incorporating Vinay's review comments --- .../azurebfs/constants/ConfigurationKeys.java | 6 ++++-- .../hadoop/fs/azurebfs/services/AbfsClient.java | 17 +++++++++++------ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java index a28e0579cf2b1..43724ecd537ea 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java @@ -67,8 +67,10 @@ public final class ConfigurationKeys { public static final String FS_AZURE_ENABLE_AUTOTHROTTLING = "fs.azure.enable.autothrottling"; public static final String FS_AZURE_ALWAYS_USE_HTTPS = "fs.azure.always.use.https"; public static final String FS_AZURE_ATOMIC_RENAME_KEY = "fs.azure.atomic.rename.key"; - /** HDFS FS defaults overwrite behaviour to true which leads to race conditions at backend with parallel operations - * issued to same path. This config provides a means to override the default overwrite flag. + /** HDFS FS defaults overwrite behaviour to true which can lead to race + * conditions at times at backend with parallel operations issued to same path. + * Hence in ABFS driver the overwrite default behaviour is being set to false. + * This is added as a config to provide relief for any contigencies. */ public static final String FS_AZURE_DISABLE_DEFAULT_CREATE_OVERWRITE = "fs.azure.disable.default.create.overwrite"; /** Provides a config to provide comma separated path prefixes on which Appendblob based files are created diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java index 13368d974a042..5d5e741e99761 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java @@ -270,13 +270,17 @@ public AbfsRestOperation createPath(final String path, final String permission, final String umask, final boolean isAppendBlob) throws AzureBlobFileSystemException { - boolean isFirstAttemptToCreateWithoutOverwrite = false; - String operation = isFile ? SASTokenProvider.CREATE_FILE_OPERATION : SASTokenProvider.CREATE_DIRECTORY_OPERATION; - // attemptFileCreateWithoutOverwriteFirst + // HDFS FS defaults overwrite behaviour to true for create file which leads + // to majority create API traffic with overwrite=true. In some cases, this + // will end in race conditions at backend with parallel operations issued to + // same path either by means of the customer workload or ABFS driver retry. + // Disabling the create overwrite default setting to false should + // significantly reduce the chances for such race conditions. + boolean isFirstAttemptToCreateWithoutOverwrite = false; if (isFile && overwrite && abfsConfiguration.isDefaultCreateOverwriteDisabled()) { isFirstAttemptToCreateWithoutOverwrite = true; @@ -296,15 +300,16 @@ public AbfsRestOperation createPath(final String path, appendSASTokenToQuery(path, operation, abfsUriQueryBuilder); try { - op = createPath(path, abfsUriQueryBuilder, + op = createPathImpl(path, abfsUriQueryBuilder, (isFirstAttemptToCreateWithoutOverwrite ? false : overwrite), permission, umask); } catch (AbfsRestOperationException e) { if ((e.getStatusCode() == HttpURLConnection.HTTP_CONFLICT) && isFirstAttemptToCreateWithoutOverwrite) { + isFirstAttemptToCreateWithoutOverwrite = false; // was a first attempt made to create without overwrite. Now try again // with overwrite now. - op = createPath(path, abfsUriQueryBuilder, true, permission, umask); + op = createPathImpl(path, abfsUriQueryBuilder, true, permission, umask); } else { throw e; } @@ -313,7 +318,7 @@ public AbfsRestOperation createPath(final String path, return op; } - private AbfsRestOperation createPath(final String path, + private AbfsRestOperation createPathImpl(final String path, AbfsUriQueryBuilder abfsUriQueryBuilder, final boolean overwrite, final String permission, From ab088a5a31fc2e8bf04494487ab4f0183f01f2e1 Mon Sep 17 00:00:00 2001 From: Sneha Vijayarajan Date: Wed, 26 Aug 2020 17:19:01 +0530 Subject: [PATCH 05/11] Checkstyle fixes --- .../java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java | 4 +++- .../hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java index a5216eb958ce7..4faa777a590d3 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java @@ -577,7 +577,9 @@ public String getAzureAtomicRenameDirs() { return this.azureAtomicDirs; } - public boolean isDefaultCreateOverwriteDisabled() { return this.disableDefaultCreateOverwrite; } + public boolean isDefaultCreateOverwriteDisabled() { + return this.disableDefaultCreateOverwrite; + } public String getAppendBlobDirs() { return this.azureAppendBlobDirs; diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java index cf10a3404b863..b4b7cc718e50b 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java @@ -232,7 +232,7 @@ public void testCreateFileOverwrite(boolean defaultDisableCreateOverwrite) // Case 2: Not Overwrite - File pre-exists intercept(FileAlreadyExistsException.class, - () -> fs.create(nonOverwriteFile,false)); + () -> fs.create(nonOverwriteFile, false)); // One request to server to create path should be issued createRequestCount++; From f4ef74006b7eb05c90d19fd065b9b6dc753f4a3e Mon Sep 17 00:00:00 2001 From: Sneha Vijayarajan Date: Fri, 11 Sep 2020 17:21:16 +0530 Subject: [PATCH 06/11] Add etag precondition check --- ...urrentWriteOperationDetectedException.java | 32 ++++++++++ .../fs/azurebfs/services/AbfsClient.java | 58 +++++++++++++++++-- .../ITestAzureBlobFileSystemCreate.java | 19 +++++- 3 files changed, 101 insertions(+), 8 deletions(-) create mode 100644 hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/contracts/exceptions/ConcurrentWriteOperationDetectedException.java diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/contracts/exceptions/ConcurrentWriteOperationDetectedException.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/contracts/exceptions/ConcurrentWriteOperationDetectedException.java new file mode 100644 index 0000000000000..79813ddfe6400 --- /dev/null +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/contracts/exceptions/ConcurrentWriteOperationDetectedException.java @@ -0,0 +1,32 @@ +/** + * 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.contracts.exceptions; + +/** + * Thrown when a concurrent write operation is detected. + */ +@org.apache.hadoop.classification.InterfaceAudience.Public +@org.apache.hadoop.classification.InterfaceStability.Evolving +public class ConcurrentWriteOperationDetectedException + extends AzureBlobFileSystemException { + + public ConcurrentWriteOperationDetectedException(String message) { + super(message); + } +} diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java index 5d5e741e99761..f3bf877625980 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java @@ -41,6 +41,7 @@ import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AzureBlobFileSystemException; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AbfsRestOperationException; +import org.apache.hadoop.fs.azurebfs.contracts.exceptions.ConcurrentWriteOperationDetectedException; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.InvalidUriException; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.SASTokenProviderException; import org.apache.hadoop.fs.azurebfs.extensions.ExtensionHelper; @@ -293,6 +294,7 @@ public AbfsRestOperation createPath(final String path, operation.equals(SASTokenProvider.CREATE_FILE_OPERATION) ? FILE : DIRECTORY); + if (isAppendBlob) { abfsUriQueryBuilder.addQuery(QUERY_PARAM_BLOBTYPE, APPEND_BLOB_TYPE); } @@ -302,14 +304,55 @@ public AbfsRestOperation createPath(final String path, try { op = createPathImpl(path, abfsUriQueryBuilder, (isFirstAttemptToCreateWithoutOverwrite ? false : overwrite), - permission, umask); + permission, umask, null); } catch (AbfsRestOperationException e) { if ((e.getStatusCode() == HttpURLConnection.HTTP_CONFLICT) && isFirstAttemptToCreateWithoutOverwrite) { + // Was the first attempt made to create file without overwrite which + // failed because there is a pre-existing file. + // + // There is a rare possibility of race condition incase of create with + // overwrite=true. One such incident lead to data loss, where a retried + // create overwrite succeeded followed by an append, but the very first + // create overwrite made it to backend post the append. This lead to the + // file being overwritten and hence data loss. To prevent this scenario, + // first fetch the eTag of current file and set condition to overwrite + // only if eTag matches. isFirstAttemptToCreateWithoutOverwrite = false; - // was a first attempt made to create without overwrite. Now try again - // with overwrite now. - op = createPathImpl(path, abfsUriQueryBuilder, true, permission, umask); + + // Fetch eTag + try { + op = getPathStatus(path, false); + } catch (AbfsRestOperationException ex) { + if (e.getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) { + // Is a parallel access case, as file which was found to be + // present went missing by this request. + throw new ConcurrentWriteOperationDetectedException( + "Parallel access to the create path detected. Failing request " + + "to honor single writer semantics"); + } else { + throw ex; + } + } + + String eTag = op.getResult().getResponseHeader(ETAG); + + try { + // overwrite + op = createPathImpl(path, abfsUriQueryBuilder, true, permission, + umask, eTag); + } catch (AbfsRestOperationException ex) { + if (e.getStatusCode() == HttpURLConnection.HTTP_PRECON_FAILED) { + // Is a parallel access case, as file with eTag was just queried + // and precondition failure can happen only when another file with + // different etag got created. + throw new ConcurrentWriteOperationDetectedException( + "Parallel access to the create path detected. Failing request " + + "to honor single writer semantics"); + } else { + throw ex; + } + } } else { throw e; } @@ -322,7 +365,8 @@ private AbfsRestOperation createPathImpl(final String path, AbfsUriQueryBuilder abfsUriQueryBuilder, final boolean overwrite, final String permission, - final String umask) throws AzureBlobFileSystemException { + final String umask, + final String eTag) throws AzureBlobFileSystemException { final List requestHeaders = createDefaultHeaders(); if (!overwrite) { @@ -337,6 +381,10 @@ private AbfsRestOperation createPathImpl(final String path, requestHeaders.add(new AbfsHttpHeader(HttpHeaderConfigurations.X_MS_UMASK, umask)); } + if (eTag != null && !eTag.isEmpty()) { + requestHeaders.add(new AbfsHttpHeader(HttpHeaderConfigurations.IF_MATCH, eTag)); + } + final URL url = createRequestUrl(path, abfsUriQueryBuilder.toString()); final AbfsRestOperation op = new AbfsRestOperation( AbfsRestOperationType.CreatePath, diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java index b4b7cc718e50b..9717ff2bf6d0d 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java @@ -194,6 +194,17 @@ public void testFilterFSWriteAfterClose() throws Throwable { }); } + /** + * Tests if the number of connections made for: + * 1. create overwrite=false of a file that doesnt pre-exist + * 2. create overwrite=false of a file that pre-exists + * 3. create overwrite=true of a file that doesnt pre-exist + * 4. create overwrite=true of a file that pre-exists + * matches the expectation when run against both combinations of + * fs.azure.disable.default.create.overwrite=true and + * fs.azure.disable.default.create.overwrite=false + * @throws Throwable + */ @Test public void testDefaultCreateOverwriteFileTest() throws Throwable { testCreateFileOverwrite(true); @@ -261,9 +272,11 @@ public void testCreateFileOverwrite(boolean defaultDisableCreateOverwrite) fs.create(overwriteFilePath, true); if (defaultDisableCreateOverwrite) { - // Two requests will be sent to server to create path, one without overwrite - // and another with overwrite should be issued. - createRequestCount += 2; + // Three requests will be sent to server to create path, + // 1. create without overwrite + // 2. GetFileStatus to get eTag + // 3. create with overwrite + createRequestCount += 3; } else { createRequestCount++; } From 6ff28f13ded5ad719d79e1b47201874c5098b50e Mon Sep 17 00:00:00 2001 From: Sneha Vijayarajan Date: Mon, 14 Sep 2020 14:27:05 +0530 Subject: [PATCH 07/11] Fixing and adding additional testcase --- .../fs/azurebfs/services/AbfsClient.java | 7 +- .../azurebfs/ITestAbfsNetworkStatistics.java | 48 ++--- .../ITestAzureBlobFileSystemCreate.java | 166 +++++++++++++++++- .../fs/azurebfs/services/TestAbfsClient.java | 11 +- 4 files changed, 207 insertions(+), 25 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java index f3bf877625980..21aee95fb31a1 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java @@ -324,7 +324,7 @@ public AbfsRestOperation createPath(final String path, try { op = getPathStatus(path, false); } catch (AbfsRestOperationException ex) { - if (e.getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) { + if (ex.getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) { // Is a parallel access case, as file which was found to be // present went missing by this request. throw new ConcurrentWriteOperationDetectedException( @@ -342,7 +342,7 @@ public AbfsRestOperation createPath(final String path, op = createPathImpl(path, abfsUriQueryBuilder, true, permission, umask, eTag); } catch (AbfsRestOperationException ex) { - if (e.getStatusCode() == HttpURLConnection.HTTP_PRECON_FAILED) { + if (ex.getStatusCode() == HttpURLConnection.HTTP_PRECON_FAILED) { // Is a parallel access case, as file with eTag was just queried // and precondition failure can happen only when another file with // different etag got created. @@ -361,7 +361,8 @@ public AbfsRestOperation createPath(final String path, return op; } - private AbfsRestOperation createPathImpl(final String path, + @VisibleForTesting + public AbfsRestOperation createPathImpl(final String path, AbfsUriQueryBuilder abfsUriQueryBuilder, final boolean overwrite, final String permission, diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java index d1e03394eae75..f96097ea52950 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java @@ -110,15 +110,16 @@ public void testAbfsHttpSendStatistics() throws IOException { connectionsMade++; requestsSent++; + try (AbfsOutputStream out = createAbfsOutputStreamWithFlushEnabled(fs, sendRequestPath)) { - boolean createOverwriteNeedsAdditionalRequest = false; - if (fs.getAbfsStore() - .getAbfsConfiguration() - .isDefaultCreateOverwriteDisabled()) { - // if the default overwrite=true behaviour is disabled by config, - // the re-create here will need 2 requests. - createOverwriteNeedsAdditionalRequest = true; + + // Is a file overwrite case + long createRequestCalls = 1; + long createTriggeredGFSForETag = 0; + if (this.getConfiguration().isDefaultCreateOverwriteDisabled()) { + createRequestCalls += 1; + createTriggeredGFSForETag = 1; } for (int i = 0; i < LARGE_OPERATIONS; i++) { @@ -149,22 +150,20 @@ public void testAbfsHttpSendStatistics() throws IOException { * wrote each time). * */ - long expectedConnectionsMade = connectionsMade + 1 - + (createOverwriteNeedsAdditionalRequest ? 1 : 0); - long expectedSendRequests = requestsSent + 1 - + (createOverwriteNeedsAdditionalRequest ? 1 : 0); + connectionsMade += createRequestCalls + createTriggeredGFSForETag; + requestsSent += createRequestCalls; if (fs.getAbfsStore().isAppendBlobKey(fs.makeQualified(sendRequestPath).toString())) { // no network calls are made for hflush in case of appendblob assertAbfsStatistics(CONNECTIONS_MADE, - expectedConnectionsMade + LARGE_OPERATIONS, metricMap); + connectionsMade + LARGE_OPERATIONS, metricMap); assertAbfsStatistics(SEND_REQUESTS, - expectedSendRequests + LARGE_OPERATIONS, metricMap); + requestsSent + LARGE_OPERATIONS, metricMap); } else { assertAbfsStatistics(CONNECTIONS_MADE, - expectedConnectionsMade + LARGE_OPERATIONS * 2, metricMap); + connectionsMade + LARGE_OPERATIONS * 2, metricMap); assertAbfsStatistics(SEND_REQUESTS, - expectedSendRequests + LARGE_OPERATIONS * 2, metricMap); + requestsSent + LARGE_OPERATIONS * 2, metricMap); } assertAbfsStatistics(AbfsStatistic.BYTES_SENT, bytesSent + LARGE_OPERATIONS * (testNetworkStatsString.getBytes().length), @@ -250,14 +249,21 @@ public void testAbfsHttpResponseStatistics() throws IOException { try { /* - * Creating a file and writing buffer into it. Also recording the - * buffer for future read() call. + * Creating a file and writing buffer into it. + * This is a file recreate, so it will trigger + * 2 extra calls if create overwrite is off by default. + * Also recording the buffer for future read() call. * This creating outputStream and writing requires 2 * * (LARGE_OPERATIONS) get requests. */ StringBuilder largeBuffer = new StringBuilder(); out = fs.create(getResponsePath); + long createRequestCalls = 1; + if (this.getConfiguration().isDefaultCreateOverwriteDisabled()) { + createRequestCalls += 2; + } + for (int i = 0; i < LARGE_OPERATIONS; i++) { out.write(testResponseString.getBytes()); out.hflush(); @@ -282,7 +288,8 @@ public void testAbfsHttpResponseStatistics() throws IOException { * * get_response : get_responses(Last assertion) + 1 * (OutputStream) + 2 * LARGE_OPERATIONS(Writing and flushing - * LARGE_OPERATIONS times) + 1(open()) + 1(read()). + * LARGE_OPERATIONS times) + 1(open()) + 1(read()) + + * 1 (createOverwriteTriggeredGetForeTag). * * bytes_received : bytes_received(Last assertion) + LARGE_OPERATIONS * * bytes wrote each time (bytes_received is equal to bytes wrote in the @@ -298,7 +305,8 @@ public void testAbfsHttpResponseStatistics() throws IOException { getResponses + 3 + LARGE_OPERATIONS, metricMap); } else { assertAbfsStatistics(AbfsStatistic.GET_RESPONSES, - getResponses + 3 + 2 * LARGE_OPERATIONS, metricMap); + getResponses + 2 + createRequestCalls + 2 * LARGE_OPERATIONS, + metricMap); } } finally { @@ -333,4 +341,4 @@ public void testAbfsHttpResponseFailure() throws IOException { IOUtils.cleanupWithLogger(LOG, out); } } -} +} \ No newline at end of file diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java index 9717ff2bf6d0d..d7bd7b38153d7 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java @@ -35,9 +35,26 @@ import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.test.GenericTestUtils; +import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AbfsRestOperationException; +import org.apache.hadoop.fs.azurebfs.contracts.exceptions.ConcurrentWriteOperationDetectedException; +import org.apache.hadoop.fs.azurebfs.services.AbfsHttpOperation; +import org.apache.hadoop.fs.azurebfs.services.AbfsRestOperation; +import org.apache.hadoop.fs.azurebfs.services.AbfsUriQueryBuilder; + +import static java.net.HttpURLConnection.HTTP_CONFLICT; +import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR; +import static java.net.HttpURLConnection.HTTP_NOT_FOUND; +import static java.net.HttpURLConnection.HTTP_OK; +import static java.net.HttpURLConnection.HTTP_PRECON_FAILED; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + import static org.apache.hadoop.fs.contract.ContractTestUtils.assertIsFile; import static org.apache.hadoop.test.LambdaTestUtils.intercept; - import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CONNECTIONS_MADE; /** @@ -286,4 +303,151 @@ public void testCreateFileOverwrite(boolean defaultDisableCreateOverwrite) totalConnectionMadeBeforeTest + createRequestCount, fs.getInstrumentationMap()); } + + /** + * Test negative scenarios with Create overwrite=false as default + * With create overwrite=true ending in 3 calls: + * A. Create overwrite=false + * B. GFS + * C. Create overwrite=true + * + * Scn1: A fails with HTTP409, leading to B which fails with HTTP404, + * detect parallel access + * Scn2: A fails with HTTP409, leading to B which fails with HTTP500, + * fail create with HTTP500 + * Scn3: A fails with HTTP409, leading to B and then C, + * which fails with HTTP412, detect parallel access + * Scn4: A fails with HTTP409, leading to B and then C, + * which fails with HTTP500, fail create with HTTP500 + * Scn5: A fails with HTTP500, fail create with HTTP500 + */ + @Test + public void testNegativeScenariosForCreateOverwriteDisabled() + throws Throwable { + + final AzureBlobFileSystem currentFs = getFileSystem(); + Configuration config = new Configuration(this.getRawConfiguration()); + config.set("fs.azure.disable.default.create.overwrite", + Boolean.toString(true)); + + final AzureBlobFileSystem fs = + (AzureBlobFileSystem) FileSystem.newInstance(currentFs.getUri(), + config); + + // Get mock AbfsClient with current config + org.apache.hadoop.fs.azurebfs.services.AbfsClient + mockClient + = org.apache.hadoop.fs.azurebfs.services.TestAbfsClient.getMockAbfsClient( + fs.getAbfsStore().getClient(), + fs.getAbfsStore().getAbfsConfiguration()); + + AbfsRestOperation successOp = mock( + AbfsRestOperation.class); + AbfsHttpOperation http200Op = mock( + AbfsHttpOperation.class); + when(http200Op.getStatusCode()).thenReturn(HTTP_OK); + when(successOp.getResult()).thenReturn(http200Op); + + AbfsRestOperationException conflictResponseEx + = getMockAbfsRestOperationException(HTTP_CONFLICT); + AbfsRestOperationException serverErrorResponseEx + = getMockAbfsRestOperationException(HTTP_INTERNAL_ERROR); + AbfsRestOperationException fileNotFoundResponseEx + = getMockAbfsRestOperationException(HTTP_NOT_FOUND); + AbfsRestOperationException preConditionResponseEx + = getMockAbfsRestOperationException(HTTP_PRECON_FAILED); + + doThrow(conflictResponseEx) // Scn1: GFS fails with Http404 + .doThrow(conflictResponseEx) // Scn2: GFS fails with Http500 + .doThrow( + conflictResponseEx) // Scn3: create overwrite=true fails with Http412 + .doThrow( + conflictResponseEx) // Scn4: create overwrite=true fails with Http500 + .doThrow( + serverErrorResponseEx) // Scn5: create overwrite=false fails with Http500 + .when(mockClient) + .createPathImpl(any(String.class), any( + AbfsUriQueryBuilder.class), + eq(false), any(String.class), any(String.class), eq(null)); + + doThrow(fileNotFoundResponseEx) // Scn1: GFS fails with Http404 + .doThrow(serverErrorResponseEx) // Scn2: GFS fails with Http500 + .doReturn(successOp) // Scn3: create overwrite=true fails with Http412 + .doReturn(successOp) // Scn4: create overwrite=true fails with Http500 + .when(mockClient) + .getPathStatus(any(String.class), eq(false)); + + doThrow( + preConditionResponseEx) // Scn3: create overwrite=true fails with Http412 + .doThrow( + serverErrorResponseEx) // Scn4: create overwrite=true fails with Http500 + .when(mockClient) + .createPathImpl(any(String.class), any( + AbfsUriQueryBuilder.class), + eq(true), any(String.class), any(String.class), eq(null)); + + when(mockClient.createPath(any(String.class), eq(true), eq(true), + any(String.class), + any(String.class), eq(false))).thenCallRealMethod(); + + // Scn1: GFS fails with Http404 + // Sequence of events expected: + // 1. create overwrite=false - fail with conflict + // 2. GFS - fail with File Not found + // Create will fail with ConcurrentWriteOperationDetectedException + intercept( + ConcurrentWriteOperationDetectedException.class, + () -> + mockClient.createPath("someTestPath", true, true, "0644", "0022", + false)); + + // Scn2: GFS fails with Http500 + // Sequence of events expected: + // 1. create overwrite=false - fail with conflict + // 2. GFS - fail with Server error + // Create will fail with 500 + intercept( + AbfsRestOperationException.class, + () -> + mockClient.createPath("someTestPath", true, true, "0644", "0022", + false)); + + // Scn3: create overwrite=true fails with Http412 + // Sequence of events expected: + // 1. create overwrite=false - fail with conflict + // 2. GFS - pass + // 3. create overwrite=true - fail with Pre-Condition + // Create will fail with ConcurrentWriteOperationDetectedException + intercept( + ConcurrentWriteOperationDetectedException.class, + () -> + mockClient.createPath("someTestPath", true, true, "0644", "0022", + false)); + + // Scn4: create overwrite=true fails with Http500 + // Sequence of events expected: + // 1. create overwrite=false - fail with conflict + // 2. GFS - pass + // 3. create overwrite=true - fail with Server error + // Create will fail with 500 + intercept( + AbfsRestOperationException.class, + () -> + mockClient.createPath("someTestPath", true, true, "0644", "0022", + false)); + + // Scn5: create overwrite=false fails with Http500 + // Sequence of events expected: + // 1. create overwrite=false - fail with server error + // Create will fail with 500 + intercept( + AbfsRestOperationException.class, + () -> + mockClient.createPath("someTestPath", true, true, "0644", "0022", + false)); + } + + private AbfsRestOperationException getMockAbfsRestOperationException(int status) { + return new AbfsRestOperationException(status, "", "", new Exception()); + } } diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java index 0d904c85523e7..2badbfa796e61 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java @@ -309,10 +309,19 @@ public static AbfsClient getMockAbfsClient(AbfsClient baseAbfsClientInstance, when(client.getSharedKeyCredentials()).thenCallRealMethod(); when(client.createDefaultHeaders()).thenCallRealMethod(); + // override baseurl + Field abfsConfigurationField = AbfsClient.class.getDeclaredField("abfsConfiguration"); + abfsConfigurationField.setAccessible(true); + Field modifiersField = Field.class.getDeclaredField("modifiers"); + modifiersField.setAccessible(true); + modifiersField.setInt(abfsConfigurationField, + abfsConfigurationField.getModifiers() + & ~java.lang.reflect.Modifier.FINAL); + abfsConfigurationField.set(client, abfsConfig); + // override baseurl Field baseUrlField = AbfsClient.class.getDeclaredField("baseUrl"); baseUrlField.setAccessible(true); - Field modifiersField = Field.class.getDeclaredField("modifiers"); modifiersField.setAccessible(true); modifiersField.setInt(baseUrlField, baseUrlField.getModifiers() & ~java.lang.reflect.Modifier.FINAL); baseUrlField.set(client, baseAbfsClientInstance.getBaseUrl()); From c4de926a22fbe9d5a474a4249cf200f9ede36669 Mon Sep 17 00:00:00 2001 From: Sneha Vijayarajan Date: Mon, 14 Sep 2020 15:06:57 +0530 Subject: [PATCH 08/11] Dummy commit to trigger yetus --- .../hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java index bfb88643c9e71..ab3271c5906bc 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java @@ -47,6 +47,10 @@ public void testCreateDirWithExistingDir() throws Exception { assertMkdirs(fs, path); } + /** + * Test mkdir for possible values of fs.azure.disable.default.create.overwrite + * @throws Exception + */ @Test public void testCreateRoot() throws Exception { assertMkdirs(getFileSystem(), new Path("/")); From bceadbd84b4a031e354170e9909dfdd41f87ebae Mon Sep 17 00:00:00 2001 From: Sneha Vijayarajan Date: Wed, 16 Sep 2020 17:50:57 +0530 Subject: [PATCH 09/11] Incorporating comments from Thomas's review --- .../hadoop/fs/azurebfs/AbfsConfiguration.java | 10 ++++---- .../azurebfs/constants/ConfigurationKeys.java | 8 +++---- .../constants/FileSystemConfigurations.java | 2 +- .../fs/azurebfs/services/AbfsClient.java | 23 ++++++------------- .../azurebfs/ITestAbfsNetworkStatistics.java | 4 ++-- .../ITestAzureBlobFileSystemCreate.java | 14 +++++------ .../ITestAzureBlobFileSystemMkDir.java | 6 ++--- 7 files changed, 28 insertions(+), 39 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java index 4faa777a590d3..72a8a435717e3 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java @@ -181,9 +181,9 @@ public class AbfsConfiguration{ DefaultValue = DEFAULT_FS_AZURE_ATOMIC_RENAME_DIRECTORIES) private String azureAtomicDirs; - @BooleanConfigurationValidatorAnnotation(ConfigurationKey = FS_AZURE_DISABLE_DEFAULT_CREATE_OVERWRITE, - DefaultValue = DEFAULT_FS_AZURE_DISABLE_DEFAULT_CREATE_OVERWRITE) - private boolean disableDefaultCreateOverwrite; + @BooleanConfigurationValidatorAnnotation(ConfigurationKey = FS_AZURE_ENABLE_CONDITIONAL_CREATE_OVERWRITE, + DefaultValue = DEFAULT_FS_AZURE_ENABLE_CONDITIONAL_CREATE_OVERWRITE) + private boolean enableConditionalCreateOverwrite; @StringConfigurationValidatorAnnotation(ConfigurationKey = FS_AZURE_APPEND_BLOB_KEY, DefaultValue = DEFAULT_FS_AZURE_APPEND_BLOB_DIRECTORIES) @@ -577,8 +577,8 @@ public String getAzureAtomicRenameDirs() { return this.azureAtomicDirs; } - public boolean isDefaultCreateOverwriteDisabled() { - return this.disableDefaultCreateOverwrite; + public boolean isConditionalCreateOverwriteEnabled() { + return this.enableConditionalCreateOverwrite; } public String getAppendBlobDirs() { diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java index 43724ecd537ea..c15c470a4455f 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java @@ -67,12 +67,10 @@ public final class ConfigurationKeys { public static final String FS_AZURE_ENABLE_AUTOTHROTTLING = "fs.azure.enable.autothrottling"; public static final String FS_AZURE_ALWAYS_USE_HTTPS = "fs.azure.always.use.https"; public static final String FS_AZURE_ATOMIC_RENAME_KEY = "fs.azure.atomic.rename.key"; - /** HDFS FS defaults overwrite behaviour to true which can lead to race - * conditions at times at backend with parallel operations issued to same path. - * Hence in ABFS driver the overwrite default behaviour is being set to false. - * This is added as a config to provide relief for any contigencies. + /** This config ensures that during create overwrite an existing file will be + * overwritten only if there is a match on the eTag of existing file. */ - public static final String FS_AZURE_DISABLE_DEFAULT_CREATE_OVERWRITE = "fs.azure.disable.default.create.overwrite"; + public static final String FS_AZURE_ENABLE_CONDITIONAL_CREATE_OVERWRITE = "fs.azure.enable.conditional.create.overwrite"; /** Provides a config to provide comma separated path prefixes on which Appendblob based files are created * Default is empty. **/ public static final String FS_AZURE_APPEND_BLOB_KEY = "fs.azure.appendblob.directories"; diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/FileSystemConfigurations.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/FileSystemConfigurations.java index a85727681c298..fa0ee6a89212d 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/FileSystemConfigurations.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/FileSystemConfigurations.java @@ -70,7 +70,7 @@ public final class FileSystemConfigurations { public static final boolean DEFAULT_AZURE_SKIP_USER_GROUP_METADATA_DURING_INITIALIZATION = false; public static final String DEFAULT_FS_AZURE_ATOMIC_RENAME_DIRECTORIES = "/hbase"; - public static final boolean DEFAULT_FS_AZURE_DISABLE_DEFAULT_CREATE_OVERWRITE = true; + public static final boolean DEFAULT_FS_AZURE_ENABLE_CONDITIONAL_CREATE_OVERWRITE = true; public static final String DEFAULT_FS_AZURE_APPEND_BLOB_DIRECTORIES = ""; public static final int DEFAULT_READ_AHEAD_QUEUE_DEPTH = -1; diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java index 21aee95fb31a1..0adac8626f626 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java @@ -275,15 +275,13 @@ public AbfsRestOperation createPath(final String path, ? SASTokenProvider.CREATE_FILE_OPERATION : SASTokenProvider.CREATE_DIRECTORY_OPERATION; - // HDFS FS defaults overwrite behaviour to true for create file which leads - // to majority create API traffic with overwrite=true. In some cases, this - // will end in race conditions at backend with parallel operations issued to - // same path either by means of the customer workload or ABFS driver retry. - // Disabling the create overwrite default setting to false should - // significantly reduce the chances for such race conditions. + // if "fs.azure.enable.conditional.create.overwrite" is enabled, + // trigger a create with overwrite=false first so that eTag fetch can be + // avoided for cases when no pre-existing file is present (which is the + // case with most part of create traffic) boolean isFirstAttemptToCreateWithoutOverwrite = false; if (isFile && overwrite - && abfsConfiguration.isDefaultCreateOverwriteDisabled()) { + && abfsConfiguration.isConditionalCreateOverwriteEnabled()) { isFirstAttemptToCreateWithoutOverwrite = true; } @@ -310,14 +308,7 @@ public AbfsRestOperation createPath(final String path, && isFirstAttemptToCreateWithoutOverwrite) { // Was the first attempt made to create file without overwrite which // failed because there is a pre-existing file. - // - // There is a rare possibility of race condition incase of create with - // overwrite=true. One such incident lead to data loss, where a retried - // create overwrite succeeded followed by an append, but the very first - // create overwrite made it to backend post the append. This lead to the - // file being overwritten and hence data loss. To prevent this scenario, - // first fetch the eTag of current file and set condition to overwrite - // only if eTag matches. + // resetting the first attempt flag for readabiltiy isFirstAttemptToCreateWithoutOverwrite = false; // Fetch eTag @@ -338,7 +329,7 @@ public AbfsRestOperation createPath(final String path, String eTag = op.getResult().getResponseHeader(ETAG); try { - // overwrite + // overwrite only if eTag matches with the file properties fetched befpre op = createPathImpl(path, abfsUriQueryBuilder, true, permission, umask, eTag); } catch (AbfsRestOperationException ex) { diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java index f96097ea52950..c2dbe937b812b 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java @@ -117,7 +117,7 @@ public void testAbfsHttpSendStatistics() throws IOException { // Is a file overwrite case long createRequestCalls = 1; long createTriggeredGFSForETag = 0; - if (this.getConfiguration().isDefaultCreateOverwriteDisabled()) { + if (this.getConfiguration().isConditionalCreateOverwriteEnabled()) { createRequestCalls += 1; createTriggeredGFSForETag = 1; } @@ -260,7 +260,7 @@ public void testAbfsHttpResponseStatistics() throws IOException { out = fs.create(getResponsePath); long createRequestCalls = 1; - if (this.getConfiguration().isDefaultCreateOverwriteDisabled()) { + if (this.getConfiguration().isConditionalCreateOverwriteEnabled()) { createRequestCalls += 2; } diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java index d7bd7b38153d7..76a528c7d1f49 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java @@ -218,8 +218,8 @@ public void testFilterFSWriteAfterClose() throws Throwable { * 3. create overwrite=true of a file that doesnt pre-exist * 4. create overwrite=true of a file that pre-exists * matches the expectation when run against both combinations of - * fs.azure.disable.default.create.overwrite=true and - * fs.azure.disable.default.create.overwrite=false + * fs.azure.enable.conditional.create.overwrite=true and + * fs.azure.enable.conditional.create.overwrite=false * @throws Throwable */ @Test @@ -228,12 +228,12 @@ public void testDefaultCreateOverwriteFileTest() throws Throwable { testCreateFileOverwrite(false); } - public void testCreateFileOverwrite(boolean defaultDisableCreateOverwrite) + public void testCreateFileOverwrite(boolean enableConditionalCreateOverwrite) throws Throwable { final AzureBlobFileSystem currentFs = getFileSystem(); Configuration config = new Configuration(this.getRawConfiguration()); - config.set("fs.azure.disable.default.create.overwrite", - Boolean.toString(defaultDisableCreateOverwrite)); + config.set("fs.azure.enable.conditional.create.overwrite", + Boolean.toString(enableConditionalCreateOverwrite)); final AzureBlobFileSystem fs = (AzureBlobFileSystem) FileSystem.newInstance(currentFs.getUri(), @@ -288,7 +288,7 @@ public void testCreateFileOverwrite(boolean defaultDisableCreateOverwrite) // Case 4: Overwrite - File pre-exists fs.create(overwriteFilePath, true); - if (defaultDisableCreateOverwrite) { + if (enableConditionalCreateOverwrite) { // Three requests will be sent to server to create path, // 1. create without overwrite // 2. GetFileStatus to get eTag @@ -327,7 +327,7 @@ public void testNegativeScenariosForCreateOverwriteDisabled() final AzureBlobFileSystem currentFs = getFileSystem(); Configuration config = new Configuration(this.getRawConfiguration()); - config.set("fs.azure.disable.default.create.overwrite", + config.set("fs.azure.enable.conditional.create.overwrite", Boolean.toString(true)); final AzureBlobFileSystem fs = diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java index ab3271c5906bc..4620642373ed6 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java @@ -64,12 +64,12 @@ public void testDefaultCreateOverwriteDirTest() throws Throwable { testCreateDirOverwrite(false); } - public void testCreateDirOverwrite(boolean defaultDisableCreateOverwrite) + public void testCreateDirOverwrite(boolean enableConditionalCreateOverwrite) throws Throwable { final AzureBlobFileSystem currentFs = getFileSystem(); Configuration config = new Configuration(this.getRawConfiguration()); - config.set("fs.azure.disable.default.create.overwrite", - Boolean.toString(defaultDisableCreateOverwrite)); + config.set("fs.azure.enable.conditional.create.overwrite", + Boolean.toString(enableConditionalCreateOverwrite)); final AzureBlobFileSystem fs = (AzureBlobFileSystem) FileSystem.newInstance(currentFs.getUri(), From 70a17c1347e8a1f20c12c52c9f130d940ba1bcc2 Mon Sep 17 00:00:00 2001 From: Sneha Vijayarajan Date: Fri, 18 Sep 2020 22:32:21 +0530 Subject: [PATCH 10/11] Addressing review comments from Thomas --- .../fs/azurebfs/AzureBlobFileSystemStore.java | 101 +++++++++++++++- .../fs/azurebfs/services/AbfsClient.java | 111 +++--------------- .../ITestAzureBlobFileSystemCreate.java | 81 +++++++------ .../ITestAzureBlobFileSystemMkDir.java | 8 +- .../fs/azurebfs/services/TestAbfsClient.java | 67 +++++------ 5 files changed, 186 insertions(+), 182 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java index 23d2b5a3d63fb..d2a1d538f6380 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java @@ -66,6 +66,7 @@ import org.apache.hadoop.fs.azurebfs.constants.HttpHeaderConfigurations; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AbfsRestOperationException; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AzureBlobFileSystemException; +import org.apache.hadoop.fs.azurebfs.contracts.exceptions.ConcurrentWriteOperationDetectedException; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.FileSystemOperationUnhandledException; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.InvalidAbfsRestOperationException; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.InvalidFileSystemPropertyException; @@ -464,10 +465,32 @@ public OutputStream createFile(final Path path, isAppendBlob = true; } - final AbfsRestOperation op = client.createPath(relativePath, true, overwrite, - isNamespaceEnabled ? getOctalNotation(permission) : null, - isNamespaceEnabled ? getOctalNotation(umask) : null, - isAppendBlob); + // if "fs.azure.enable.conditional.create.overwrite" is enabled and + // is a create request with overwrite=true, create will follow different + // flow. + boolean triggerConditionalCreateOverwrite = false; + if (overwrite + && abfsConfiguration.isConditionalCreateOverwriteEnabled()) { + triggerConditionalCreateOverwrite = true; + } + + AbfsRestOperation op; + if (triggerConditionalCreateOverwrite) { + op = conditionalCreateOverwriteFile(relativePath, + statistics, + isNamespaceEnabled ? getOctalNotation(permission) : null, + isNamespaceEnabled ? getOctalNotation(umask) : null, + isAppendBlob + ); + + } else { + op = client.createPath(relativePath, true, + overwrite, + isNamespaceEnabled ? getOctalNotation(permission) : null, + isNamespaceEnabled ? getOctalNotation(umask) : null, + isAppendBlob, + null); + } perfInfo.registerResult(op.getResult()).registerSuccess(true); return new AbfsOutputStream( @@ -479,6 +502,74 @@ public OutputStream createFile(final Path path, } } + /** + * Conditional create overwrite flow ensures that create overwrites is done + * only if there is match for eTag of existing file. + * @param relativePath + * @param statistics + * @param permission + * @param umask + * @param isAppendBlob + * @return + * @throws AzureBlobFileSystemException + */ + private AbfsRestOperation conditionalCreateOverwriteFile(final String relativePath, + final FileSystem.Statistics statistics, + final String permission, + final String umask, + final boolean isAppendBlob) throws AzureBlobFileSystemException { + AbfsRestOperation op; + + try { + // Trigger a create with overwrite=false first so that eTag fetch can be + // avoided for cases when no pre-existing file is present (major portion + // of create file traffic falls into the case of no pre-existing file). + op = client.createPath(relativePath, true, + false, permission, umask, isAppendBlob, null); + } catch (AbfsRestOperationException e) { + if (e.getStatusCode() == HttpURLConnection.HTTP_CONFLICT) { + // File pre-exists, fetch eTag + try { + op = client.getPathStatus(relativePath, false); + } catch (AbfsRestOperationException ex) { + if (ex.getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) { + // Is a parallel access case, as file which was found to be + // present went missing by this request. + throw new ConcurrentWriteOperationDetectedException( + "Parallel access to the create path detected. Failing request " + + "to honor single writer semantics"); + } else { + throw ex; + } + } + + String eTag = op.getResult() + .getResponseHeader(HttpHeaderConfigurations.ETAG); + + try { + // overwrite only if eTag matches with the file properties fetched befpre + op = client.createPath(relativePath, true, + true, permission, umask, isAppendBlob, eTag); + } catch (AbfsRestOperationException ex) { + if (ex.getStatusCode() == HttpURLConnection.HTTP_PRECON_FAILED) { + // Is a parallel access case, as file with eTag was just queried + // and precondition failure can happen only when another file with + // different etag got created. + throw new ConcurrentWriteOperationDetectedException( + "Parallel access to the create path detected. Failing request " + + "to honor single writer semantics"); + } else { + throw ex; + } + } + } else { + throw e; + } + } + + return op; + } + private AbfsOutputStreamContext populateAbfsOutputStreamContext(boolean isAppendBlob) { int bufferSize = abfsConfiguration.getWriteBufferSize(); if (isAppendBlob && bufferSize > FileSystemConfigurations.APPENDBLOB_MAX_WRITE_BUFFER_SIZE) { @@ -508,7 +599,7 @@ public void createDirectory(final Path path, final FsPermission permission, fina final AbfsRestOperation op = client.createPath(getRelativePath(path), false, true, isNamespaceEnabled ? getOctalNotation(permission) : null, - isNamespaceEnabled ? getOctalNotation(umask) : null, false); + isNamespaceEnabled ? getOctalNotation(umask) : null, false, null); perfInfo.registerResult(op.getResult()).registerSuccess(true); } } diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java index 0adac8626f626..0415857745ba6 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java @@ -40,8 +40,6 @@ import org.slf4j.LoggerFactory; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AzureBlobFileSystemException; -import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AbfsRestOperationException; -import org.apache.hadoop.fs.azurebfs.contracts.exceptions.ConcurrentWriteOperationDetectedException; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.InvalidUriException; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.SASTokenProviderException; import org.apache.hadoop.fs.azurebfs.extensions.ExtensionHelper; @@ -265,102 +263,10 @@ public AbfsRestOperation deleteFilesystem() throws AzureBlobFileSystemException return op; } - public AbfsRestOperation createPath(final String path, - final boolean isFile, - final boolean overwrite, - final String permission, - final String umask, - final boolean isAppendBlob) throws AzureBlobFileSystemException { - String operation = isFile - ? SASTokenProvider.CREATE_FILE_OPERATION - : SASTokenProvider.CREATE_DIRECTORY_OPERATION; - - // if "fs.azure.enable.conditional.create.overwrite" is enabled, - // trigger a create with overwrite=false first so that eTag fetch can be - // avoided for cases when no pre-existing file is present (which is the - // case with most part of create traffic) - boolean isFirstAttemptToCreateWithoutOverwrite = false; - if (isFile && overwrite - && abfsConfiguration.isConditionalCreateOverwriteEnabled()) { - isFirstAttemptToCreateWithoutOverwrite = true; - } - - AbfsRestOperation op = null; - // Query builder - final AbfsUriQueryBuilder abfsUriQueryBuilder = createDefaultUriQueryBuilder(); - abfsUriQueryBuilder.addQuery(QUERY_PARAM_RESOURCE, - operation.equals(SASTokenProvider.CREATE_FILE_OPERATION) - ? FILE - : DIRECTORY); - - if (isAppendBlob) { - abfsUriQueryBuilder.addQuery(QUERY_PARAM_BLOBTYPE, APPEND_BLOB_TYPE); - } - - appendSASTokenToQuery(path, operation, abfsUriQueryBuilder); - - try { - op = createPathImpl(path, abfsUriQueryBuilder, - (isFirstAttemptToCreateWithoutOverwrite ? false : overwrite), - permission, umask, null); - } catch (AbfsRestOperationException e) { - if ((e.getStatusCode() == HttpURLConnection.HTTP_CONFLICT) - && isFirstAttemptToCreateWithoutOverwrite) { - // Was the first attempt made to create file without overwrite which - // failed because there is a pre-existing file. - // resetting the first attempt flag for readabiltiy - isFirstAttemptToCreateWithoutOverwrite = false; - - // Fetch eTag - try { - op = getPathStatus(path, false); - } catch (AbfsRestOperationException ex) { - if (ex.getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) { - // Is a parallel access case, as file which was found to be - // present went missing by this request. - throw new ConcurrentWriteOperationDetectedException( - "Parallel access to the create path detected. Failing request " - + "to honor single writer semantics"); - } else { - throw ex; - } - } - - String eTag = op.getResult().getResponseHeader(ETAG); - - try { - // overwrite only if eTag matches with the file properties fetched befpre - op = createPathImpl(path, abfsUriQueryBuilder, true, permission, - umask, eTag); - } catch (AbfsRestOperationException ex) { - if (ex.getStatusCode() == HttpURLConnection.HTTP_PRECON_FAILED) { - // Is a parallel access case, as file with eTag was just queried - // and precondition failure can happen only when another file with - // different etag got created. - throw new ConcurrentWriteOperationDetectedException( - "Parallel access to the create path detected. Failing request " - + "to honor single writer semantics"); - } else { - throw ex; - } - } - } else { - throw e; - } - } - - return op; - } - - @VisibleForTesting - public AbfsRestOperation createPathImpl(final String path, - AbfsUriQueryBuilder abfsUriQueryBuilder, - final boolean overwrite, - final String permission, - final String umask, - final String eTag) throws AzureBlobFileSystemException { + public AbfsRestOperation createPath(final String path, final boolean isFile, final boolean overwrite, + final String permission, final String umask, + final boolean isAppendBlob, final String eTag) throws AzureBlobFileSystemException { final List requestHeaders = createDefaultHeaders(); - if (!overwrite) { requestHeaders.add(new AbfsHttpHeader(IF_NONE_MATCH, AbfsHttpConstants.STAR)); } @@ -377,6 +283,17 @@ public AbfsRestOperation createPathImpl(final String path, requestHeaders.add(new AbfsHttpHeader(HttpHeaderConfigurations.IF_MATCH, eTag)); } + final AbfsUriQueryBuilder abfsUriQueryBuilder = createDefaultUriQueryBuilder(); + abfsUriQueryBuilder.addQuery(QUERY_PARAM_RESOURCE, isFile ? FILE : DIRECTORY); + if (isAppendBlob) { + abfsUriQueryBuilder.addQuery(QUERY_PARAM_BLOBTYPE, APPEND_BLOB_TYPE); + } + + String operation = isFile + ? SASTokenProvider.CREATE_FILE_OPERATION + : SASTokenProvider.CREATE_DIRECTORY_OPERATION; + appendSASTokenToQuery(path, operation, abfsUriQueryBuilder); + final URL url = createRequestUrl(path, abfsUriQueryBuilder.toString()); final AbfsRestOperation op = new AbfsRestOperation( AbfsRestOperationType.CreatePath, diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java index 76a528c7d1f49..a5c80ce9ceb5b 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java @@ -21,6 +21,7 @@ import java.io.FileNotFoundException; import java.io.FilterOutputStream; import java.io.IOException; +import java.lang.reflect.Field; import java.util.EnumSet; import java.util.UUID; @@ -35,11 +36,13 @@ import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.test.GenericTestUtils; +import org.apache.hadoop.fs.azurebfs.AzureBlobFileSystemStore; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AbfsRestOperationException; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.ConcurrentWriteOperationDetectedException; +import org.apache.hadoop.fs.azurebfs.services.AbfsClient; +import org.apache.hadoop.fs.azurebfs.services.TestAbfsClient; import org.apache.hadoop.fs.azurebfs.services.AbfsHttpOperation; import org.apache.hadoop.fs.azurebfs.services.AbfsRestOperation; -import org.apache.hadoop.fs.azurebfs.services.AbfsUriQueryBuilder; import static java.net.HttpURLConnection.HTTP_CONFLICT; import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR; @@ -335,12 +338,15 @@ public void testNegativeScenariosForCreateOverwriteDisabled() config); // Get mock AbfsClient with current config - org.apache.hadoop.fs.azurebfs.services.AbfsClient + AbfsClient mockClient - = org.apache.hadoop.fs.azurebfs.services.TestAbfsClient.getMockAbfsClient( + = TestAbfsClient.getMockAbfsClient( fs.getAbfsStore().getClient(), fs.getAbfsStore().getAbfsConfiguration()); + AzureBlobFileSystemStore abfsStore = fs.getAbfsStore(); + abfsStore = setAzureBlobSystemStoreField(abfsStore, "client", mockClient); + AbfsRestOperation successOp = mock( AbfsRestOperation.class); AbfsHttpOperation http200Op = mock( @@ -366,9 +372,8 @@ public void testNegativeScenariosForCreateOverwriteDisabled() .doThrow( serverErrorResponseEx) // Scn5: create overwrite=false fails with Http500 .when(mockClient) - .createPathImpl(any(String.class), any( - AbfsUriQueryBuilder.class), - eq(false), any(String.class), any(String.class), eq(null)); + .createPath(any(String.class), eq(true), eq(false), any(String.class), + any(String.class), any(boolean.class), eq(null)); doThrow(fileNotFoundResponseEx) // Scn1: GFS fails with Http404 .doThrow(serverErrorResponseEx) // Scn2: GFS fails with Http500 @@ -382,35 +387,23 @@ public void testNegativeScenariosForCreateOverwriteDisabled() .doThrow( serverErrorResponseEx) // Scn4: create overwrite=true fails with Http500 .when(mockClient) - .createPathImpl(any(String.class), any( - AbfsUriQueryBuilder.class), - eq(true), any(String.class), any(String.class), eq(null)); - - when(mockClient.createPath(any(String.class), eq(true), eq(true), - any(String.class), - any(String.class), eq(false))).thenCallRealMethod(); + .createPath(any(String.class), eq(true), eq(true), any(String.class), + any(String.class), any(boolean.class), eq(null)); // Scn1: GFS fails with Http404 // Sequence of events expected: // 1. create overwrite=false - fail with conflict // 2. GFS - fail with File Not found // Create will fail with ConcurrentWriteOperationDetectedException - intercept( - ConcurrentWriteOperationDetectedException.class, - () -> - mockClient.createPath("someTestPath", true, true, "0644", "0022", - false)); + validateCreateFileException(ConcurrentWriteOperationDetectedException.class, + abfsStore); // Scn2: GFS fails with Http500 // Sequence of events expected: // 1. create overwrite=false - fail with conflict // 2. GFS - fail with Server error // Create will fail with 500 - intercept( - AbfsRestOperationException.class, - () -> - mockClient.createPath("someTestPath", true, true, "0644", "0022", - false)); + validateCreateFileException(AbfsRestOperationException.class, abfsStore); // Scn3: create overwrite=true fails with Http412 // Sequence of events expected: @@ -418,11 +411,8 @@ public void testNegativeScenariosForCreateOverwriteDisabled() // 2. GFS - pass // 3. create overwrite=true - fail with Pre-Condition // Create will fail with ConcurrentWriteOperationDetectedException - intercept( - ConcurrentWriteOperationDetectedException.class, - () -> - mockClient.createPath("someTestPath", true, true, "0644", "0022", - false)); + validateCreateFileException(ConcurrentWriteOperationDetectedException.class, + abfsStore); // Scn4: create overwrite=true fails with Http500 // Sequence of events expected: @@ -430,21 +420,38 @@ public void testNegativeScenariosForCreateOverwriteDisabled() // 2. GFS - pass // 3. create overwrite=true - fail with Server error // Create will fail with 500 - intercept( - AbfsRestOperationException.class, - () -> - mockClient.createPath("someTestPath", true, true, "0644", "0022", - false)); + validateCreateFileException(AbfsRestOperationException.class, abfsStore); // Scn5: create overwrite=false fails with Http500 // Sequence of events expected: // 1. create overwrite=false - fail with server error // Create will fail with 500 + validateCreateFileException(AbfsRestOperationException.class, abfsStore); + } + + private AzureBlobFileSystemStore setAzureBlobSystemStoreField( + final AzureBlobFileSystemStore abfsStore, + final String fieldName, + Object fieldObject) throws Exception { + + Field abfsClientField = AzureBlobFileSystemStore.class.getDeclaredField(fieldName); + abfsClientField.setAccessible(true); + Field modifiersField = Field.class.getDeclaredField("modifiers"); + modifiersField.setAccessible(true); + modifiersField.setInt(abfsClientField, + abfsClientField.getModifiers() & ~java.lang.reflect.Modifier.FINAL); + abfsClientField.set(abfsStore, fieldObject); + return abfsStore; + } + + private void validateCreateFileException(final Class exceptionClass, final AzureBlobFileSystemStore abfsStore) + throws Exception { + FsPermission permission = new FsPermission(0644); + FsPermission umask = new FsPermission(0022); + Path testPath = new Path("testFile"); intercept( - AbfsRestOperationException.class, - () -> - mockClient.createPath("someTestPath", true, true, "0644", "0022", - false)); + exceptionClass, + () -> abfsStore.createFile(testPath, null, true, permission, umask)); } private AbfsRestOperationException getMockAbfsRestOperationException(int status) { diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java index 4620642373ed6..de476a6abce9b 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemMkDir.java @@ -47,15 +47,15 @@ public void testCreateDirWithExistingDir() throws Exception { assertMkdirs(fs, path); } - /** - * Test mkdir for possible values of fs.azure.disable.default.create.overwrite - * @throws Exception - */ @Test public void testCreateRoot() throws Exception { assertMkdirs(getFileSystem(), new Path("/")); } + /** + * Test mkdir for possible values of fs.azure.disable.default.create.overwrite + * @throws Exception + */ @Test public void testDefaultCreateOverwriteDirTest() throws Throwable { // the config fs.azure.disable.default.create.overwrite should have no diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java index 2badbfa796e61..7a7992d9bb475 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java @@ -283,8 +283,7 @@ public static AbfsClient createTestClientFromCurrentContext( } public static AbfsClient getMockAbfsClient(AbfsClient baseAbfsClientInstance, - AbfsConfiguration abfsConfig) - throws IOException, NoSuchFieldException, IllegalAccessException { + AbfsConfiguration abfsConfig) throws Exception { AuthType currentAuthType = abfsConfig.getAuthType( abfsConfig.getAccountName()); @@ -310,56 +309,46 @@ public static AbfsClient getMockAbfsClient(AbfsClient baseAbfsClientInstance, when(client.createDefaultHeaders()).thenCallRealMethod(); // override baseurl - Field abfsConfigurationField = AbfsClient.class.getDeclaredField("abfsConfiguration"); - abfsConfigurationField.setAccessible(true); - Field modifiersField = Field.class.getDeclaredField("modifiers"); - modifiersField.setAccessible(true); - modifiersField.setInt(abfsConfigurationField, - abfsConfigurationField.getModifiers() - & ~java.lang.reflect.Modifier.FINAL); - abfsConfigurationField.set(client, abfsConfig); + client = TestAbfsClient.setAbfsClientField(client, "abfsConfiguration", + abfsConfig); // override baseurl - Field baseUrlField = AbfsClient.class.getDeclaredField("baseUrl"); - baseUrlField.setAccessible(true); - modifiersField.setAccessible(true); - modifiersField.setInt(baseUrlField, baseUrlField.getModifiers() & ~java.lang.reflect.Modifier.FINAL); - baseUrlField.set(client, baseAbfsClientInstance.getBaseUrl()); + client = TestAbfsClient.setAbfsClientField(client, "baseUrl", + baseAbfsClientInstance.getBaseUrl()); // override auth provider if (currentAuthType == AuthType.SharedKey) { - Field sharedKeyCredsField = AbfsClient.class.getDeclaredField( - "sharedKeyCredentials"); - sharedKeyCredsField.setAccessible(true); - modifiersField.setInt(sharedKeyCredsField, - sharedKeyCredsField.getModifiers() - & ~java.lang.reflect.Modifier.FINAL); - sharedKeyCredsField.set(client, new SharedKeyCredentials( - abfsConfig.getAccountName().substring(0, - abfsConfig.getAccountName().indexOf(DOT)), - abfsConfig.getStorageAccountKey())); + client = TestAbfsClient.setAbfsClientField(client, "sharedKeyCredentials", + new SharedKeyCredentials( + abfsConfig.getAccountName().substring(0, + abfsConfig.getAccountName().indexOf(DOT)), + abfsConfig.getStorageAccountKey())); } else { - Field tokenProviderField = AbfsClient.class.getDeclaredField( - "tokenProvider"); - tokenProviderField.setAccessible(true); - modifiersField.setInt(tokenProviderField, - tokenProviderField.getModifiers() - & ~java.lang.reflect.Modifier.FINAL); - tokenProviderField.set(client, abfsConfig.getTokenProvider()); + client = TestAbfsClient.setAbfsClientField(client, "tokenProvider", + abfsConfig.getTokenProvider()); } // override user agent String userAgent = "APN/1.0 Azure Blob FS/3.4.0-SNAPSHOT (PrivateBuild " + "JavaJRE 1.8.0_252; Linux 5.3.0-59-generic/amd64; openssl-1.0; " + "UNKNOWN/UNKNOWN) MSFT"; - Field userAgentField = AbfsClient.class.getDeclaredField( - "userAgent"); - userAgentField.setAccessible(true); - modifiersField.setInt(userAgentField, - userAgentField.getModifiers() - & ~java.lang.reflect.Modifier.FINAL); - userAgentField.set(client, userAgent); + client = TestAbfsClient.setAbfsClientField(client, "userAgent", userAgent); return client; } + + private static AbfsClient setAbfsClientField( + final AbfsClient client, + final String fieldName, + Object fieldObject) throws Exception { + + Field field = AbfsClient.class.getDeclaredField(fieldName); + field.setAccessible(true); + Field modifiersField = Field.class.getDeclaredField("modifiers"); + modifiersField.setAccessible(true); + modifiersField.setInt(field, + field.getModifiers() & ~java.lang.reflect.Modifier.FINAL); + field.set(client, fieldObject); + return client; + } } From 5783bc12448dee9b86d004651e5b520326046505 Mon Sep 17 00:00:00 2001 From: Sneha Vijayarajan Date: Fri, 18 Sep 2020 22:58:35 +0530 Subject: [PATCH 11/11] Fixing local yetus - checkstyle issues --- .../fs/azurebfs/ITestAzureBlobFileSystemCreate.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java index a5c80ce9ceb5b..981ed25cee75b 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java @@ -33,10 +33,10 @@ import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.FSDataOutputStream; import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.permission.FsAction; import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.test.GenericTestUtils; -import org.apache.hadoop.fs.azurebfs.AzureBlobFileSystemStore; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AbfsRestOperationException; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.ConcurrentWriteOperationDetectedException; import org.apache.hadoop.fs.azurebfs.services.AbfsClient; @@ -434,7 +434,8 @@ private AzureBlobFileSystemStore setAzureBlobSystemStoreField( final String fieldName, Object fieldObject) throws Exception { - Field abfsClientField = AzureBlobFileSystemStore.class.getDeclaredField(fieldName); + Field abfsClientField = AzureBlobFileSystemStore.class.getDeclaredField( + fieldName); abfsClientField.setAccessible(true); Field modifiersField = Field.class.getDeclaredField("modifiers"); modifiersField.setAccessible(true); @@ -446,8 +447,10 @@ private AzureBlobFileSystemStore setAzureBlobSystemStoreField( private void validateCreateFileException(final Class exceptionClass, final AzureBlobFileSystemStore abfsStore) throws Exception { - FsPermission permission = new FsPermission(0644); - FsPermission umask = new FsPermission(0022); + FsPermission permission = new FsPermission(FsAction.ALL, FsAction.ALL, + FsAction.ALL); + FsPermission umask = new FsPermission(FsAction.NONE, FsAction.NONE, + FsAction.NONE); Path testPath = new Path("testFile"); intercept( exceptionClass,