From 9123ad05d36358a0057a2d63c20e581c9780a8d0 Mon Sep 17 00:00:00 2001 From: Anuj Modi Date: Wed, 19 Jun 2024 22:07:05 -0700 Subject: [PATCH 1/5] Refactor getAcl --- .../fs/azurebfs/AzureBlobFileSystemStore.java | 14 +++++--- .../fs/azurebfs/ITestGetNameSpaceEnabled.java | 32 +++++++++++++++++++ 2 files changed, 42 insertions(+), 4 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 ac564f082c9e4..4e79ebdc79e8e 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 @@ -395,15 +395,21 @@ private synchronized boolean getNamespaceEnabledInformationFromServer( try { LOG.debug("Get root ACL status"); getClient().getAclStatus(AbfsHttpConstants.ROOT_PATH, tracingContext); + // If getAcl succeeds, namespace is enabled. isNamespaceEnabled = Trilean.getTrilean(true); } catch (AbfsRestOperationException ex) { - // Get ACL status is a HEAD request, its response doesn't contain - // errorCode + // Get ACL status is a HEAD request, its response doesn't contain errorCode // So can only rely on its status code to determine its account type. - if (HttpURLConnection.HTTP_BAD_REQUEST != ex.getStatusCode()) { + if (HttpURLConnection.HTTP_BAD_REQUEST == ex.getStatusCode()) { + // If getAcl fails with 400, namespace is disabled. + isNamespaceEnabled = Trilean.getTrilean(false); + } else if (HttpURLConnection.HTTP_NOT_FOUND == ex.getStatusCode()) { + // If getAcl fails with 404, namespace is enabled. + isNamespaceEnabled = Trilean.getTrilean(true); + } else { + // Any other server error, throw exception. throw ex; } - isNamespaceEnabled = Trilean.getTrilean(false); } catch (AzureBlobFileSystemException ex) { throw ex; } diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestGetNameSpaceEnabled.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestGetNameSpaceEnabled.java index b40e317d2e32d..7660569a50cc4 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestGetNameSpaceEnabled.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestGetNameSpaceEnabled.java @@ -24,19 +24,23 @@ import org.junit.Assume; import org.junit.Test; import org.assertj.core.api.Assertions; +import org.mockito.Mockito; import org.apache.hadoop.fs.CommonConfigurationKeysPublic; import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AbfsRestOperationException; import org.apache.hadoop.fs.azurebfs.services.AbfsClient; import org.apache.hadoop.fs.azurebfs.services.AbfsRestOperation; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.azurebfs.enums.Trilean; import org.apache.hadoop.fs.azurebfs.utils.TracingContext; +import org.apache.kerby.config.Conf; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -48,6 +52,7 @@ import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_ACCOUNT_IS_HNS_ENABLED; import static org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.FS_AZURE_TEST_NAMESPACE_ENABLED_ACCOUNT; import static org.apache.hadoop.test.LambdaTestUtils.intercept; +import static org.mockito.Mockito.when; /** * Test getIsNamespaceEnabled call. @@ -217,4 +222,31 @@ private AbfsClient callAbfsGetIsNamespaceEnabledAndReturnMockAbfsClient() return mockClient; } + @Test + public void ensureGetAclDetermineHnsStatusAccurately() throws Exception { + ensureGetAclDetermineHnsStatusAccuratelyInternal(400, false); + ensureGetAclDetermineHnsStatusAccuratelyInternal(404, true); + ensureGetAclDetermineHnsStatusAccuratelyInternal(500, false); + ensureGetAclDetermineHnsStatusAccuratelyInternal(503, false); + } + + private void ensureGetAclDetermineHnsStatusAccuratelyInternal(int statusCode, + boolean expected) throws Exception { + AzureBlobFileSystemStore store = Mockito.spy(getFileSystem().getAbfsStore()); + AbfsClient mockClient = mock(AbfsClient.class); + store.setNamespaceEnabled(Trilean.UNKNOWN); + doReturn(mockClient).when(store).getClient(); + AbfsRestOperationException ex = new AbfsRestOperationException( + statusCode, null, Integer.toString(statusCode), null); + doThrow(ex).when(mockClient).getAclStatus(anyString(), any(TracingContext.class)); + + try { + boolean isHnsEnabled = store.getIsNamespaceEnabled( + getTestTracingContext(getFileSystem(), false)); + Assertions.assertThat(isHnsEnabled).isEqualTo(expected); + } catch (AbfsRestOperationException caughtEx) { + Assertions.assertThat(caughtEx.getStatusCode()).isEqualTo(statusCode); + Assertions.assertThat(caughtEx.getErrorMessage()).isEqualTo(ex.getErrorMessage()); + } + } } From c936ec5591b9474fec335af736f0c6e5cb1b503f Mon Sep 17 00:00:00 2001 From: Anuj Modi Date: Wed, 19 Jun 2024 23:07:13 -0700 Subject: [PATCH 2/5] Test Fix --- .../fs/azurebfs/ITestAzureBlobFileSystemInitAndCreate.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemInitAndCreate.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemInitAndCreate.java index dcd73cc3e982a..1ae9d4d7671be 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemInitAndCreate.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemInitAndCreate.java @@ -20,6 +20,7 @@ import java.io.FileNotFoundException; +import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; @@ -28,10 +29,14 @@ import org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys; import org.apache.hadoop.fs.azurebfs.contracts.exceptions.TrileanConversionException; +import org.apache.hadoop.fs.azurebfs.enums.Trilean; import org.apache.hadoop.fs.azurebfs.services.AbfsClient; import org.apache.hadoop.fs.azurebfs.services.AbfsRestOperation; import org.apache.hadoop.fs.azurebfs.utils.TracingContext; +import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_ACCOUNT_IS_HNS_ENABLED; +import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.accountProperty; + /** * Test filesystem initialization and creation. */ @@ -67,6 +72,7 @@ public void testGetAclCallOnHnsConfigAbsence() throws Exception { Mockito.doThrow(TrileanConversionException.class) .when(store) .isNamespaceEnabled(); + store.setNamespaceEnabled(Trilean.UNKNOWN); TracingContext tracingContext = getSampleTracingContext(fs, true); Mockito.doReturn(Mockito.mock(AbfsRestOperation.class)) From 7d896cd0f50716a2faa20da9537e32bcc672f118 Mon Sep 17 00:00:00 2001 From: Anuj Modi Date: Thu, 20 Jun 2024 01:59:39 -0700 Subject: [PATCH 3/5] Checkstyle Fix --- .../fs/azurebfs/ITestGetNameSpaceEnabled.java | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestGetNameSpaceEnabled.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestGetNameSpaceEnabled.java index 7660569a50cc4..054f575730372 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestGetNameSpaceEnabled.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestGetNameSpaceEnabled.java @@ -19,6 +19,7 @@ import java.io.FileNotFoundException; import java.io.IOException; +import java.net.HttpURLConnection; import java.util.UUID; import org.junit.Assume; @@ -35,8 +36,12 @@ import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.azurebfs.enums.Trilean; import org.apache.hadoop.fs.azurebfs.utils.TracingContext; -import org.apache.kerby.config.Conf; +import static java.net.HttpURLConnection.HTTP_BAD_REQUEST; +import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR; +import static java.net.HttpURLConnection.HTTP_NOT_FOUND; +import static java.net.HttpURLConnection.HTTP_SERVER_ERROR; +import static java.net.HttpURLConnection.HTTP_UNAVAILABLE; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.doReturn; @@ -52,7 +57,6 @@ import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_ACCOUNT_IS_HNS_ENABLED; import static org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.FS_AZURE_TEST_NAMESPACE_ENABLED_ACCOUNT; import static org.apache.hadoop.test.LambdaTestUtils.intercept; -import static org.mockito.Mockito.when; /** * Test getIsNamespaceEnabled call. @@ -224,10 +228,10 @@ private AbfsClient callAbfsGetIsNamespaceEnabledAndReturnMockAbfsClient() @Test public void ensureGetAclDetermineHnsStatusAccurately() throws Exception { - ensureGetAclDetermineHnsStatusAccuratelyInternal(400, false); - ensureGetAclDetermineHnsStatusAccuratelyInternal(404, true); - ensureGetAclDetermineHnsStatusAccuratelyInternal(500, false); - ensureGetAclDetermineHnsStatusAccuratelyInternal(503, false); + ensureGetAclDetermineHnsStatusAccuratelyInternal(HTTP_BAD_REQUEST, false); + ensureGetAclDetermineHnsStatusAccuratelyInternal(HTTP_NOT_FOUND, true); + ensureGetAclDetermineHnsStatusAccuratelyInternal(HTTP_INTERNAL_ERROR, false); + ensureGetAclDetermineHnsStatusAccuratelyInternal(HTTP_UNAVAILABLE, false); } private void ensureGetAclDetermineHnsStatusAccuratelyInternal(int statusCode, From 9c1698bc9bfd1e9d32b7ee5c00b1fba2fd6e73bd Mon Sep 17 00:00:00 2001 From: Anuj Modi Date: Sun, 30 Jun 2024 22:46:39 -0700 Subject: [PATCH 4/5] Fixing Test and Prod Logic --- .../fs/azurebfs/AzureBlobFileSystemStore.java | 14 +++---- ...ITestAzureBlobFileSystemInitAndCreate.java | 4 -- .../fs/azurebfs/ITestGetNameSpaceEnabled.java | 40 ++++++++++++------- 3 files changed, 32 insertions(+), 26 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 4e79ebdc79e8e..25d169bc5eaf0 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 @@ -399,17 +399,15 @@ private synchronized boolean getNamespaceEnabledInformationFromServer( isNamespaceEnabled = Trilean.getTrilean(true); } catch (AbfsRestOperationException ex) { // Get ACL status is a HEAD request, its response doesn't contain errorCode - // So can only rely on its status code to determine its account type. - if (HttpURLConnection.HTTP_BAD_REQUEST == ex.getStatusCode()) { - // If getAcl fails with 400, namespace is disabled. - isNamespaceEnabled = Trilean.getTrilean(false); - } else if (HttpURLConnection.HTTP_NOT_FOUND == ex.getStatusCode()) { - // If getAcl fails with 404, namespace is enabled. + // So can only rely on its status code to determine account type. + if (HttpURLConnection.HTTP_BAD_REQUEST != ex.getStatusCode()) { + // If getAcl fails with anything other than 400, namespace is enabled. isNamespaceEnabled = Trilean.getTrilean(true); - } else { - // Any other server error, throw exception. + // Continue to throw exception as earlier. throw ex; } + // If getAcl fails with 400, namespace is disabled. + isNamespaceEnabled = Trilean.getTrilean(false); } catch (AzureBlobFileSystemException ex) { throw ex; } diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemInitAndCreate.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemInitAndCreate.java index 1ae9d4d7671be..1ff3458fdbaac 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemInitAndCreate.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemInitAndCreate.java @@ -20,7 +20,6 @@ import java.io.FileNotFoundException; -import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; @@ -34,9 +33,6 @@ import org.apache.hadoop.fs.azurebfs.services.AbfsRestOperation; import org.apache.hadoop.fs.azurebfs.utils.TracingContext; -import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_ACCOUNT_IS_HNS_ENABLED; -import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.accountProperty; - /** * Test filesystem initialization and creation. */ diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestGetNameSpaceEnabled.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestGetNameSpaceEnabled.java index 054f575730372..d168ed38844df 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestGetNameSpaceEnabled.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestGetNameSpaceEnabled.java @@ -19,7 +19,6 @@ import java.io.FileNotFoundException; import java.io.IOException; -import java.net.HttpURLConnection; import java.util.UUID; import org.junit.Assume; @@ -40,7 +39,6 @@ import static java.net.HttpURLConnection.HTTP_BAD_REQUEST; import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR; import static java.net.HttpURLConnection.HTTP_NOT_FOUND; -import static java.net.HttpURLConnection.HTTP_SERVER_ERROR; import static java.net.HttpURLConnection.HTTP_UNAVAILABLE; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; @@ -228,14 +226,18 @@ private AbfsClient callAbfsGetIsNamespaceEnabledAndReturnMockAbfsClient() @Test public void ensureGetAclDetermineHnsStatusAccurately() throws Exception { - ensureGetAclDetermineHnsStatusAccuratelyInternal(HTTP_BAD_REQUEST, false); - ensureGetAclDetermineHnsStatusAccuratelyInternal(HTTP_NOT_FOUND, true); - ensureGetAclDetermineHnsStatusAccuratelyInternal(HTTP_INTERNAL_ERROR, false); - ensureGetAclDetermineHnsStatusAccuratelyInternal(HTTP_UNAVAILABLE, false); + ensureGetAclDetermineHnsStatusAccuratelyInternal(HTTP_BAD_REQUEST, + false, false); + ensureGetAclDetermineHnsStatusAccuratelyInternal(HTTP_NOT_FOUND, + true, true); + ensureGetAclDetermineHnsStatusAccuratelyInternal(HTTP_INTERNAL_ERROR, + true, true); + ensureGetAclDetermineHnsStatusAccuratelyInternal(HTTP_UNAVAILABLE, + true, true); } private void ensureGetAclDetermineHnsStatusAccuratelyInternal(int statusCode, - boolean expected) throws Exception { + boolean expectedValue, boolean isExceptionExpected) throws Exception { AzureBlobFileSystemStore store = Mockito.spy(getFileSystem().getAbfsStore()); AbfsClient mockClient = mock(AbfsClient.class); store.setNamespaceEnabled(Trilean.UNKNOWN); @@ -244,13 +246,23 @@ private void ensureGetAclDetermineHnsStatusAccuratelyInternal(int statusCode, statusCode, null, Integer.toString(statusCode), null); doThrow(ex).when(mockClient).getAclStatus(anyString(), any(TracingContext.class)); - try { - boolean isHnsEnabled = store.getIsNamespaceEnabled( - getTestTracingContext(getFileSystem(), false)); - Assertions.assertThat(isHnsEnabled).isEqualTo(expected); - } catch (AbfsRestOperationException caughtEx) { - Assertions.assertThat(caughtEx.getStatusCode()).isEqualTo(statusCode); - Assertions.assertThat(caughtEx.getErrorMessage()).isEqualTo(ex.getErrorMessage()); + if (isExceptionExpected) { + try { + store.getIsNamespaceEnabled(getTestTracingContext(getFileSystem(), false)); + Assertions.fail( + "Exception Should have been thrown with status code: " + statusCode); + } catch (AbfsRestOperationException caughtEx) { + Assertions.assertThat(caughtEx.getStatusCode()).isEqualTo(statusCode); + Assertions.assertThat(caughtEx.getErrorMessage()).isEqualTo(ex.getErrorMessage()); + } } + // This should not trigger extra getAcl() call in case of exceptions. + boolean isHnsEnabled = store.getIsNamespaceEnabled( + getTestTracingContext(getFileSystem(), false)); + Assertions.assertThat(isHnsEnabled).isEqualTo(expectedValue); + + // GetAcl() should be called only once to determine the HNS status. + Mockito.verify(mockClient, times(1)) + .getAclStatus(anyString(), any(TracingContext.class)); } } From 51f03ff1f52b51f78a757215ef668c12f3eb762a Mon Sep 17 00:00:00 2001 From: Anuj Modi Date: Wed, 10 Jul 2024 21:43:09 -0700 Subject: [PATCH 5/5] Debug Logs --- .../apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java | 3 +++ 1 file changed, 3 insertions(+) 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 25d169bc5eaf0..449b123d9212a 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 @@ -404,9 +404,12 @@ private synchronized boolean getNamespaceEnabledInformationFromServer( // If getAcl fails with anything other than 400, namespace is enabled. isNamespaceEnabled = Trilean.getTrilean(true); // Continue to throw exception as earlier. + LOG.debug("Failed to get ACL status with non 400. Inferring namespace enabled", ex); throw ex; } // If getAcl fails with 400, namespace is disabled. + LOG.debug("Failed to get ACL status with 400. " + + "Inferring namespace disabled and ignoring error", ex); isNamespaceEnabled = Trilean.getTrilean(false); } catch (AzureBlobFileSystemException ex) { throw ex;