Skip to content

HADOOP-17323 Skipping IO for root directory when empty directory flag is not required. #2479

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

Merged
merged 2 commits into from
Nov 24, 2020
Merged

HADOOP-17323 Skipping IO for root directory when empty directory flag is not required. #2479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
202b671
HADOOP-17323 Skipping IO for root directory when empty directory
mukund-thakur Nov 20, 2020
d5d9f6d
Review comments
mukund-thakur Nov 24, 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
4 changes: 4 additions & 0 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 @@ -3144,6 +3144,10 @@ S3AFileStatus s3GetFileStatus(final Path path,
"s3GetFileStatus(%s) wants to know if a directory is empty but"
+ " does not request a list probe", path);

if (key.isEmpty() && !needEmptyDirectoryFlag) {
return new S3AFileStatus(Tristate.UNKNOWN, path, username);
}

if (!key.isEmpty() && !key.endsWith("/")
&& probes.contains(StatusProbeEnum.Head)) {
try {
Expand Down
31 changes: 31 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 @@ -29,6 +29,7 @@
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.assertj.core.api.Assertions;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -258,6 +259,36 @@ public void testCostOfGetFileStatusOnMissingFile() throws Throwable {
GET_FILE_STATUS_FNFE);
}

@Test
public void testCostOfRootFileStatus() throws Throwable {
Path root = path("/");
S3AFileStatus rootStatus = verifyRawInnerGetFileStatus(
root,
false,
StatusProbeEnum.ALL,
ROOT_FILE_STATUS_PROBE);
String rootStatusContent = rootStatus.toString();
Assertions.assertThat(rootStatus.isDirectory())
Copy link
Contributor

Choose a reason for hiding this comment

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

for strictness, how about before L270 you do a status.toString() into a variable, and include that at the end of each describedAs message. That way, if there really was an assertion failure, we'd see the status

.describedAs("Status returned should be a directory "
+ rootStatusContent)
.isEqualTo(true);
Assertions.assertThat(rootStatus.isEmptyDirectory())
.isEqualTo(Tristate.UNKNOWN);

rootStatus = verifyRawInnerGetFileStatus(
root,
true,
StatusProbeEnum.ALL,
FILE_STATUS_DIR_PROBE);
Assertions.assertThat(rootStatus.isDirectory())
.describedAs("Status returned should be a directory "
+ rootStatusContent)
.isEqualTo(true);
Assertions.assertThat(rootStatus.isEmptyDirectory())
.isNotEqualByComparingTo(Tristate.UNKNOWN);

}

@Test
public void testIsDirIsFileMissingPath() throws Throwable {
describe("performing isDir and isFile on a missing file");
Expand Down
5 changes: 5 additions & 0 deletions ...op-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/performance/OperationCost.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,11 @@ public final class OperationCost {
*/
public static final OperationCost FILE_STATUS_FILE_PROBE = HEAD_OPERATION;

/**
* Cost of getFileStatus on root directory.
*/
public static final OperationCost ROOT_FILE_STATUS_PROBE = NO_IO;

/**
* Cost of {@link org.apache.hadoop.fs.s3a.impl.StatusProbeEnum#ALL}.
*/
Expand Down