Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ public class AbfsConfiguration{
private final AbfsServiceType fsConfiguredServiceType;
private final boolean isSecure;
private static final Logger LOG = LoggerFactory.getLogger(AbfsConfiguration.class);
private Trilean isNamespaceEnabled = null;

@StringConfigurationValidatorAnnotation(ConfigurationKey = FS_AZURE_ACCOUNT_IS_HNS_ENABLED,
DefaultValue = DEFAULT_FS_AZURE_ACCOUNT_IS_HNS_ENABLED)
Expand Down Expand Up @@ -525,8 +526,11 @@ public AbfsConfiguration(final Configuration rawConfig, String accountName)
* @return TRUE/FALSE value if configured, UNKNOWN if not configured.
*/
public Trilean getIsNamespaceEnabledAccount() {
return Trilean.getTrilean(
getString(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, isNamespaceEnabledAccount));
if (isNamespaceEnabled == null) {
isNamespaceEnabled = Trilean.getTrilean(
getString(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, isNamespaceEnabledAccount));
}
return isNamespaceEnabled;
}

/**
Expand Down Expand Up @@ -1525,8 +1529,8 @@ void setMaxBackoffIntervalMilliseconds(int maxBackoffInterval) {
}

@VisibleForTesting
void setIsNamespaceEnabledAccount(String isNamespaceEnabledAccount) {
this.isNamespaceEnabledAccount = isNamespaceEnabledAccount;
public void setIsNamespaceEnabledAccount(Trilean isNamespaceEnabledAccount) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be setting this value on abfsconfiguration object only when we know the exact value. Do we really need this to be Trilean? I think we should pass the definitive argument and do the Boolean to Trilean conversion here just to make sure that no one accidently sets UNKNOWN here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense, made changes accordingly.

isNamespaceEnabled = isNamespaceEnabledAccount;
Copy link
Contributor

Choose a reason for hiding this comment

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

why did we remove the **this.**isNamespaceEnabled part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted this.

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,8 @@ public void initialize(URI uri, Configuration configuration)
* HNS Account Cannot have Blob Endpoint URI.
*/
try {
// This will update namespaceEnable based on getAcl in case config is not set.
// This Information will be stored in abfsConfiguration class.
abfsConfiguration.validateConfiguredServiceType(
tryGetIsNamespaceEnabled(initFSTracingContext));
} catch (InvalidConfigurationValueException ex) {
Expand Down Expand Up @@ -296,7 +298,6 @@ public void initialize(URI uri, Configuration configuration)
}
}
}
getAbfsStore().updateClientWithNamespaceInfo(new TracingContext(initFSTracingContext));

LOG.trace("Initiate check for delegation token manager");
if (UserGroupInformation.isSecurityEnabled()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,6 @@ public class AzureBlobFileSystemStore implements Closeable, ListingSupport {

private final AbfsConfiguration abfsConfiguration;
private Set<String> azureInfiniteLeaseDirSet;
private volatile Trilean isNamespaceEnabled;
private final AuthType authType;
private final UserGroupInformation userGroupInformation;
private final IdentityTransformerInterface identityTransformer;
Expand Down Expand Up @@ -234,8 +233,6 @@ public AzureBlobFileSystemStore(

LOG.trace("AbfsConfiguration init complete");

this.isNamespaceEnabled = abfsConfiguration.getIsNamespaceEnabledAccount();

this.userGroupInformation = UserGroupInformation.getCurrentUser();
this.userName = userGroupInformation.getShortUserName();
LOG.trace("UGI init complete");
Expand Down Expand Up @@ -287,18 +284,6 @@ public AzureBlobFileSystemStore(
"abfs-bounded");
}

/**
* Updates the client with the namespace information.
*
* @param tracingContext the tracing context to be used for the operation
* @throws AzureBlobFileSystemException if an error occurs while updating the client
*/
public void updateClientWithNamespaceInfo(TracingContext tracingContext)
throws AzureBlobFileSystemException {
boolean isNamespaceEnabled = getIsNamespaceEnabled(tracingContext);
AbfsClient.setIsNamespaceEnabled(isNamespaceEnabled);
}

/**
* Checks if the given key in Azure Storage should be stored as a page
* blob instead of block blob.
Expand Down Expand Up @@ -409,32 +394,32 @@ public boolean getIsNamespaceEnabled(TracingContext tracingContext)

private synchronized boolean getNamespaceEnabledInformationFromServer(
final TracingContext tracingContext) throws AzureBlobFileSystemException {
if (isNamespaceEnabled != Trilean.UNKNOWN) {
return isNamespaceEnabled.toBoolean();
if (abfsConfiguration.getIsNamespaceEnabledAccount() != Trilean.UNKNOWN) {
return isNamespaceEnabled();
}
try {
LOG.debug("Get root ACL status");
getClient(AbfsServiceType.DFS).getAclStatus(AbfsHttpConstants.ROOT_PATH, tracingContext);
// If getAcl succeeds, namespace is enabled.
isNamespaceEnabled = Trilean.getTrilean(true);
setNamespaceEnabled(Trilean.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 account type.
if (HttpURLConnection.HTTP_BAD_REQUEST != ex.getStatusCode()) {
// If getAcl fails with anything other than 400, namespace is enabled.
isNamespaceEnabled = Trilean.getTrilean(true);
setNamespaceEnabled(Trilean.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);
setNamespaceEnabled(Trilean.FALSE);
} catch (AzureBlobFileSystemException ex) {
throw ex;
}
return isNamespaceEnabled.toBoolean();
return isNamespaceEnabled();
}

/**
Expand All @@ -443,7 +428,7 @@ private synchronized boolean getNamespaceEnabledInformationFromServer(
*/
@VisibleForTesting
boolean isNamespaceEnabled() throws TrileanConversionException {
return this.isNamespaceEnabled.toBoolean();
return abfsConfiguration.getIsNamespaceEnabledAccount().toBoolean();
Copy link
Contributor

Choose a reason for hiding this comment

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

here also should we have a try-catch block for TrileanConversionException like we have in AbfsClient-line 1722?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method should throw an exception if namespace enabled isn't initialized, so a try-catch block isn't necessary here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Better to use getter here and other places for abfsconfiguration. This will make sure same object is being referred everywhere and can easily be mocked if needed in future.

}

@VisibleForTesting
Expand Down Expand Up @@ -2028,7 +2013,7 @@ DataBlocks.BlockFactory getBlockFactory() {

@VisibleForTesting
void setNamespaceEnabled(Trilean isNamespaceEnabled){
this.isNamespaceEnabled = isNamespaceEnabled;
abfsConfiguration.setIsNamespaceEnabledAccount(isNamespaceEnabled);
}

@VisibleForTesting
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
import org.apache.hadoop.fs.azurebfs.contracts.exceptions.InvalidFileSystemPropertyException;
import org.apache.hadoop.fs.azurebfs.contracts.exceptions.InvalidUriException;
import org.apache.hadoop.fs.azurebfs.contracts.exceptions.SASTokenProviderException;
import org.apache.hadoop.fs.azurebfs.contracts.exceptions.TrileanConversionException;
import org.apache.hadoop.fs.azurebfs.contracts.services.AppendRequestParameters;
import org.apache.hadoop.fs.azurebfs.contracts.services.AzureServiceErrorCode;
import org.apache.hadoop.fs.azurebfs.contracts.services.ListResultEntrySchema;
Expand Down Expand Up @@ -194,7 +195,6 @@
private KeepAliveCache keepAliveCache;

private AbfsApacheHttpClient abfsApacheHttpClient;
private static boolean isNamespaceEnabled = false;

/**
* logging the rename failure if metadata is in an incomplete state.
Expand Down Expand Up @@ -1717,17 +1717,13 @@
*
* @return True if the namespace is enabled, false otherwise.
*/
public static boolean getIsNamespaceEnabled() {
return isNamespaceEnabled;
}

/**
* Sets the namespace enabled status.
*
* @param namespaceEnabled True to enable the namespace, false to disable it.
*/
public static void setIsNamespaceEnabled(final boolean namespaceEnabled) {
isNamespaceEnabled = namespaceEnabled;
public boolean getIsNamespaceEnabled() throws TrileanConversionException {

Check failure on line 1720 in hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java

View check run for this annotation

ASF Cloudbees Jenkins ci-hadoop / Apache Yetus

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java#L1720

javadoc: warning: no @throws for org.apache.hadoop.fs.azurebfs.contracts.exceptions.TrileanConversionException

Check failure on line 1720 in hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java

View check run for this annotation

ASF Cloudbees Jenkins ci-hadoop / Apache Yetus

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java#L1720

javadoc: warning: no @throws for org.apache.hadoop.fs.azurebfs.contracts.exceptions.TrileanConversionException
try {
return abfsConfiguration.getIsNamespaceEnabledAccount().toBoolean();
} catch (TrileanConversionException ex) {
LOG.error("Failed to convert namespace enabled account property to boolean", ex);
throw ex;
}
}

protected boolean isRenameResilience() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1565,7 +1565,8 @@
* @return client transaction id
*/
@VisibleForTesting
public String addClientTransactionIdToHeader(List<AbfsHttpHeader> requestHeaders) {
public String addClientTransactionIdToHeader(List<AbfsHttpHeader> requestHeaders)

Check failure on line 1568 in hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsDfsClient.java

View check run for this annotation

ASF Cloudbees Jenkins ci-hadoop / Apache Yetus

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsDfsClient.java#L1568

javadoc: warning: no @throws for org.apache.hadoop.fs.azurebfs.contracts.exceptions.AzureBlobFileSystemException

Check failure on line 1568 in hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsDfsClient.java

View check run for this annotation

ASF Cloudbees Jenkins ci-hadoop / Apache Yetus

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsDfsClient.java#L1568

javadoc: warning: no @throws for org.apache.hadoop.fs.azurebfs.contracts.exceptions.AzureBlobFileSystemException
throws AzureBlobFileSystemException{
String clientTransactionId = null;
// Set client transaction ID if the namespace and client transaction ID config are enabled.
if (getIsNamespaceEnabled() && getAbfsConfiguration().getIsClientTransactionIdEnabled()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.apache.hadoop.fs.azurebfs.contracts.annotations.ConfigurationValidationAnnotations.LongConfigurationValidatorAnnotation;
import org.apache.hadoop.fs.azurebfs.contracts.annotations.ConfigurationValidationAnnotations.Base64StringConfigurationValidatorAnnotation;
import org.apache.hadoop.fs.azurebfs.contracts.exceptions.KeyProviderException;
import org.apache.hadoop.fs.azurebfs.enums.Trilean;
import org.apache.hadoop.fs.azurebfs.utils.Base64;

import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_SSL_CHANNEL_MODE_KEY;
Expand Down Expand Up @@ -210,6 +211,66 @@ public void testSSLSocketFactoryConfiguration()
.isEqualTo(DelegatingSSLSocketFactory.SSLChannelMode.OpenSSL);
}

/**
* Tests the behavior of AbfsConfiguration when the namespace-enabled
* configuration is not explicitly set (i.e., set to an empty string).
*
* Expects the namespace value to be set as UNKNOWN.
*
* @throws Exception if any error occurs during configuration setup or evaluation
*/
@Test
public void testNameSpaceConfigNotSet() throws Exception {
Configuration configuration = new Configuration();
configuration.set(ConfigurationKeys.FS_AZURE_ACCOUNT_IS_HNS_ENABLED, "");
AbfsConfiguration abfsConfig = new AbfsConfiguration(configuration, "bogusAccountName");

// Test that the namespace enabled config is set correctly
Assertions.assertThat(abfsConfig.getIsNamespaceEnabledAccount())
.describedAs("Namespace enabled should be unknown in case config is not set")
.isEqualTo(Trilean.UNKNOWN);
}

/**
* Tests the behavior of AbfsConfiguration when the namespace-enabled
* configuration is explicitly set to "true".
*
* Expects the namespace value to be set as TRUE.
*
* @throws Exception if any error occurs during configuration setup or evaluation
*/
@Test
public void testNameSpaceConfigSetToTrue() throws Exception {
Configuration configuration = new Configuration();
configuration.set(ConfigurationKeys.FS_AZURE_ACCOUNT_IS_HNS_ENABLED, "true");
AbfsConfiguration abfsConfig = new AbfsConfiguration(configuration, "bogusAccountName");

// Test that the namespace enabled config is set correctly
Assertions.assertThat(abfsConfig.getIsNamespaceEnabledAccount())
.describedAs("Namespace enabled should be true in case config is set to true")
.isEqualTo(Trilean.TRUE);
}

/**
* Tests the behavior of AbfsConfiguration when the namespace-enabled
* configuration is explicitly set to "false".
*
* Expects the namespace value to be set as FALSE.
*
* @throws Exception if any error occurs during configuration setup or evaluation
*/
@Test
public void testNameSpaceConfigSetToFalse() throws Exception {
Configuration configuration = new Configuration();
configuration.set(ConfigurationKeys.FS_AZURE_ACCOUNT_IS_HNS_ENABLED, "false");
AbfsConfiguration abfsConfig = new AbfsConfiguration(configuration, "bogusAccountName");

// Test that the namespace enabled config is set correctly
Assertions.assertThat(abfsConfig.getIsNamespaceEnabledAccount())
.describedAs("Namespace enabled should be false in case config is set to false")
.isEqualTo(Trilean.FALSE);
}

public static AbfsConfiguration updateRetryConfigs(AbfsConfiguration abfsConfig,
int retryCount,
int backoffTime) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;

import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AzureBlobFileSystemException;
import org.apache.hadoop.fs.azurebfs.utils.TracingContext;
import org.apache.hadoop.util.functional.FunctionRaisingIOE;

Expand Down Expand Up @@ -370,7 +371,7 @@ public static void mockGetRenameBlobHandler(AbfsBlobClient blobClient,
* @param clientTransactionId An array to hold the generated transaction ID.
*/
public static void mockAddClientTransactionIdToHeader(AbfsDfsClient abfsDfsClient,
String[] clientTransactionId) {
String[] clientTransactionId) throws AzureBlobFileSystemException {
Mockito.doAnswer(addClientTransactionId -> {
clientTransactionId[0] = UUID.randomUUID().toString();
List<AbfsHttpHeader> headers = addClientTransactionId.getArgument(0);
Expand Down