Iterating on Gabor's feedback

Author: Sean Mackrory

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java

@@ -238,7 +238,7 @@ public class S3AFileSystem extends FileSystem implements StreamCapabilities,
   private volatile boolean isClosed = false;
   private MetadataStore metadataStore;
   private boolean allowAuthoritativeMetadataStore;
-  private Collection<String> allowAuthoritativePaths = new ArrayList<>();
+  private Collection<String> allowAuthoritativePaths;
 
   /** Delegation token integration; non-empty when DT support is enabled. */
   private Optional<S3ADelegationTokens> delegationTokens = Optional.empty();
@@ -400,15 +400,7 @@ public void initialize(URI name, Configuration originalConf)
       setMetadataStore(S3Guard.getMetadataStore(this));
       allowAuthoritativeMetadataStore = conf.getBoolean(METADATASTORE_AUTHORITATIVE,
           DEFAULT_METADATASTORE_AUTHORITATIVE);
-
-      String[] authoritativePaths =
-            conf.getTrimmedStrings(AUTHORITATIVE_PATH, DEFAULT_AUTHORITATIVE_PATH);
-      if (authoritativePaths.length > 0) {
-        for (int i = 0; i < authoritativePaths.length; i++) {
-          Path qualified = qualify(new Path(authoritativePaths[i]));
-          allowAuthoritativePaths.add(maybeAddTrailingSlash(qualified.toString()));
-        }
-      }
+      allowAuthoritativePaths = S3Guard.getAuthoritativePaths(this);
 
       if (hasMetadataStore()) {
         LOG.debug("Using metadata store {}, authoritative store={}, authoritative path={}",
@@ -421,23 +413,6 @@ public void initialize(URI name, Configuration originalConf)
 
   }
 
-  @VisibleForTesting
-  boolean allowAuthoritative(Path p) {
-    String haystack = maybeAddTrailingSlash(p.toString());
-    if (allowAuthoritativeMetadataStore) {
-      return true;
-    }
-    if (!allowAuthoritativePaths.isEmpty()) {
-      for (String needle : allowAuthoritativePaths) {
-
-        if (haystack.startsWith(needle)) {
-          return true;
-        }
-      }
-    }
-    return false;
-  }
-
   /**
    * Initialize the thread pool.
    * This must be re-invoked after replacing the S3Client during test
@@ -868,7 +843,8 @@ public String pathToKey(Path path) {
    * @param key s3 key or ""
    * @return the with a trailing "/", or, if it is the root key, "",
    */
-  private String maybeAddTrailingSlash(String key) {
+  @InterfaceAudience.Private
+  public String maybeAddTrailingSlash(String key) {
     if (!key.isEmpty() && !key.endsWith("/")) {
       return key + '/';
     } else {
@@ -2426,7 +2402,9 @@ public FileStatus[] innerListStatus(Path f) throws FileNotFoundException,
 
       DirListingMetadata dirMeta =
           S3Guard.listChildrenWithTtl(metadataStore, path, ttlTimeProvider);
-      if (allowAuthoritative(f) && dirMeta != null && dirMeta.isAuthoritative()) {
+      boolean allowAuthoritative = S3Guard.allowAuthoritative(f, this,
+          allowAuthoritativeMetadataStore, allowAuthoritativePaths);
+      if (allowAuthoritative && dirMeta != null && dirMeta.isAuthoritative()) {
         return S3Guard.dirMetaToStatuses(dirMeta);
       }
 
@@ -2443,8 +2421,9 @@ public FileStatus[] innerListStatus(Path f) throws FileNotFoundException,
         result.add(files.next());
       }
       // merge the results. This will update the store as needed
+
       return S3Guard.dirListingUnion(metadataStore, path, result, dirMeta,
-          allowAuthoritative(f), ttlTimeProvider);
+          allowAuthoritative, ttlTimeProvider);
     } else {
       LOG.debug("Adding: rd (not a dir): {}", path);
       FileStatus[] stats = new FileStatus[1];
@@ -2657,8 +2636,10 @@ S3AFileStatus innerGetFileStatus(final Path f,
       // dest is also a directory, there's no difference.
       // TODO After HADOOP-16085 the modification detection can be done with
       //  etags or object version instead of modTime
+      boolean allowAuthoritative = S3Guard.allowAuthoritative(f, this,
+          allowAuthoritativeMetadataStore, allowAuthoritativePaths);
       if (!pm.getFileStatus().isDirectory() &&
-          !allowAuthoritative(f)) {
+          !allowAuthoritative) {
         LOG.debug("Metadata for {} found in the non-auth metastore.", path);
         final long msModTime = pm.getFileStatus().getModificationTime();
 
@@ -3823,13 +3804,16 @@ private RemoteIterator<S3ALocatedFileStatus> innerListFiles(Path f, boolean
             key, delimiter);
         final RemoteIterator<S3AFileStatus> cachedFilesIterator;
         final Set<Path> tombstones;
+        boolean allowAuthoritative = S3Guard.allowAuthoritative(f, this,
+            allowAuthoritativeMetadataStore, allowAuthoritativePaths);
         if (recursive) {
           final PathMetadata pm = metadataStore.get(path, true);
           // shouldn't need to check pm.isDeleted() because that will have
           // been caught by getFileStatus above.
+
           MetadataStoreListFilesIterator metadataStoreListFilesIterator =
               new MetadataStoreListFilesIterator(metadataStore, pm,
-                  allowAuthoritative(f));
+                  allowAuthoritative);
           tombstones = metadataStoreListFilesIterator.listTombstones();
           cachedFilesIterator = metadataStoreListFilesIterator;
         } else {
@@ -3842,7 +3826,7 @@ private RemoteIterator<S3ALocatedFileStatus> innerListFiles(Path f, boolean
           }
           cachedFilesIterator = listing.createProvidedFileStatusIterator(
               S3Guard.dirMetaToStatuses(meta), ACCEPT_ALL, acceptor);
-          if (allowAuthoritative(f) && meta != null && meta.isAuthoritative()) {
+          if (allowAuthoritative && meta != null && meta.isAuthoritative()) {
             // metadata listing is authoritative, so return it directly
             return listing.createLocatedFileStatusIterator(cachedFilesIterator);
           }
@@ -3915,7 +3899,9 @@ public RemoteIterator<LocatedFileStatus> listLocatedStatus(final Path f,
               final RemoteIterator<S3AFileStatus> cachedFileStatusIterator =
                   listing.createProvidedFileStatusIterator(
                       S3Guard.dirMetaToStatuses(meta), filter, acceptor);
-              return (allowAuthoritative(f) && meta != null
+              boolean allowAuthoritative = S3Guard.allowAuthoritative(f, this,
+                  allowAuthoritativeMetadataStore, allowAuthoritativePaths);
+              return (allowAuthoritative && meta != null
                   && meta.isAuthoritative())
                   ? listing.createLocatedFileStatusIterator(
                   cachedFileStatusIterator)

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3Guard.java

@@ -34,6 +34,7 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
+import org.apache.hadoop.fs.s3a.S3AFileSystem;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -49,9 +50,8 @@
 import org.apache.hadoop.fs.s3a.S3AInstrumentation;
 import org.apache.hadoop.util.ReflectionUtils;
 
-import static org.apache.hadoop.fs.s3a.Constants.DEFAULT_METADATASTORE_METADATA_TTL;
-import static org.apache.hadoop.fs.s3a.Constants.METADATASTORE_METADATA_TTL;
-import static org.apache.hadoop.fs.s3a.Constants.S3_METADATA_STORE_IMPL;
+import static org.apache.hadoop.fs.s3a.Constants.*;
+import static org.apache.hadoop.fs.s3a.Constants.DEFAULT_AUTHORITATIVE_PATH;
 import static org.apache.hadoop.fs.s3a.Statistic.S3GUARD_METADATASTORE_PUT_PATH_LATENCY;
 import static org.apache.hadoop.fs.s3a.Statistic.S3GUARD_METADATASTORE_PUT_PATH_REQUEST;
 import static org.apache.hadoop.fs.s3a.S3AUtils.createUploadFileStatus;
@@ -772,4 +772,33 @@ public static DirListingMetadata listChildrenWithTtl(MetadataStore ms,
     return dlm;
   }
 
+  public static Collection<String> getAuthoritativePaths(S3AFileSystem fs) {
+    String[] rawAuthoritativePaths =
+        fs.getConf().getTrimmedStrings(AUTHORITATIVE_PATH, DEFAULT_AUTHORITATIVE_PATH);
+    Collection<String> authoritativePaths = new ArrayList<>();
+    if (rawAuthoritativePaths.length > 0) {
+      for (int i = 0; i < rawAuthoritativePaths.length; i++) {
+        Path qualified = fs.qualify(new Path(rawAuthoritativePaths[i]));
+        authoritativePaths.add(fs.maybeAddTrailingSlash(qualified.toString()));
+      }
+    }
+    return authoritativePaths;
+  }
+
+  public static boolean allowAuthoritative(Path p, S3AFileSystem fs,
+      boolean authMetadataStore, Collection<String> authPaths) {
+    String haystack = fs.maybeAddTrailingSlash(p.toString());
+    if (authMetadataStore) {
+      return true;
+    }
+    if (!authPaths.isEmpty()) {
+      for (String needle : authPaths) {
+
+        if (haystack.startsWith(needle)) {
+          return true;
+        }
+      }
+    }
+    return false;
+  }
 }

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestAuthoritativePath.java

@@ -20,8 +20,10 @@
 
 import java.io.IOException;
 import java.net.URI;
+import java.util.Collection;
 
 import org.apache.hadoop.fs.s3a.s3guard.NullMetadataStore;
+import org.apache.hadoop.fs.s3a.s3guard.S3Guard;
 import org.apache.hadoop.io.IOUtils;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.Path;
@@ -45,7 +47,7 @@ public class ITestAuthoritativePath extends AbstractS3ATestBase {
   public Path testRoot;
 
   private S3AFileSystem fullyAuthFS;
-  private S3AFileSystem unguardedFS;
+  private S3AFileSystem rawFS;
 
   private MetadataStore ms;
 
@@ -75,9 +77,9 @@ public void setup() throws Exception {
     assertTrue("Authoritative mode off in fullyAuthFS",
         fullyAuthFS.hasAuthoritativeMetadataStore());
 
-    unguardedFS = createUnguardedFS();
+    rawFS = createRawFS();
     assertFalse("UnguardedFS still has S3Guard",
-        unguardedFS.hasMetadataStore());
+        rawFS.hasMetadataStore());
   }
 
   private void cleanUpFS(S3AFileSystem fs) {
@@ -92,7 +94,7 @@ public void teardown() throws Exception {
     fullyAuthFS.delete(testRoot, true);
 
     cleanUpFS(fullyAuthFS);
-    cleanUpFS(unguardedFS);
+    cleanUpFS(rawFS);
     super.teardown();
   }
 
@@ -141,20 +143,7 @@ private S3AFileSystem createMultiPathAuthFS(String first, String middle, String
     return newFS;
   }
 
-  private S3AFileSystem createNonAuthFS() throws Exception {
-    S3AFileSystem testFS = getFileSystem();
-    Configuration config = new Configuration(testFS.getConf());
-    URI uri = testFS.getUri();
-
-    removeBaseAndBucketOverrides(uri.getHost(), config,
-        METADATASTORE_AUTHORITATIVE, AUTHORITATIVE_PATH);
-    final S3AFileSystem newFS = createFS(uri, config);
-    // set back the same metadata store instance
-    newFS.setMetadataStore(ms);
-    return newFS;
-  }
-
-  private S3AFileSystem createUnguardedFS() throws Exception {
+  private S3AFileSystem createRawFS() throws Exception {
     S3AFileSystem testFS = getFileSystem();
     Configuration config = new Configuration(testFS.getConf());
     URI uri = testFS.getUri();
@@ -190,7 +179,7 @@ private void runTestOutsidePath(S3AFileSystem partiallyAuthFS, Path nonAuthPath)
     // trigger an authoritative write-back
     fullyAuthFS.listStatus(inBandPath.getParent());
 
-    touch(unguardedFS, outOfBandPath);
+    touch(rawFS, outOfBandPath);
 
     // listing lacks outOfBandPath => short-circuited by auth mode
     checkListingDoesNotContainPath(fullyAuthFS, outOfBandPath);
@@ -213,7 +202,7 @@ private void runTestInsidePath(S3AFileSystem partiallyAuthFS, Path authPath) thr
     // trigger an authoritative write-back
     fullyAuthFS.listStatus(inBandPath.getParent());
 
-    touch(unguardedFS, outOfBandPath);
+    touch(rawFS, outOfBandPath);
 
     // listing lacks outOfBandPath => short-circuited by auth mode
     checkListingDoesNotContainPath(fullyAuthFS, outOfBandPath);
@@ -291,16 +280,20 @@ public void testMultiAuthPath() throws Exception {
   @Test
   public void testPrefixVsDirectory() throws Exception {
     S3AFileSystem fs = createSinglePathAuthFS("/auth");
+    Collection<String> authPaths = S3Guard.getAuthoritativePaths(fs);
 
     try{
       Path totalMismatch = new Path(testRoot, "/non-auth");
-      assertFalse(fs.allowAuthoritative(totalMismatch));
+      assertFalse(S3Guard.allowAuthoritative(totalMismatch, fs,
+          false, authPaths));
 
       Path prefixMatch = new Path(testRoot, "/authoritative");
-      assertFalse(fs.allowAuthoritative(prefixMatch));
+      assertFalse(S3Guard.allowAuthoritative(prefixMatch, fs,
+          false, authPaths));
 
       Path directoryMatch = new Path(testRoot, "/auth/oritative");
-      assertTrue(fs.allowAuthoritative(directoryMatch));
+      assertTrue(S3Guard.allowAuthoritative(directoryMatch, fs,
+          false, authPaths));
     } finally {
       cleanUpFS(fs);
     }