anujmodi2021 commented on code in PR #7272:
URL: https://github.com/apache/hadoop/pull/7272#discussion_r1917752527
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java:
##########
@@ -234,6 +236,27 @@ public void initialize(URI uri, Configuration
configuration)
throw new
InvalidConfigurationValueException(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, ex);
}
+ /**
+ * Validates if the FixedSASTokenProvider is configured for non-HNS
accounts.
+ * If the namespace is not enabled and the FixedSASTokenProvider is not
configured,
+ * the filesystem is closed and an InvalidConfigurationValueException is
thrown.
+ *
+ * @throws InvalidConfigurationValueException if the namespace is not
enabled and the FixedSASTokenProvider is not configured.
+ */
+ try {
+ if (!getIsNamespaceEnabled(new TracingContext(initFSTracingContext))
+ && !abfsConfiguration.isFixedSASTokenProviderConfigured()) {
+ close();
Review Comment:
No need of close() here. FileSystem creation happening after all the checks
now.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java:
##########
@@ -234,6 +236,27 @@ public void initialize(URI uri, Configuration
configuration)
throw new
InvalidConfigurationValueException(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, ex);
}
+ /**
Review Comment:
This should be a blocked comment instead of javadoc to avoid dangling
javadoc issues.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java:
##########
@@ -234,6 +236,27 @@ public void initialize(URI uri, Configuration
configuration)
throw new
InvalidConfigurationValueException(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, ex);
}
+ /**
+ * Validates if the FixedSASTokenProvider is configured for non-HNS
accounts.
+ * If the namespace is not enabled and the FixedSASTokenProvider is not
configured,
+ * the filesystem is closed and an InvalidConfigurationValueException is
thrown.
+ *
+ * @throws InvalidConfigurationValueException if the namespace is not
enabled and the FixedSASTokenProvider is not configured.
+ */
+ try {
+ if (!getIsNamespaceEnabled(new TracingContext(initFSTracingContext))
Review Comment:
Better use `tryGetIsNamespaceEnabled()` here with proper exception handling.
Refer to:
https://github.com/apache/hadoop/blob/f38d7072566e88c77e47d1533e4be4c1bd98a06a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java#L242
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/contracts/exceptions/InvalidIngressServiceException.java:
##########
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.fs.azurebfs.contracts.exceptions;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+
+/**
+ * Exception thrown when an invalid ingress service is encountered.
+ *
+ * <p>This exception is used to indicate that the ingress service being used
+ * is not valid or supported. It extends the {@link AbfsRestOperationException}
+ * to provide additional context about the error.</p>
+ *
+ * @see AbfsRestOperationException
+ */
[email protected]
[email protected]
+public final class InvalidIngressServiceException extends
AbfsRestOperationException {
+
+ /**
+ * Constructs a new InvalidIngressServiceException with the specified
details.
+ *
+ * @param statusCode the HTTP status code
+ * @param errorCode the error code
+ * @param errorMessage the error message
+ * @param innerException the inner exception
+ */
+ public InvalidIngressServiceException(final int statusCode,
Review Comment:
May be I am missing something here, but I can see there are 2 constructors
for base exception and the one that excepts AbfsHttpOperation as a separate
parameter calls the `formatMessage()` which adds the req Id to exception
message.
Refering to:
https://github.com/apache/hadoop/blob/f38d7072566e88c77e47d1533e4be4c1bd98a06a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/contracts/exceptions/AbfsRestOperationException.java#L83
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java:
##########
@@ -234,6 +236,27 @@ public void initialize(URI uri, Configuration
configuration)
throw new
InvalidConfigurationValueException(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, ex);
}
+ /**
+ * Validates if the FixedSASTokenProvider is configured for non-HNS
accounts.
Review Comment:
I feel like the comment here should be re-phrased. Its sounds like only
FixedSasTokenProvider can be used with FNS. It should clarify that if Auth Type
is SAS then user can only configure a fixed SAS Token and SASTokenProvider
should not be configured at all otherwise Fixed SAS Token won't be used.
Refer to
https://github.com/apache/hadoop/blob/f38d7072566e88c77e47d1533e4be4c1bd98a06a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java#L1225
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java:
##########
@@ -234,6 +236,27 @@ public void initialize(URI uri, Configuration
configuration)
throw new
InvalidConfigurationValueException(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, ex);
}
+ /**
+ * Validates if the FixedSASTokenProvider is configured for non-HNS
accounts.
+ * If the namespace is not enabled and the FixedSASTokenProvider is not
configured,
+ * the filesystem is closed and an InvalidConfigurationValueException is
thrown.
+ *
+ * @throws InvalidConfigurationValueException if the namespace is not
enabled and the FixedSASTokenProvider is not configured.
+ */
+ try {
+ if (!getIsNamespaceEnabled(new TracingContext(initFSTracingContext))
+ && !abfsConfiguration.isFixedSASTokenProviderConfigured()) {
+ close();
+ throw new InvalidConfigurationValueException(
+ FS_AZURE_SAS_TOKEN_PROVIDER_TYPE, UNAUTHORIZED_SAS);
+ }
+ } catch (AzureBlobFileSystemException ex) {
Review Comment:
We need better exception handling here.
We should first catch SASTokenProviderException and throw
InvalidConfigurationValueException() with original exception included as it
will tell why getSASTokenProvder() failed.
Then we should catch the AzureBlobFileSystemException that can be thrown
while determining account type.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/AbfsHttpConstants.java:
##########
@@ -279,6 +303,13 @@ public static ApiVersion getCurrentVersion() {
+ "non-hierarchical-namespace account:"
+ CPK_CONFIG_LIST;
+
+ /**
+ * Exception message on filesystem init if token-provider-auth-type configs
are provided
+ */
+ public static final String UNAUTHORIZED_SAS =
Review Comment:
Can be moved to AbfsErrors.java class
--
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]
