Skip to content

HADOOP-19284: [ABFS] Allow "fs.azure.account.hns.enabled" to be set as Account Specific Config #7062

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
surendralilhore merged 6 commits into from
Sep 27, 2024
Merged

HADOOP-19284: [ABFS] Allow "fs.azure.account.hns.enabled" to be set as Account Specific Config #7062

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
74cd5dc
Making is HNS enabled account specific setting
Sep 23, 2024
ba947aa
Fixing Failing tests
Sep 23, 2024
ad926a3
Updating Tests
Sep 23, 2024
52f928d
Resolving Comments
Sep 26, 2024
cc45f99
Resolving Comments
Sep 26, 2024
f3d371f
Yetus Checks
Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,8 +450,17 @@ public AbfsConfiguration(final Configuration rawConfig, String accountName)
this(rawConfig, accountName, AbfsServiceType.DFS);
}

/**
* Returns the account type as per the user configuration. Gets the account
* specific value if it exists, then looks for an account agnostic value.
* If not configured driver makes additional getAcl call to determine
* the account type during file system initialization.
* @return TRUE/FALSE value if configured, UNKNOWN if not configured.
*/
public Trilean getIsNamespaceEnabledAccount() {
return Trilean.getTrilean(isNamespaceEnabledAccount);
String isNamespaceEnabledAccountString
Comment thread
surendralilhore marked this conversation as resolved.
Outdated
= getString(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, isNamespaceEnabledAccount);
return Trilean.getTrilean(isNamespaceEnabledAccountString);

@bhattmanish98 bhattmanish98 Sep 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the variable isNamespaceEnabledAccountString is used at one place only, shouldn't it be better to make it inplace call instead of creating new variable?

@anujmodi2021 anujmodi2021 Sep 26, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that sounds better

}

/**
Expand Down
2 changes: 1 addition & 1 deletion ...ls/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ private synchronized boolean getNamespaceEnabledInformationFromServer(
}
try {
LOG.debug("Get root ACL status");
getClient().getAclStatus(AbfsHttpConstants.ROOT_PATH, tracingContext);
getClient(AbfsServiceType.DFS).getAclStatus(AbfsHttpConstants.ROOT_PATH, tracingContext);
// If getAcl succeeds, namespace is enabled.
isNamespaceEnabled = Trilean.getTrilean(true);
} catch (AbfsRestOperationException ex) {
Expand Down
4 changes: 4 additions & 0 deletions ...hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemE2E.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.AZURE_HTTP_READ_TIMEOUT;
import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.AZURE_MAX_IO_RETRIES;
import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.AZURE_TOLERATE_CONCURRENT_APPEND;
import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_ACCOUNT_IS_HNS_ENABLED;
import static org.apache.hadoop.fs.contract.ContractTestUtils.assertPathDoesNotExist;
import static org.apache.hadoop.fs.contract.ContractTestUtils.assertPathExists;
import static org.apache.hadoop.test.LambdaTestUtils.intercept;
Expand Down Expand Up @@ -259,6 +260,9 @@ public void testHttpReadTimeout() throws Exception {

public void testHttpTimeouts(int connectionTimeoutMs, int readTimeoutMs)
throws Exception {
// This is to make sure File System creation goes through before network calls start failing.
assumeValidTestConfigPresent(this.getRawConfiguration(), FS_AZURE_ACCOUNT_IS_HNS_ENABLED);
Comment thread
surendralilhore marked this conversation as resolved.

Configuration conf = this.getRawConfiguration();
// set to small values that will cause timeouts
conf.setInt(AZURE_HTTP_CONNECTION_TIMEOUT, connectionTimeoutMs);
Expand Down
3 changes: 2 additions & 1 deletion ...re/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemInitAndCreate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.junit.Test;
import org.mockito.Mockito;

import org.apache.hadoop.fs.azurebfs.constants.AbfsServiceType;
import org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys;
import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AbfsRestOperationException;
import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AzureBlobFileSystemException;
Expand Down Expand Up @@ -76,7 +77,7 @@ public void testGetAclCallOnHnsConfigAbsence() throws Exception {
getRawConfiguration()));
AzureBlobFileSystemStore store = Mockito.spy(fs.getAbfsStore());
AbfsClient client = Mockito.spy(fs.getAbfsClient());
Mockito.doReturn(client).when(store).getClient();
Mockito.doReturn(client).when(store).getClient(AbfsServiceType.DFS);

Mockito.doThrow(TrileanConversionException.class)
.when(store)
Expand Down
54 changes: 53 additions & 1 deletion ...ls/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestGetNameSpaceEnabled.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

import org.apache.hadoop.fs.CommonConfigurationKeysPublic;
import org.apache.hadoop.fs.FileSystem;
import org.apache.hadoop.fs.azurebfs.constants.AbfsServiceType;
import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AbfsRestOperationException;
import org.apache.hadoop.fs.azurebfs.services.AbfsClient;
import org.apache.hadoop.fs.azurebfs.services.AbfsDfsClient;
Expand All @@ -41,6 +42,7 @@
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.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.accountProperty;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.doReturn;
Expand Down Expand Up @@ -126,6 +128,8 @@ private AzureBlobFileSystem getNewFSWithHnsConf(
Configuration rawConfig = new Configuration();
rawConfig.addResource(TEST_CONFIGURATION_FILE_NAME);
rawConfig.set(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, isNamespaceEnabledAccount);
rawConfig.set(accountProperty(FS_AZURE_ACCOUNT_IS_HNS_ENABLED,
this.getAccountName()), isNamespaceEnabledAccount);
rawConfig
.setBoolean(AZURE_CREATE_REMOTE_FILESYSTEM_DURING_INITIALIZATION, true);
rawConfig.set(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY,
Expand Down Expand Up @@ -247,7 +251,7 @@ private void ensureGetAclDetermineHnsStatusAccuratelyInternal(int statusCode,
AzureBlobFileSystemStore store = Mockito.spy(getFileSystem().getAbfsStore());
AbfsClient mockClient = mock(AbfsClient.class);
store.setNamespaceEnabled(Trilean.UNKNOWN);
doReturn(mockClient).when(store).getClient();
doReturn(mockClient).when(store).getClient(AbfsServiceType.DFS);
AbfsRestOperationException ex = new AbfsRestOperationException(
statusCode, null, Integer.toString(statusCode), null);
doThrow(ex).when(mockClient).getAclStatus(anyString(), any(TracingContext.class));
Expand All @@ -271,4 +275,52 @@ private void ensureGetAclDetermineHnsStatusAccuratelyInternal(int statusCode,
Mockito.verify(mockClient, times(1))
.getAclStatus(anyString(), any(TracingContext.class));
}

@Test
public void testAccountSpecificConfig() throws Exception {
Configuration rawConfig = new Configuration();
rawConfig.addResource(TEST_CONFIGURATION_FILE_NAME);
rawConfig.unset(FS_AZURE_ACCOUNT_IS_HNS_ENABLED);
rawConfig.unset(accountProperty(FS_AZURE_ACCOUNT_IS_HNS_ENABLED,
this.getAccountName()));
String accountName1 = "account1.dfs.core.windows.net";
String accountName2 = "account2.dfs.core.windows.net";
String accountName3 = "account3.dfs.core.windows.net";
String defaultUri1 = this.getTestUrl().replace(this.getAccountName(), accountName1);
String defaultUri2 = this.getTestUrl().replace(this.getAccountName(), accountName2);
String defaultUri3 = this.getTestUrl().replace(this.getAccountName(), accountName3);

// Set both account specific and account agnostic config for account 1
rawConfig.set(accountProperty(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, accountName1), FALSE_STR);
rawConfig.set(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, TRUE_STR);
rawConfig.set(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, defaultUri1);
AzureBlobFileSystem fs1 = (AzureBlobFileSystem) FileSystem.newInstance(rawConfig);
Comment thread
surendralilhore marked this conversation as resolved.
Outdated
// Assert that account specific config takes precedence
Assertions.assertThat(getIsNamespaceEnabled(fs1)).describedAs(
"getIsNamespaceEnabled should return true when the "
+ "account specific config is set as true").isFalse();

// Set only the account specific config for account 2
rawConfig.set(accountProperty(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, accountName2), FALSE_STR);
rawConfig.unset(FS_AZURE_ACCOUNT_IS_HNS_ENABLED);
rawConfig.set(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, defaultUri2);
AzureBlobFileSystem fs2 = (AzureBlobFileSystem) FileSystem.newInstance(rawConfig);
// Assert that account specific config is enough.
Assertions.assertThat(getIsNamespaceEnabled(fs2)).describedAs(
"getIsNamespaceEnabled should return true when the "
+ "account specific config is set as true").isFalse();

// Set only account agnostic config for account 3
rawConfig.set(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, FALSE_STR);
rawConfig.unset(accountProperty(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, accountName3));
rawConfig.set(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, defaultUri3);
AzureBlobFileSystem fs3 = (AzureBlobFileSystem) FileSystem.newInstance(rawConfig);
// Assert that account agnostic config is enough.
Assertions.assertThat(getIsNamespaceEnabled(fs3)).describedAs(

@anmolanmol1234 anmolanmol1234 Sep 23, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion statement should be changed right ? If the config is not set it does getAcl to determine the account type

@anujmodi2021 anujmodi2021 Sep 26, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this comment was before my latest commit. So, it might be not applicable now.
But still, we are setting account-agnostic setting here so getAcl won't be needed.

"getIsNamespaceEnabled should return true when the "
+ "account specific config is not set").isFalse();
fs1.close();
fs2.close();
fs3.close();
Comment thread
surendralilhore marked this conversation as resolved.
Outdated
}
Comment thread
anujmodi2021 marked this conversation as resolved.
}