steveloughran commented on a change in pull request #2038:
URL: https://github.com/apache/hadoop/pull/2038#discussion_r438125493
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
##########
@@ -4181,79 +4181,114 @@ public LocatedFileStatus next() throws IOException {
Path path = qualify(f);
LOG.debug("listFiles({}, {})", path, recursive);
try {
- // if a status was given, that is used, otherwise
- // call getFileStatus, which triggers an existence check
- final S3AFileStatus fileStatus = status != null
- ? status
- : (S3AFileStatus) getFileStatus(path);
- if (fileStatus.isFile()) {
+ // if a status was given and it is a file.
+ if (status != null && status.isFile()) {
// simple case: File
LOG.debug("Path is a file");
return new Listing.SingleStatusRemoteIterator(
- toLocatedFileStatus(fileStatus));
- } else {
- // directory: do a bulk operation
- String key = maybeAddTrailingSlash(pathToKey(path));
- String delimiter = recursive ? null : "/";
- LOG.debug("Requesting all entries under {} with delimiter '{}'",
- key, delimiter);
- final RemoteIterator<S3AFileStatus> cachedFilesIterator;
- final Set<Path> tombstones;
- boolean allowAuthoritative = allowAuthoritative(f);
- 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);
- tombstones = metadataStoreListFilesIterator.listTombstones();
- // if all of the below is true
- // - authoritative access is allowed for this metadatastore for
this directory,
- // - all the directory listings are authoritative on the client
- // - the caller does not force non-authoritative access
- // return the listing without any further s3 access
- if (!forceNonAuthoritativeMS &&
- allowAuthoritative &&
- metadataStoreListFilesIterator.isRecursivelyAuthoritative()) {
- S3AFileStatus[] statuses = S3Guard.iteratorToStatuses(
- metadataStoreListFilesIterator, tombstones);
- cachedFilesIterator = listing.createProvidedFileStatusIterator(
- statuses, ACCEPT_ALL, acceptor);
- return
listing.createLocatedFileStatusIterator(cachedFilesIterator);
- }
- cachedFilesIterator = metadataStoreListFilesIterator;
- } else {
- DirListingMetadata meta =
- S3Guard.listChildrenWithTtl(metadataStore, path, ttlTimeProvider,
- allowAuthoritative);
- if (meta != null) {
- tombstones = meta.listTombstones();
- } else {
- tombstones = null;
- }
- cachedFilesIterator = listing.createProvidedFileStatusIterator(
- S3Guard.dirMetaToStatuses(meta), ACCEPT_ALL, acceptor);
- if (allowAuthoritative && meta != null && meta.isAuthoritative()) {
- // metadata listing is authoritative, so return it directly
- return
listing.createLocatedFileStatusIterator(cachedFilesIterator);
- }
+ toLocatedFileStatus(status));
+ }
+ // Assuming the path to be a directory
+ // do a bulk operation.
+ RemoteIterator<S3ALocatedFileStatus> listFilesAssumingDir =
+ getListFilesAssumingDir(path,
+ recursive,
+ acceptor,
+ collectTombstones,
+ forceNonAuthoritativeMS);
+ // If there are no list entries present, we
+ // fallback to file existence check as the path
+ // can be a file or empty directory.
+ if (!listFilesAssumingDir.hasNext()) {
+ final S3AFileStatus fileStatus = (S3AFileStatus) getFileStatus(path);
Review comment:
innerGetFileStatus with probes for file and dir marker
-`HEAD_OR_DIR_MARKER`; we don't need to issue a second LIST call, do we? This
will save a call on listing a missing path
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileOperationCost.java
##########
@@ -168,6 +168,65 @@ public void testCostOfListLocatedStatusOnNonEmptyDir()
throws Throwable {
}
}
+ @Test
+ public void testCostOfListFilesOnFile() throws Throwable {
+ describe("Performing listFiles() on a file");
+ Path file = path(getMethodName() + ".txt");
+ S3AFileSystem fs = getFileSystem();
+ touch(fs, file);
+ resetMetricDiffs();
+ fs.listFiles(file, true);
+ if (!fs.hasMetadataStore()) {
+ metadataRequests.assertDiffEquals(1);
+ } else {
+ if (fs.allowAuthoritative(file)) {
+ listRequests.assertDiffEquals(0);
+ } else {
+ listRequests.assertDiffEquals(1);
+ }
+ }
+ }
+
+ @Test
+ public void testCostOfListFilesOnEmptyDir() throws Throwable {
+ describe("Performing listFiles() on an empty dir");
+ Path dir = path(getMethodName());
+ S3AFileSystem fs = getFileSystem();
+ fs.mkdirs(dir);
+ resetMetricDiffs();
+ fs.listFiles(dir, true);
+ if (!fs.hasMetadataStore()) {
+ verifyOperationCount(2, 1);
+ } else {
+ if (fs.allowAuthoritative(dir)) {
+ verifyOperationCount(0, 0);
+ } else {
+ verifyOperationCount(0, 1);
+ }
+ }
+ }
+
+ @Test
+ public void testCostOfListFilesOnNonEmptyDir() throws Throwable {
Review comment:
add a test on a nonexistent path and see what the cost changes to there
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
##########
@@ -4181,79 +4181,114 @@ public LocatedFileStatus next() throws IOException {
Path path = qualify(f);
LOG.debug("listFiles({}, {})", path, recursive);
try {
- // if a status was given, that is used, otherwise
- // call getFileStatus, which triggers an existence check
- final S3AFileStatus fileStatus = status != null
- ? status
- : (S3AFileStatus) getFileStatus(path);
- if (fileStatus.isFile()) {
+ // if a status was given and it is a file.
+ if (status != null && status.isFile()) {
// simple case: File
LOG.debug("Path is a file");
return new Listing.SingleStatusRemoteIterator(
- toLocatedFileStatus(fileStatus));
- } else {
- // directory: do a bulk operation
- String key = maybeAddTrailingSlash(pathToKey(path));
- String delimiter = recursive ? null : "/";
- LOG.debug("Requesting all entries under {} with delimiter '{}'",
- key, delimiter);
- final RemoteIterator<S3AFileStatus> cachedFilesIterator;
- final Set<Path> tombstones;
- boolean allowAuthoritative = allowAuthoritative(f);
- 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);
- tombstones = metadataStoreListFilesIterator.listTombstones();
- // if all of the below is true
- // - authoritative access is allowed for this metadatastore for
this directory,
- // - all the directory listings are authoritative on the client
- // - the caller does not force non-authoritative access
- // return the listing without any further s3 access
- if (!forceNonAuthoritativeMS &&
- allowAuthoritative &&
- metadataStoreListFilesIterator.isRecursivelyAuthoritative()) {
- S3AFileStatus[] statuses = S3Guard.iteratorToStatuses(
- metadataStoreListFilesIterator, tombstones);
- cachedFilesIterator = listing.createProvidedFileStatusIterator(
- statuses, ACCEPT_ALL, acceptor);
- return
listing.createLocatedFileStatusIterator(cachedFilesIterator);
- }
- cachedFilesIterator = metadataStoreListFilesIterator;
- } else {
- DirListingMetadata meta =
- S3Guard.listChildrenWithTtl(metadataStore, path, ttlTimeProvider,
- allowAuthoritative);
- if (meta != null) {
- tombstones = meta.listTombstones();
- } else {
- tombstones = null;
- }
- cachedFilesIterator = listing.createProvidedFileStatusIterator(
- S3Guard.dirMetaToStatuses(meta), ACCEPT_ALL, acceptor);
- if (allowAuthoritative && meta != null && meta.isAuthoritative()) {
- // metadata listing is authoritative, so return it directly
- return
listing.createLocatedFileStatusIterator(cachedFilesIterator);
- }
+ toLocatedFileStatus(status));
+ }
+ // Assuming the path to be a directory
+ // do a bulk operation.
+ RemoteIterator<S3ALocatedFileStatus> listFilesAssumingDir =
+ getListFilesAssumingDir(path,
+ recursive,
+ acceptor,
+ collectTombstones,
+ forceNonAuthoritativeMS);
+ // If there are no list entries present, we
+ // fallback to file existence check as the path
+ // can be a file or empty directory.
+ if (!listFilesAssumingDir.hasNext()) {
+ final S3AFileStatus fileStatus = (S3AFileStatus) getFileStatus(path);
+ if (fileStatus.isFile()) {
+ return new Listing.SingleStatusRemoteIterator(
+ toLocatedFileStatus(fileStatus));
}
- return listing.createTombstoneReconcilingIterator(
- listing.createLocatedFileStatusIterator(
- listing.createFileStatusListingIterator(path,
- createListObjectsRequest(key, delimiter),
- ACCEPT_ALL,
- acceptor,
- cachedFilesIterator)),
- collectTombstones ? tombstones : null);
}
+ // If we have reached here, it means either there are files
+ // in this directory or it is empty.
+ return listFilesAssumingDir;
} catch (AmazonClientException e) {
// TODO S3Guard: retry on file not found exception
Review comment:
we need to consider this and see if the TODO is already done.
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
##########
@@ -4181,79 +4181,114 @@ public LocatedFileStatus next() throws IOException {
Path path = qualify(f);
LOG.debug("listFiles({}, {})", path, recursive);
try {
- // if a status was given, that is used, otherwise
- // call getFileStatus, which triggers an existence check
- final S3AFileStatus fileStatus = status != null
- ? status
- : (S3AFileStatus) getFileStatus(path);
- if (fileStatus.isFile()) {
+ // if a status was given and it is a file.
+ if (status != null && status.isFile()) {
// simple case: File
LOG.debug("Path is a file");
return new Listing.SingleStatusRemoteIterator(
- toLocatedFileStatus(fileStatus));
- } else {
- // directory: do a bulk operation
- String key = maybeAddTrailingSlash(pathToKey(path));
- String delimiter = recursive ? null : "/";
- LOG.debug("Requesting all entries under {} with delimiter '{}'",
- key, delimiter);
- final RemoteIterator<S3AFileStatus> cachedFilesIterator;
- final Set<Path> tombstones;
- boolean allowAuthoritative = allowAuthoritative(f);
- 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);
- tombstones = metadataStoreListFilesIterator.listTombstones();
- // if all of the below is true
- // - authoritative access is allowed for this metadatastore for
this directory,
- // - all the directory listings are authoritative on the client
- // - the caller does not force non-authoritative access
- // return the listing without any further s3 access
- if (!forceNonAuthoritativeMS &&
- allowAuthoritative &&
- metadataStoreListFilesIterator.isRecursivelyAuthoritative()) {
- S3AFileStatus[] statuses = S3Guard.iteratorToStatuses(
- metadataStoreListFilesIterator, tombstones);
- cachedFilesIterator = listing.createProvidedFileStatusIterator(
- statuses, ACCEPT_ALL, acceptor);
- return
listing.createLocatedFileStatusIterator(cachedFilesIterator);
- }
- cachedFilesIterator = metadataStoreListFilesIterator;
- } else {
- DirListingMetadata meta =
- S3Guard.listChildrenWithTtl(metadataStore, path, ttlTimeProvider,
- allowAuthoritative);
- if (meta != null) {
- tombstones = meta.listTombstones();
- } else {
- tombstones = null;
- }
- cachedFilesIterator = listing.createProvidedFileStatusIterator(
- S3Guard.dirMetaToStatuses(meta), ACCEPT_ALL, acceptor);
- if (allowAuthoritative && meta != null && meta.isAuthoritative()) {
- // metadata listing is authoritative, so return it directly
- return
listing.createLocatedFileStatusIterator(cachedFilesIterator);
- }
+ toLocatedFileStatus(status));
+ }
+ // Assuming the path to be a directory
+ // do a bulk operation.
+ RemoteIterator<S3ALocatedFileStatus> listFilesAssumingDir =
+ getListFilesAssumingDir(path,
+ recursive,
+ acceptor,
+ collectTombstones,
+ forceNonAuthoritativeMS);
+ // If there are no list entries present, we
+ // fallback to file existence check as the path
+ // can be a file or empty directory.
+ if (!listFilesAssumingDir.hasNext()) {
+ final S3AFileStatus fileStatus = (S3AFileStatus) getFileStatus(path);
+ if (fileStatus.isFile()) {
+ return new Listing.SingleStatusRemoteIterator(
+ toLocatedFileStatus(fileStatus));
}
- return listing.createTombstoneReconcilingIterator(
- listing.createLocatedFileStatusIterator(
- listing.createFileStatusListingIterator(path,
- createListObjectsRequest(key, delimiter),
- ACCEPT_ALL,
- acceptor,
- cachedFilesIterator)),
- collectTombstones ? tombstones : null);
}
+ // If we have reached here, it means either there are files
+ // in this directory or it is empty.
+ return listFilesAssumingDir;
} catch (AmazonClientException e) {
// TODO S3Guard: retry on file not found exception
throw translateException("listFiles", path, e);
}
}
+ /**
+ * List files under a path assuming the path to be a directory.
Review comment:
i wonder if we can/should move this into the Listing class.
in #2046 I'm looking at doing that for openfile, just to pull something
complex out of the main fs. I know it hurts maintenance a bit (all subsequent
patches will need this patch before cherry picking), but your work is big
enough that will happen anyway.
imagine we made the `Listing` class a subclass of
`org.apache.hadoop.fs.s3a.impl.AbstractStoreOperation` and added a callback
interface purely to those s3a FS calls it needs - just like we do with rename
with `org.apache.hadoop.fs.s3a.impl.OperationCallbacks`. we'd then move all the
work over there and isolate it better.
of course, one thing this will make harder is for my directory marker
changes, but that shouldn't be a blocker.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
