steveloughran commented on code in PR #6752:
URL: https://github.com/apache/hadoop/pull/6752#discussion_r1621197499
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java:
##########
@@ -367,16 +366,35 @@ private String[] authorityParts(URI uri) throws
InvalidUriAuthorityException, In
public boolean getIsNamespaceEnabled(TracingContext tracingContext)
throws AzureBlobFileSystemException {
try {
- return this.isNamespaceEnabled.toBoolean();
+ return getNamespaceConfig();
} catch (TrileanConversionException e) {
LOG.debug("isNamespaceEnabled is UNKNOWN; fall back and determine
through"
+ " getAcl server call", e);
}
- isNamespaceEnabled =
Trilean.getTrilean(NamespaceUtil.isNamespaceEnabled(client, tracingContext));
+ try {
+ LOG.debug("Get root ACL status");
+ getClient().getAclStatus(AbfsHttpConstants.ROOT_PATH, tracingContext);
+ 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()) {
+ throw ex;
+ }
+ isNamespaceEnabled = Trilean.getTrilean(false);
+ } catch (AzureBlobFileSystemException ex) {
+ throw ex;
+ }
return isNamespaceEnabled.toBoolean();
}
+ @VisibleForTesting
+ boolean getNamespaceConfig() throws TrileanConversionException {
Review Comment:
1. The name here should just be `isNamespaceEnabled`; getNamespaceConfig
implies there's more information coming back
2. the javadocs *which are needed* should explain why the exception is raised
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/AbfsHttpConstants.java:
##########
@@ -165,5 +169,20 @@ public static ApiVersion getCurrentVersion() {
*/
public static final Integer HTTP_STATUS_CATEGORY_QUOTIENT = 100;
+ private static final String CPK_CONFIG_LIST =
Review Comment:
again, javadoc with + {@value}. for the IDEs, more than the docs themselves
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java:
##########
@@ -367,16 +366,35 @@ private String[] authorityParts(URI uri) throws
InvalidUriAuthorityException, In
public boolean getIsNamespaceEnabled(TracingContext tracingContext)
throws AzureBlobFileSystemException {
try {
- return this.isNamespaceEnabled.toBoolean();
+ return getNamespaceConfig();
} catch (TrileanConversionException e) {
LOG.debug("isNamespaceEnabled is UNKNOWN; fall back and determine
through"
+ " getAcl server call", e);
}
- isNamespaceEnabled =
Trilean.getTrilean(NamespaceUtil.isNamespaceEnabled(client, tracingContext));
+ try {
+ LOG.debug("Get root ACL status");
Review Comment:
I think this should be synchronized, with the first things in the sync block
checking for the isNamespaceEnabled flag to see if it is now valid.
Lots of fs operations do this check, and so you could have multiple threads
all triggering duplicate HTTP requests.
also, mark the field as volatile.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java:
##########
@@ -367,16 +366,35 @@ private String[] authorityParts(URI uri) throws
InvalidUriAuthorityException, In
public boolean getIsNamespaceEnabled(TracingContext tracingContext)
Review Comment:
add javadoc, make clear this will issue HTTP requests to resolve the status,
if it has not already taken place -and that threads will block if another
thread is working on this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
