From 8958ffb8832a763ccadc8eb9319db57f1d54b07c Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Wed, 4 Sep 2019 19:05:30 +0100 Subject: [PATCH 1/5] HADOOP-16547 make sure that s3guard prune sets up the FS Change-Id: Iaf71561cef6c797a3c66fed110faf08da6cac361 --- .../hadoop/fs/s3a/s3guard/S3GuardTool.java | 31 +++++++++++++++++-- .../s3guard/AbstractS3GuardToolTestBase.java | 11 ++++++- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java b/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java index 25a0cb04bb4a1..2eebbb4fe0a27 100644 --- a/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java +++ b/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java @@ -152,8 +152,10 @@ protected S3GuardTool(Configuration conf, String...opts) { /** * Parse DynamoDB region from either -m option or a S3 path. * - * This function should only be called from {@link S3GuardTool.Init} or - * {@link S3GuardTool.Destroy}. + * Note that as a side effect, if the paths included an S3 path, + * and there is no region set on the CLI, then the S3A FS is + * initialized, after which {@link #filesystem} will no longer + * be null. * * @param paths remaining parameters from CLI. * @throws IOException on I/O errors. @@ -338,6 +340,7 @@ protected MetadataStore initMetadataStore(boolean forceCreate) * @throws ExitUtil.ExitException if the FS is not an S3A FS */ protected void initS3AFileSystem(String path) throws IOException { + LOG.debug("Initializing S3A FS to {}", path); URI uri = toUri(path); // Make sure that S3AFileSystem does not hold an actual MetadataStore // implementation. @@ -363,6 +366,27 @@ protected void initS3AFileSystem(String path) throws IOException { filesystem = (S3AFileSystem) fs; } + /** + * Initialize the filesystem if there is none bonded to already and + * the command line path list is not empty. + * @param paths path list. + * @return true if at the end of the call, getFilesystem() is not null + * @throws IOException failure to instantiate. + */ + protected boolean maybeInitFilesystem(final List paths) + throws IOException { + // is there an S3 FS to create? + if (getFilesystem() == null) { + // none yet -create one + if (!paths.isEmpty()) { + initS3AFileSystem(paths.get(0)); + } else { + LOG.debug("No path on command line, so not instantiating FS"); + } + } + return getFilesystem() != null; + } + /** * Parse CLI arguments and returns the position arguments. * The options are stored in {@link #commandFormat}. @@ -1064,11 +1088,13 @@ public int run(String[] args, PrintStream out) throws InterruptedException, IOException { List paths = parseArgs(args); try { + checkBucketNameOrDDBTableNameProvided(paths); parseDynamoDBRegion(paths); } catch (ExitUtil.ExitException e) { errorln(USAGE); throw e; } + maybeInitFilesystem(paths); initMetadataStore(false); Configuration conf = getConf(); @@ -1113,6 +1139,7 @@ public int run(String[] args, PrintStream out) throws return SUCCESS; } + } /** diff --git a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/AbstractS3GuardToolTestBase.java b/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/AbstractS3GuardToolTestBase.java index b918500ef98d6..b65cdac938c48 100644 --- a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/AbstractS3GuardToolTestBase.java +++ b/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/AbstractS3GuardToolTestBase.java @@ -61,6 +61,7 @@ import static org.apache.hadoop.fs.s3a.Constants.S3_METADATA_STORE_IMPL; import static org.apache.hadoop.fs.s3a.S3AUtils.clearBucketOption; import static org.apache.hadoop.fs.s3a.s3guard.S3GuardTool.E_BAD_STATE; +import static org.apache.hadoop.fs.s3a.s3guard.S3GuardTool.INVALID_ARGUMENT; import static org.apache.hadoop.fs.s3a.s3guard.S3GuardTool.SUCCESS; import static org.apache.hadoop.fs.s3a.s3guard.S3GuardToolTestHelper.exec; import static org.apache.hadoop.test.LambdaTestUtils.intercept; @@ -335,6 +336,14 @@ public void testPruneCommandTombstones() throws Exception { testPath.toString()); } + @Test + public void testPruneCommandNoPath() throws Exception { + runToFailure(INVALID_ARGUMENT, + S3GuardTool.Prune.NAME, + "-" + S3GuardTool.Prune.TOMBSTONE, + "-seconds", "0"); + } + @Test public void testPruneCommandConf() throws Exception { getConfiguration().setLong(Constants.S3GUARD_CLI_PRUNE_AGE, @@ -476,7 +485,7 @@ public void testToolsNoArgsForBucket() throws Throwable { for (Class tool : tools) { S3GuardTool cmdR = makeBindedTool(tool); describe("Calling " + cmdR.getName() + " without any arguments."); - assertExitCode(S3GuardTool.INVALID_ARGUMENT, + assertExitCode(INVALID_ARGUMENT, intercept(ExitUtil.ExitException.class, () -> cmdR.run(new String[]{tool.getName()}))); } From 063b404fe801daf51667ff7c3130d7fd00d86047 Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Thu, 5 Sep 2019 12:40:35 +0100 Subject: [PATCH 2/5] HADOOP-16547 remove a spurious newline; no other checkstyles reported. Still not gone through a full test Change-Id: I5fd525b21eb67d1241b6af8d135862211dfda5b1 --- .../main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java b/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java index 2eebbb4fe0a27..6e2d6abe09e2c 100644 --- a/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java +++ b/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java @@ -1139,7 +1139,6 @@ public int run(String[] args, PrintStream out) throws return SUCCESS; } - } /** From 15fa0a0b0cf2b9cfc92fda670b32ecc72c240a46 Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Fri, 6 Sep 2019 21:27:00 +0100 Subject: [PATCH 3/5] HADOOP-16547 review other tool commands and have them instantiate the FS if a path was provided and region calculation didn't force this in I can't think of a good way to test this with a unit test, except maybe one involving delegation tokens Change-Id: I914f2c2b4d3691c5b7c5bdbb98b84ce2dcc3cba0 --- .../main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java b/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java index 6e2d6abe09e2c..40e6772a9caf6 100644 --- a/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java +++ b/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java @@ -616,6 +616,7 @@ public int run(String[] args, PrintStream out) throws Exception { // Validate parameters. try { parseDynamoDBRegion(paths); + maybeInitFilesystem(paths); } catch (ExitUtil.ExitException e) { errorln(USAGE); throw e; @@ -668,6 +669,7 @@ public int run(String[] args, PrintStream out) throws Exception { checkBucketNameOrDDBTableNameProvided(paths); checkIfS3BucketIsGuarded(paths); parseDynamoDBRegion(paths); + maybeInitFilesystem(paths); } catch (ExitUtil.ExitException e) { errorln(USAGE); throw e; From 76d489fb1d033c6ee4c6eb7bfb5e8d798f75d1b8 Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Tue, 17 Sep 2019 19:07:27 +0100 Subject: [PATCH 4/5] HADOOP-16547 add assertion in a test to verify prune instantiates the FS. Even before the fix, this new probe would only fail in certain test configurations -but at least now we have it. Change-Id: I1290ccacedd155f6fa876de5de2eea9799d9494a --- .../fs/s3a/s3guard/AbstractS3GuardToolTestBase.java | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/AbstractS3GuardToolTestBase.java b/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/AbstractS3GuardToolTestBase.java index b65cdac938c48..41a35ff214786 100644 --- a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/AbstractS3GuardToolTestBase.java +++ b/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/AbstractS3GuardToolTestBase.java @@ -31,7 +31,6 @@ import java.util.List; import java.util.Set; import java.util.UUID; -import java.util.concurrent.Callable; import java.util.concurrent.TimeUnit; import org.apache.hadoop.fs.s3a.S3AUtils; @@ -149,12 +148,7 @@ protected void runToFailure(int status, String... args) throws Exception { ExitUtil.ExitException ex = intercept(ExitUtil.ExitException.class, - new Callable() { - @Override - public Integer call() throws Exception { - return run(args); - } - }); + () -> run(args)); if (ex.status != status) { throw ex; } @@ -334,6 +328,8 @@ public void testPruneCommandTombstones() throws Exception { "prune", "-" + S3GuardTool.Prune.TOMBSTONE, "-seconds", "0", testPath.toString()); + assertNotNull("Command did not create a filesystem", + cmd.getFilesystem()); } @Test From 90e2ab0689261bdaf797fcb0ffcc79127d521648 Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Wed, 18 Sep 2019 13:22:41 +0100 Subject: [PATCH 5/5] HADOOP-16547. more tests Change-Id: I156e563ce64d1d031706075463d693b965c4a201 --- .../hadoop/fs/s3a/s3guard/S3GuardTool.java | 3 ++- .../s3guard/AbstractS3GuardToolTestBase.java | 24 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java b/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java index 40e6772a9caf6..9cb1efe380fda 100644 --- a/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java +++ b/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java @@ -373,7 +373,8 @@ protected void initS3AFileSystem(String path) throws IOException { * @return true if at the end of the call, getFilesystem() is not null * @throws IOException failure to instantiate. */ - protected boolean maybeInitFilesystem(final List paths) + @VisibleForTesting + boolean maybeInitFilesystem(final List paths) throws IOException { // is there an S3 FS to create? if (getFilesystem() == null) { diff --git a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/AbstractS3GuardToolTestBase.java b/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/AbstractS3GuardToolTestBase.java index 41a35ff214786..2d17ca5424957 100644 --- a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/AbstractS3GuardToolTestBase.java +++ b/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/AbstractS3GuardToolTestBase.java @@ -27,6 +27,7 @@ import java.net.URI; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -332,6 +333,29 @@ public void testPruneCommandTombstones() throws Exception { cmd.getFilesystem()); } + /** + * HADOOP-16457. In certain cases prune doesn't create an FS. + */ + @Test + public void testMaybeInitFilesystem() throws Exception { + Path testPath = path("maybeInitFilesystem"); + S3GuardTool.Prune cmd = new S3GuardTool.Prune(getFileSystem().getConf()); + cmd.maybeInitFilesystem(Collections.singletonList(testPath.toString())); + assertNotNull("Command did not create a filesystem", + cmd.getFilesystem()); + } + + /** + * HADOOP-16457. In certain cases prune doesn't create an FS. + */ + @Test + public void testMaybeInitFilesystemNoPath() throws Exception { + S3GuardTool.Prune cmd = new S3GuardTool.Prune(getFileSystem().getConf()); + cmd.maybeInitFilesystem(Collections.emptyList()); + assertNull("Command should not have created a filesystem", + cmd.getFilesystem()); + } + @Test public void testPruneCommandNoPath() throws Exception { runToFailure(INVALID_ARGUMENT,