Resolved Comments

Author: Anuj Modi

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

@@ -942,24 +942,31 @@ public AccessTokenProvider getTokenProvider() throws TokenAccessProviderExceptio
   }
 
   /**
-   * The following method chooses between a configured fixed sas token, and a user implementation of the SASTokenProvider interface,
-   * depending on which one is available. In case a user SASTokenProvider implementation is not present, and a fixed token is configured,
-   * it simply returns null, to set the sasTokenProvider object for current configuration instance to null.
-   * The fixed token is read and used later. This is done to:
-   * 1. check for cases where both are not set, while initializing AbfsConfiguration,
-   * to not proceed further than thi stage itself when none of the options are available.
-   * 2. avoid using  similar tokenProvider implementation to just read the configured fixed token,
-   * as this could create confusion. The configuration is introduced
-   * primarily to avoid using any tokenProvider class/interface. Also,implementing the SASTokenProvider requires relying on the raw configurations.
-   * It is more stable to depend on the AbfsConfiguration with which a filesystem is initialized,
+   * The user can choose between a configured fixed sas token, and a user
+   * implementation of the SASTokenProvider interface. Preference will be given
+   * to SASTokenProvider class provided as the value of "fs.azure.sas.token.provider.type".
+   * If above config is not set, it is expected that user wants to use a
+   * fixed SAS Token provided as value of "fs.azure.sas.fixed.token".
+   * <ol>
+   *   <li>If both the configs are not provided,
+   *   initialization fails and {@link TokenAccessProviderException} is thrown.</li>
+   *   <li>If both are present, SASTokenProvider class will be used to generate SAS Token.</li>
+   *   <li>If only fixed SAS Token is configured, this will return null
+   *   and Fixed SAS token will be used to sign requests.</li>
+   * </ol>
+   * Avoid using a tokenProvider implementation just to read the configured fixed token,
+   * as this could create confusion. Also,implementing the SASTokenProvider
+   * requires relying on the raw configurations. It is more stable to depend on the
+   * AbfsConfiguration with which a filesystem is initialized,
    * and eliminate chances of dynamic modifications and spurious situations.
    * @return sasTokenProvider object
    * @throws AzureBlobFileSystemException
    */
   public SASTokenProvider getSASTokenProvider() throws AzureBlobFileSystemException {
     AuthType authType = getEnum(FS_AZURE_ACCOUNT_AUTH_TYPE_PROPERTY_NAME, AuthType.SharedKey);
     if (authType != AuthType.SAS) {
-      throw new SASTokenProviderException(String.format("Invalid auth type: %s is being used, expecting SAS", authType));
+      throw new SASTokenProviderException(String.format(
+          "Invalid auth type: %s is being used, expecting SAS.", authType));
     }
 
     try {
@@ -970,30 +977,30 @@ public SASTokenProvider getSASTokenProvider() throws AzureBlobFileSystemExceptio
       String configuredFixedToken = this.rawConfig.get(FS_AZURE_SAS_FIXED_TOKEN,
           null);
 
-      Preconditions.checkArgument(!(sasTokenProviderImplementation == null
-              && configuredFixedToken == null),
-          String.format(
-              "The value for both \"%s\" and \"%s\" cannot be invalid.",
-              FS_AZURE_SAS_TOKEN_PROVIDER_TYPE, FS_AZURE_SAS_FIXED_TOKEN));
+      Preconditions.checkArgument(
+          sasTokenProviderImplementation != null || configuredFixedToken != null,
+          "At least one of the \"%s\" and \"%s\" must be set.",
+              FS_AZURE_SAS_TOKEN_PROVIDER_TYPE, FS_AZURE_SAS_FIXED_TOKEN);
 
+      // Prefer SASTokenProvider Implementation if configured.
       if (sasTokenProviderImplementation != null) {
-        LOG.trace(
-            "Using SASTokenProvider class because it is given precedence when it is set");
+        LOG.trace("Using SASTokenProvider class because it is given precedence when it is set.");
         SASTokenProvider sasTokenProvider = ReflectionUtils.newInstance(
             sasTokenProviderImplementation, rawConfig);
         Preconditions.checkArgument(sasTokenProvider != null,
-            String.format("Failed to initialize %s",
-                sasTokenProviderImplementation));
+            "Failed to initialize %s", sasTokenProviderImplementation);
 
         LOG.trace("Initializing {}", sasTokenProviderImplementation.getName());
         sasTokenProvider.initialize(rawConfig, accountName);
         LOG.trace("{} init complete", sasTokenProviderImplementation.getName());
         return sasTokenProvider;
       } else {
+        // Configured Fixed SAS Token will be used to sign the requests.
         return null;
       }
     } catch (Exception e) {
-      throw new TokenAccessProviderException("Unable to load SAS token provider class: " + e, e);
+      throw new TokenAccessProviderException(
+          "Unable to load SAS token provider class: " + e, e);
     }
   }
 
@@ -1006,14 +1013,14 @@ public EncryptionContextProvider createEncryptionContextProvider() {
       Class<? extends EncryptionContextProvider> encryptionContextClass =
           getAccountSpecificClass(configKey, null,
               EncryptionContextProvider.class);
-      Preconditions.checkArgument(encryptionContextClass != null, String.format(
+      Preconditions.checkArgument(encryptionContextClass != null,
           "The configuration value for %s is invalid, or config key is not account-specific",
-          configKey));
+          configKey);
 
       EncryptionContextProvider encryptionContextProvider =
           ReflectionUtils.newInstance(encryptionContextClass, rawConfig);
       Preconditions.checkArgument(encryptionContextProvider != null,
-          String.format("Failed to initialize %s", encryptionContextClass));
+         "Failed to initialize %s", encryptionContextClass);
 
       LOG.trace("{} init complete", encryptionContextClass.getName());
       return encryptionContextProvider;

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

@@ -262,17 +262,13 @@ public final class ConfigurationKeys {
   /** Add extra resilience to rename failures, at the expense of performance. */
   public static final String FS_AZURE_ABFS_RENAME_RESILIENCE = "fs.azure.enable.rename.resilience";
 
-  public static String accountProperty(String property, String account) {
-    return property + "." + account;
-  }
-
   public static final String FS_AZURE_ENABLE_DELEGATION_TOKEN = "fs.azure.enable.delegation.token";
   public static final String FS_AZURE_DELEGATION_TOKEN_PROVIDER_TYPE = "fs.azure.delegation.token.provider.type";
 
-  /** Key for fixed SAS token **/
+  /** Key for fixed SAS token: {@value}. **/
   public static final String FS_AZURE_SAS_FIXED_TOKEN = "fs.azure.sas.fixed.token";
 
-  /** Key for SAS token provider **/
+  /** Key for SAS token provider: {@value}. **/
   public static final String FS_AZURE_SAS_TOKEN_PROVIDER_TYPE = "fs.azure.sas.token.provider.type";
 
   /** For performance, AbfsInputStream/AbfsOutputStream re-use SAS tokens until the expiry is within this number of seconds. **/

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

@@ -310,7 +310,7 @@ public AbfsRestOperation createFilesystem(TracingContext tracingContext)
 
     final AbfsUriQueryBuilder abfsUriQueryBuilder = new AbfsUriQueryBuilder();
     abfsUriQueryBuilder.addQuery(QUERY_PARAM_RESOURCE, FILESYSTEM);
-    // appending SAS Token to query
+
     appendSASTokenToQuery(ROOT_PATH, "", abfsUriQueryBuilder);
 
     final URL url = createRequestUrl(abfsUriQueryBuilder.toString());
@@ -334,7 +334,7 @@ public AbfsRestOperation setFilesystemProperties(final String properties,
 
     final AbfsUriQueryBuilder abfsUriQueryBuilder = createDefaultUriQueryBuilder();
     abfsUriQueryBuilder.addQuery(QUERY_PARAM_RESOURCE, FILESYSTEM);
-    // appending SAS Token to query
+
     appendSASTokenToQuery(ROOT_PATH, "", abfsUriQueryBuilder);
 
     final URL url = createRequestUrl(abfsUriQueryBuilder.toString());
@@ -376,7 +376,7 @@ public AbfsRestOperation getFilesystemProperties(TracingContext tracingContext)
 
     final AbfsUriQueryBuilder abfsUriQueryBuilder = createDefaultUriQueryBuilder();
     abfsUriQueryBuilder.addQuery(QUERY_PARAM_RESOURCE, FILESYSTEM);
-    // appending SAS Token to query
+
     appendSASTokenToQuery(ROOT_PATH, "", abfsUriQueryBuilder);
 
     final URL url = createRequestUrl(abfsUriQueryBuilder.toString());
@@ -394,7 +394,7 @@ public AbfsRestOperation deleteFilesystem(TracingContext tracingContext) throws
 
     final AbfsUriQueryBuilder abfsUriQueryBuilder = createDefaultUriQueryBuilder();
     abfsUriQueryBuilder.addQuery(QUERY_PARAM_RESOURCE, FILESYSTEM);
-    // appending SAS Token to query
+
     appendSASTokenToQuery(ROOT_PATH, "", abfsUriQueryBuilder);
 
     final URL url = createRequestUrl(abfsUriQueryBuilder.toString());
@@ -954,6 +954,7 @@ public AbfsRestOperation flush(final String path, final long position,
     abfsUriQueryBuilder.addQuery(QUERY_PARAM_POSITION, Long.toString(position));
     abfsUriQueryBuilder.addQuery(QUERY_PARAM_RETAIN_UNCOMMITTED_DATA, String.valueOf(retainUncommittedData));
     abfsUriQueryBuilder.addQuery(QUERY_PARAM_CLOSE, String.valueOf(isClose));
+
     // AbfsInputStream/AbfsOutputStream reuse SAS tokens for better performance
     String sasTokenForReuse = appendSASTokenToQuery(path, SASTokenProvider.WRITE_OPERATION,
         abfsUriQueryBuilder, cachedSasToken);
@@ -1044,6 +1045,7 @@ public AbfsRestOperation read(final String path,
     requestHeaders.add(new AbfsHttpHeader(IF_MATCH, eTag));
 
     final AbfsUriQueryBuilder abfsUriQueryBuilder = createDefaultUriQueryBuilder();
+
     // AbfsInputStream/AbfsOutputStream reuse SAS tokens for better performance
     String sasTokenForReuse = appendSASTokenToQuery(path, SASTokenProvider.READ_OPERATION,
         abfsUriQueryBuilder, cachedSasToken);
@@ -1294,8 +1296,16 @@ public static String getDirectoryQueryParameter(final String path) {
     return directory;
   }
 
+  /**
+   * Chooses between the SAS token provided by SASTokeProvider class and the configured fixed SAS token.
+   * Preference given to SASTokenProvider implementation to generate the SAS.
+   * If SASTokenProvider is null, returns the fixed SAS Token configured.
+   * @param operation
+   * @param path
+   * @return sasToken
+   * @throws IOException
+   */
   private String chooseSASToken(String operation, String path) throws IOException {
-    // chooses the SAS token provider class if it is configured, otherwise reads the configured fixed token
     if (sasTokenProvider == null) {
       return abfsConfiguration.get(ConfigurationKeys.FS_AZURE_SAS_FIXED_TOKEN);
     }
@@ -1341,16 +1351,17 @@ private String appendSASTokenToQuery(String path,
           sasToken = cachedSasToken;
           LOG.trace("Using cached SAS token.");
         }
+
         // if SAS Token contains a prefix of ?, it should be removed
         if (sasToken.charAt(0) == '?') {
           sasToken = sasToken.substring(1);
         }
+
         queryBuilder.setSASToken(sasToken);
         LOG.trace("SAS token fetch complete for {} on {}", operation, path);
       } catch (Exception ex) {
-        throw new SASTokenProviderException(String.format("Failed to acquire a SAS token for %s on %s due to %s",
-            operation,
-            path,
+        throw new SASTokenProviderException(String.format(
+            "Failed to acquire a SAS token for %s on %s due to %s", operation, path,
             ex.toString()));
       }
     }