HADOOP-19208: [ABFS] Fixing logic to determine HNS nature of account to avoid extra getAcl() calls (#6893) (#6946)

Contributed by Anuj Modi

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

@@ -395,14 +395,21 @@ public class AzureBlobFileSystemStore implements Closeable, ListingSupport {
     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
-      // So can only rely on its status code to determine its account type.
+      // Get ACL status is a HEAD request, its response doesn't contain errorCode
+      // 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);
+        // 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;

hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemInitAndCreate.java

@@ -28,6 +28,7 @@ import org.mockito.Mockito;
 
 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;
@@ -67,6 +68,7 @@ public class ITestAzureBlobFileSystemInitAndCreate extends
     Mockito.doThrow(TrileanConversionException.class)
         .when(store)
         .isNamespaceEnabled();
+    store.setNamespaceEnabled(Trilean.UNKNOWN);
 
     TracingContext tracingContext = getSampleTracingContext(fs, true);
     Mockito.doReturn(Mockito.mock(AbfsRestOperation.class))

hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestGetNameSpaceEnabled.java

@@ -24,9 +24,11 @@ import java.util.UUID;
 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;
@@ -34,9 +36,14 @@ import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.fs.azurebfs.enums.Trilean;
 import org.apache.hadoop.fs.azurebfs.utils.TracingContext;
 
+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_UNAVAILABLE;
 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;
@@ -217,4 +224,45 @@ public class ITestGetNameSpaceEnabled extends AbstractAbfsIntegrationTest {
     return mockClient;
   }
 
+  @Test
+  public void ensureGetAclDetermineHnsStatusAccurately() throws Exception {
+    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 expectedValue, boolean isExceptionExpected) 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));
+
+    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));
+  }
 }