Skip to content

HADOOP-17022 Tune S3AFileSystem.listFiles() api. #2038

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

HADOOP-17022 Tune S3AFileSystem.listFiles() api. #2038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fa8093e
HADOOP-17022 Tune S3AFileSystem.listFiles() api.
mukund-thakur May 28, 2020
35ff50e
Adding listFiles() and listLocatedStatus() checks in ITestAssumeRole
mukund-thakur Jun 10, 2020
25ccf2d
Adding test for operations cost of listFiles() non existing directory
mukund-thakur Jun 11, 2020
36aec4d
Fix for test failures. Regression
mukund-thakur Jun 23, 2020
5b631fa
Fixing raw s3 test failures and some checkstyle
mukund-thakur Jun 25, 2020
abdbbbb
Reusing passed filestatus
mukund-thakur Jul 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
171 changes: 109 additions & 62 deletions hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4181,79 +4181,126 @@ private RemoteIterator<S3ALocatedFileStatus> innerListFiles(
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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we are keeping this, include the path f in the log message, just to make more sense of it in a busy log

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()) {
// If file status was already passed, reuse it.
final S3AFileStatus fileStatus = status != null
? status
: (S3AFileStatus) getFileStatus(path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's go to innerGetFileStatus here so the invocation count is consistent and we can declare we don't want to fall back to the list operation, just StatusProbeEnum.HEAD_OR_DIR_MARKER. Saves a LIST call when listing a nonexistent path. Also lets us avoid that cast

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work as explained in #2038 (comment)

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to consider this and see if the TODO is already done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any retries in listFiles().
Even the listLocatedStatus() and listStatus() are called with Invoker.Once(). That means just the calls with exception translated without any retries.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets cut that TODO line

throw translateException("listFiles", path, e);
}
}

/**
* List files under a path assuming the path to be a directory.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* @param path input path.
* @param recursive recursive listing?
* @param acceptor file status filter
* @param collectTombstones should tombstones be collected from S3Guard?
* @param forceNonAuthoritativeMS forces metadata store to act like non
* authoritative. This is useful when
* listFiles output is used by import tool.
* @return an iterator over listing.
* @throws IOException any exception.
*/
private RemoteIterator<S3ALocatedFileStatus> getListFilesAssumingDir(
Path path,
boolean recursive, Listing.FileStatusAcceptor acceptor,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add on a new line

boolean collectTombstones,
boolean forceNonAuthoritativeMS) throws IOException {

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(path);
if (recursive) {
final PathMetadata pm = metadataStore.get(path, true);
if (pm != null) {
if (pm.isDeleted()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a copy/paste of the other use of this? if so, what about we pull lines 4250-4255 into a new static method in org.apache.hadoop.fs.s3a.s3guard.S3Guard and use in both places. could even have a test, maybe?

OffsetDateTime deletedAt = OffsetDateTime
.ofInstant(Instant.ofEpochMilli(
pm.getFileStatus().getModificationTime()),
ZoneOffset.UTC);
throw new FileNotFoundException("Path " + path + " is recorded as " +
"deleted by S3Guard at " + deletedAt);
}
}
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);
}
}
return listing.createTombstoneReconcilingIterator(
listing.createLocatedFileStatusIterator(
listing.createFileStatusListingIterator(path,
createListObjectsRequest(key, delimiter),
ACCEPT_ALL,
acceptor,
cachedFilesIterator)),
collectTombstones ? tombstones : null);
}

/**
* Override superclass so as to add statistic collection.
* {@inheritDoc}
Expand Down
70 changes: 70 additions & 0 deletions ...op-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileOperationCost.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,76 @@ 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 {
describe("Performing listFiles() on a non empty dir");
Path dir = path(getMethodName());
S3AFileSystem fs = getFileSystem();
fs.mkdirs(dir);
Path file = new Path(dir, "file.txt");
touch(fs, file);
resetMetricDiffs();
fs.listFiles(dir, true);
if (!fs.hasMetadataStore()) {
verifyOperationCount(0, 1);
} else {
if (fs.allowAuthoritative(dir)) {
verifyOperationCount(0, 0);
} else {
verifyOperationCount(0, 1);
}
}
}

@Test
public void testCostOfListFilesOnNonExistingDir() throws Throwable {
describe("Performing listFiles() on a non existing dir");
Path dir = path(getMethodName());
S3AFileSystem fs = getFileSystem();
resetMetricDiffs();
intercept(FileNotFoundException.class,
() -> fs.listFiles(dir, true));
verifyOperationCount(2, 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with a move to innerGetFileStatus and only do the file/marker probes, we could get cost to (2, 1)

}

@Test
public void testCostOfGetFileStatusOnFile() throws Throwable {
describe("performing getFileStatus on a file");
Expand Down
6 changes: 5 additions & 1 deletion ...-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3GuardListConsistency.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.fs.RemoteIterator;
import org.apache.hadoop.fs.contract.AbstractFSContract;
import org.apache.hadoop.fs.contract.ContractTestUtils;
import org.apache.hadoop.fs.contract.s3a.S3AContract;

import com.google.common.collect.Lists;
Expand Down Expand Up @@ -271,7 +272,10 @@ public void testConsistentRenameAfterDelete() throws Exception {
assertTrue(fs.delete(testDirs[1], false));
assertTrue(fs.delete(testDirs[2], false));

fs.rename(path("a"), path("a3"));
ContractTestUtils.rename(fs, path("a"), path("a3"));
ContractTestUtils.assertPathsDoNotExist(fs,
"Source paths shouldn't exist post rename operation",
testDirs[0], testDirs[1], testDirs[2]);
FileStatus[] paths = fs.listStatus(path("a3/b"));
List<Path> list = new ArrayList<>();
for (FileStatus fileState : paths) {
Expand Down
4 changes: 4 additions & 0 deletions hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/auth/ITestAssumeRole.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,10 @@ public void testAssumeRoleRestrictedPolicyFS() throws Exception {
}
forbidden("",
() -> fs.listStatus(ROOT));
forbidden("",
() -> fs.listFiles(ROOT, true));
forbidden("",
() -> fs.listLocatedStatus(ROOT));
forbidden("",
() -> fs.mkdirs(path("testAssumeRoleFS")));
}
Expand Down