Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: HADOOP-13345
    • Fix Version/s: HADOOP-13345
    • Component/s: documentation, fs/s3
    • Labels:
      None

      Description

      Pre-merge cleanup while it's still easy to do

      • Read through all the docs, tune
      • Diff the trunk/branch files to see if we can reduce the delta (and hence the changes)
      • Review the new tests
      1. HADOOP-14749-HADOOP-13345-001.patch
        119 kB
        Steve Loughran
      2. HADOOP-14749-HADOOP-13345-002.patch
        116 kB
        Steve Loughran
      3. HADOOP-14749-HADOOP-13345-003.patch
        123 kB
        Steve Loughran
      4. HADOOP-14749-HADOOP-13345-004.patch
        126 kB
        Steve Loughran
      5. HADOOP-14749-HADOOP-13345-005.patch
        135 kB
        Steve Loughran
      6. HADOOP-14749-HADOOP-13345-006.patch
        137 kB
        Steve Loughran
      7. HADOOP-14749-HADOOP-13345-007.patch
        138 kB
        Steve Loughran
      8. HADOOP-14749-HADOOP-13345-008.patch
        140 kB
        Steve Loughran

        Issue Links

          Activity

          Hide
          stevel@apache.org Steve Loughran added a comment -

          +

          • review javadocs
          • arranging imports in roughly the same order as our style requirements.
          • review tests
          Show
          stevel@apache.org Steve Loughran added a comment - + review javadocs arranging imports in roughly the same order as our style requirements. review tests
          Hide
          stevel@apache.org Steve Loughran added a comment - - edited

          Big review

          • docs reviewed, edited. Added: per-bucket config example, security, more troubleshooting.
          • moved section on testing into main testing.md file
          • javadocs audited
          • moved imports on new files into the project's preferred order.
          • tuned the tests

          Other than javadocs, imports and some layout, the only real code change in production is to use a switch statement in S3AFileSystem.innerMkdirs().

          Show
          stevel@apache.org Steve Loughran added a comment - - edited Big review docs reviewed, edited. Added: per-bucket config example, security, more troubleshooting. moved section on testing into main testing.md file javadocs audited moved imports on new files into the project's preferred order. tuned the tests Other than javadocs, imports and some layout, the only real code change in production is to use a switch statement in S3AFileSystem.innerMkdirs() .
          Hide
          stevel@apache.org Steve Loughran added a comment -

          S3Guard.assertQualified added a vargs version to make things shorter...not something I'm too opinionated about

          DirectoryStatus checkPathForDirectory seems to always go to S3 if the path maps to a file, even if the store has a record in s3guard. Have I misread it?

          site docs

          • should we use the term MetadataStore in the docs, or Metadata Store?
          • what architecture doc should go in? There's a lot in the javadocs...we could just say "look there", but its nice to have a good online doc we can point people at.
          Show
          stevel@apache.org Steve Loughran added a comment - S3Guard.assertQualified added a vargs version to make things shorter...not something I'm too opinionated about DirectoryStatus checkPathForDirectory seems to always go to S3 if the path maps to a file, even if the store has a record in s3guard. Have I misread it? site docs should we use the term MetadataStore in the docs, or Metadata Store ? what architecture doc should go in? There's a lot in the javadocs...we could just say "look there", but its nice to have a good online doc we can point people at.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Testing

          All well apart from existing failures HADOOP-14735 and HADOOP-14733 (patches available); one run failed with HADOOP-14750 stack trace

          Show
          stevel@apache.org Steve Loughran added a comment - Testing All well apart from existing failures HADOOP-14735 and HADOOP-14733 (patches available); one run failed with HADOOP-14750 stack trace
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 0s Docker mode activated.
          -1 patch 0m 6s HADOOP-14749 does not apply to HADOOP-13345. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help.



          Subsystem Report/Notes
          JIRA Issue HADOOP-14749
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12880921/HADOOP-14749-HADOOP-13345-001.patch
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12990/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. -1 patch 0m 6s HADOOP-14749 does not apply to HADOOP-13345 . Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help. Subsystem Report/Notes JIRA Issue HADOOP-14749 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12880921/HADOOP-14749-HADOOP-13345-001.patch Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12990/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          fabbri Aaron Fabbri added a comment -

          Thanks for the patch Steve Loughran. This is good stuff.

             /**
          -   * Should not be called by clients.  Only used so {@link org.apache.hadoop
          -   * .fs.s3a.s3guard.MetadataStore} can maintain this flag when caching
          -   * FileStatuses on behalf of s3a.
          +   * Should not be called by clients.  Only used so {@code MetadataStore}
          +   * can maintain this flag when caching FileStatuses on behalf of s3a.
              * @param value for directories: TRUE / FALSE if known empty/not-empty,
              *              UNKNOWN otherwise
              */
          

          Actually, can we remove setIsEmptyDirectory() now? IIRC this is not used since I reworked the empty directory handling logic.

          +          // with a metadata store, the object entries need tup be updated,
          +          // including, potentially, the ancestors
          

          /tup/to/

          +  /**
          +   * Determine the directory status of a path, going via any
          +   * MetadataStore before checking S3.
          +   * @param path path to check
          +   * @return the determined status
          +   * @throws IOException IO failure other than FileNotFoundException
          +   */
             private DirectoryStatus checkPathForDirectory(Path path) throws
                 IOException {
          

          I thought HADOOP-14505 eliminated checkPathForDirectory()? I had suggested just using getFileStatus() would be more efficient and less code.

          +            // metadata listing is authoritative, so return it directory
          

          /directory/directly/ ?

          -    // If FileStatus' path is missing host, but should have one, add it.
          +    // If FileStatus's path is missing host, but should have one, add it.
          

          Either is correct, BTW.

          -    assertQualified(srcRoot);
          -    assertQualified(srcPath);
          -    assertQualified(dstPath);
          +    assertQualified(srcRoot, srcPath, dstPath);
          

          Nice.

          +#### Errpr `"DynamoDB table TABLE does not exist in region REGION; auto-creation is turned off"`
          

          /Errpr/Error/

          The docs changes look good, but the diff became a bit hard to follow. Looks like you moved some stuff to testing doc, which is fine.

          Show
          fabbri Aaron Fabbri added a comment - Thanks for the patch Steve Loughran . This is good stuff. /** - * Should not be called by clients. Only used so {@link org.apache.hadoop - * .fs.s3a.s3guard.MetadataStore} can maintain this flag when caching - * FileStatuses on behalf of s3a. + * Should not be called by clients. Only used so {@code MetadataStore} + * can maintain this flag when caching FileStatuses on behalf of s3a. * @param value for directories: TRUE / FALSE if known empty/not-empty, * UNKNOWN otherwise */ Actually, can we remove setIsEmptyDirectory() now? IIRC this is not used since I reworked the empty directory handling logic. + // with a metadata store, the object entries need tup be updated, + // including, potentially, the ancestors /tup/to/ + /** + * Determine the directory status of a path, going via any + * MetadataStore before checking S3. + * @param path path to check + * @return the determined status + * @throws IOException IO failure other than FileNotFoundException + */ private DirectoryStatus checkPathForDirectory(Path path) throws IOException { I thought HADOOP-14505 eliminated checkPathForDirectory()? I had suggested just using getFileStatus() would be more efficient and less code. + // metadata listing is authoritative, so return it directory /directory/directly/ ? - // If FileStatus' path is missing host, but should have one, add it. + // If FileStatus's path is missing host, but should have one, add it. Either is correct, BTW. - assertQualified(srcRoot); - assertQualified(srcPath); - assertQualified(dstPath); + assertQualified(srcRoot, srcPath, dstPath); Nice. +#### Errpr `"DynamoDB table TABLE does not exist in region REGION; auto-creation is turned off"` /Errpr/Error/ The docs changes look good, but the diff became a bit hard to follow. Looks like you moved some stuff to testing doc, which is fine.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Patch 002; sync with s3guard after the various patch-pending patches went in. Essentially: less to review.

          While looking at the diff, I'm now worried about the high-ascii chars in the illustration in TestDynamoDBMetadataStore.verifyRootDirectory(). It's a lovely diagram, and I had to look at it to see how it was done —which is with chars > 0x80. I don't know how well this works in different locale; I do know we can't use other high ascii symbols, eg. "—" without encoding to &mdash. (I say that, but a quick scan for "—" shows lots of uses in hadoop-aws, and I probably the guilty party. We should perhaps fix that.

          Show
          stevel@apache.org Steve Loughran added a comment - Patch 002; sync with s3guard after the various patch-pending patches went in. Essentially: less to review. While looking at the diff, I'm now worried about the high-ascii chars in the illustration in TestDynamoDBMetadataStore.verifyRootDirectory() . It's a lovely diagram, and I had to look at it to see how it was done —which is with chars > 0x80. I don't know how well this works in different locale; I do know we can't use other high ascii symbols, eg. "—" without encoding to &mdash. (I say that, but a quick scan for "—" shows lots of uses in hadoop-aws, and I probably the guilty party. We should perhaps fix that.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Aaron: just seen your comments. Yes, the patch was out of date. And I have moved all s3guard testing into the "testing" doc as everyone testing s3a needs to know about it, while general s3guard users don't.

          I'll do a revised patch

          Show
          stevel@apache.org Steve Loughran added a comment - Aaron: just seen your comments. Yes, the patch was out of date. And I have moved all s3guard testing into the "testing" doc as everyone testing s3a needs to know about it, while general s3guard users don't. I'll do a revised patch
          Hide
          stevel@apache.org Steve Loughran added a comment -

          + feedback from Ewan Higgs

          +          // with a metadata store, the object entries need tup be updated,
          Grammar/spelling.
           
          +   * This will always be non-null, but may be bound to the
          If something will be not null, maybe use @NotNull. I don’t see any uses of it yet in the Hadoop codebase, so maybe someone decided against using it.
           
          +      if (status == DirectoryStatus.DOES_NOT_EXIST
          +          || status == DirectoryStatus.EXISTS_AND_IS_DIRECTORY_ON_S3_ONLY) {
          I think this indents the || one too many. checkstyle should pick it up.
           
          +      // TODO s3guard: retry on file not found exception
          Other places you are normalizing spelling to use capital S and capital G (even in comments) and the nature of this patch is nit fixes... :)
           
          +   * Generally,  callers should use {@link #initialize(FileSystem)}
          +   * with an initialized S3 file system.
           
          A wise man once said “Object Stores are not File Systems”. So do we want “with an initialized {@link S3AFileSystem} ? or “initialized S3 FileSystem” so it includes S3 and S3N (which will will be removed soon).
           
          +   * Without a filesystem to act as a reference point, the configuration itself
          file system or filesystem. cf previous comment.
           
          +#### Errpr `"DynamoDB table TABLE does not exist in region REGION; auto-creation is turned off"`
          Error (spelling).
           
          +
          +### Warning About Concurrent Tests
          +
          +You must not run S3A and S3N tests in parallel on the same bucket.  This is
          +especially true when S3Guard is enabled.  S3Guard requires that all clients
          +that are modifying the bucket have S3Guard enabled, so having S3N
          +integration tests running in parallel with S3A tests will cause strange
          +failures.
           
          So if someone adds to the bucket using s3cmd in production what will happen? This seems like a severe limitation that can effect of ephemeral mounts for Provided Storage where a purpose is to async repl between s3 and hdfs.
           
          +The two S3Guard scale testse are `ITestDynamoDBMetadataStoreScale` and
          tests (spelling)
          
          Show
          stevel@apache.org Steve Loughran added a comment - + feedback from Ewan Higgs + // with a metadata store, the object entries need tup be updated, Grammar/spelling. + * This will always be non- null , but may be bound to the If something will be not null , maybe use @NotNull. I don’t see any uses of it yet in the Hadoop codebase, so maybe someone decided against using it. + if (status == DirectoryStatus.DOES_NOT_EXIST + || status == DirectoryStatus.EXISTS_AND_IS_DIRECTORY_ON_S3_ONLY) { I think this indents the || one too many. checkstyle should pick it up. + // TODO s3guard: retry on file not found exception Other places you are normalizing spelling to use capital S and capital G (even in comments) and the nature of this patch is nit fixes... :) + * Generally, callers should use {@link #initialize(FileSystem)} + * with an initialized S3 file system. A wise man once said “ Object Stores are not File Systems”. So do we want “with an initialized {@link S3AFileSystem} ? or “initialized S3 FileSystem” so it includes S3 and S3N (which will will be removed soon). + * Without a filesystem to act as a reference point, the configuration itself file system or filesystem. cf previous comment. +#### Errpr ` "DynamoDB table TABLE does not exist in region REGION; auto-creation is turned off" ` Error (spelling). + +### Warning About Concurrent Tests + +You must not run S3A and S3N tests in parallel on the same bucket. This is +especially true when S3Guard is enabled. S3Guard requires that all clients +that are modifying the bucket have S3Guard enabled, so having S3N +integration tests running in parallel with S3A tests will cause strange +failures. So if someone adds to the bucket using s3cmd in production what will happen? This seems like a severe limitation that can effect of ephemeral mounts for Provided Storage where a purpose is to async repl between s3 and hdfs. +The two S3Guard scale testse are `ITestDynamoDBMetadataStoreScale` and tests (spelling)
          Hide
          stevel@apache.org Steve Loughran added a comment -

          patch 003

          Ewan and Aaron's suggestions, plus use S3Guard in all comments and strings where it is appropriate

          Aaron, regarding "FileStatus' path" vs "If FileStatus's path" , I'm now worried that either I've got it wrong or there's one of those US:UK rule variants. I'll go with yours

          Show
          stevel@apache.org Steve Loughran added a comment - patch 003 Ewan and Aaron's suggestions, plus use S3Guard in all comments and strings where it is appropriate Aaron, regarding "FileStatus' path" vs "If FileStatus's path" , I'm now worried that either I've got it wrong or there's one of those US:UK rule variants. I'll go with yours
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 13s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 15 new or modified test files.
                HADOOP-13345 Compile Tests
          +1 mvninstall 13m 25s HADOOP-13345 passed
          +1 compile 0m 22s HADOOP-13345 passed
          +1 checkstyle 0m 15s HADOOP-13345 passed
          +1 mvnsite 0m 24s HADOOP-13345 passed
          +1 findbugs 0m 30s HADOOP-13345 passed
          +1 javadoc 0m 14s HADOOP-13345 passed
                Patch Compile Tests
          +1 mvninstall 0m 19s the patch passed
          +1 compile 0m 19s the patch passed
          +1 javac 0m 19s the patch passed
          -0 checkstyle 0m 13s hadoop-tools/hadoop-aws: The patch generated 22 new + 55 unchanged - 4 fixed = 77 total (was 59)
          +1 mvnsite 0m 23s the patch passed
          -1 whitespace 0m 0s The patch has 4 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 0m 39s the patch passed
          +1 javadoc 0m 13s the patch passed
                Other Tests
          +1 unit 0m 38s hadoop-aws in the patch passed.
          +1 asflicense 0m 17s The patch does not generate ASF License warnings.
          19m 46s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14749
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881045/HADOOP-14749-HADOOP-13345-003.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle
          uname Linux dd0430918c9b 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision HADOOP-13345 / b4c2ab2
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12995/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-aws.txt
          whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/12995/artifact/patchprocess/whitespace-eol.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12995/testReport/
          modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12995/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 13s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 15 new or modified test files.       HADOOP-13345 Compile Tests +1 mvninstall 13m 25s HADOOP-13345 passed +1 compile 0m 22s HADOOP-13345 passed +1 checkstyle 0m 15s HADOOP-13345 passed +1 mvnsite 0m 24s HADOOP-13345 passed +1 findbugs 0m 30s HADOOP-13345 passed +1 javadoc 0m 14s HADOOP-13345 passed       Patch Compile Tests +1 mvninstall 0m 19s the patch passed +1 compile 0m 19s the patch passed +1 javac 0m 19s the patch passed -0 checkstyle 0m 13s hadoop-tools/hadoop-aws: The patch generated 22 new + 55 unchanged - 4 fixed = 77 total (was 59) +1 mvnsite 0m 23s the patch passed -1 whitespace 0m 0s The patch has 4 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 0m 39s the patch passed +1 javadoc 0m 13s the patch passed       Other Tests +1 unit 0m 38s hadoop-aws in the patch passed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 19m 46s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14749 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881045/HADOOP-14749-HADOOP-13345-003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle uname Linux dd0430918c9b 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision HADOOP-13345 / b4c2ab2 Default Java 1.8.0_131 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12995/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-aws.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/12995/artifact/patchprocess/whitespace-eol.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12995/testReport/ modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12995/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          fabbri Aaron Fabbri added a comment -

          +1 on v3 patch.

          Show
          fabbri Aaron Fabbri added a comment - +1 on v3 patch.
          Hide
          liuml07 Mingliang Liu added a comment -

          +1. Nice work. Thanks Steve Loughran.

          Nits:

          1. I saw a few TODOs that do not have associated JIRA numbers. Should we file and point to them?
          2. According to my experience at Amazon, DynamoDB and Dynamo are two different systems though they share lots of core principles and design. Should we replace all dynamo in doc/comment as DynamoDB?
          3. In S3GuardTool L1130, code System.exit() on all exeuction paths. should be @code System.exit() on all exeuction paths. This has a broader question: we currently don't use javadoc to generate HTML doc anymore (don't we?), so perhaps we don't need those HTML tags in javadoc which most serves as comment. I saw some usage of <li> for e.g.
          4. In doc, should we also mention sharing DDB table amortizes the provision burden besides cost-effective?
          5. In doc, there is duplicate "uses" in sentence +service uses uses the same authentication mechanisms as S3. S3Guard
          6. +### Delete a table: `s3guard destroy` has double spaces before destroy
          7. In the testing doc,

            ... launch the server if it is not yet started; creating the table if it does not exist.

            DynamoDBLocalClientFactory is starting a new in-memory local server whose instance or data is not shared among tests. So it always starts a new server, and create new table. Need to confirm.

          Show
          liuml07 Mingliang Liu added a comment - +1. Nice work. Thanks Steve Loughran . Nits: I saw a few TODOs that do not have associated JIRA numbers. Should we file and point to them? According to my experience at Amazon, DynamoDB and Dynamo are two different systems though they share lots of core principles and design. Should we replace all dynamo in doc/comment as DynamoDB ? In S3GuardTool L1130, code System.exit() on all exeuction paths. should be @code System.exit() on all exeuction paths. This has a broader question: we currently don't use javadoc to generate HTML doc anymore (don't we?), so perhaps we don't need those HTML tags in javadoc which most serves as comment. I saw some usage of <li> for e.g. In doc, should we also mention sharing DDB table amortizes the provision burden besides cost-effective? In doc, there is duplicate "uses" in sentence +service uses uses the same authentication mechanisms as S3. S3Guard +### Delete a table: `s3guard destroy` has double spaces before destroy In the testing doc, ... launch the server if it is not yet started; creating the table if it does not exist. DynamoDBLocalClientFactory is starting a new in-memory local server whose instance or data is not shared among tests. So it always starts a new server, and create new table. Need to confirm.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          thx, I'll go through Mingliang's comments and do a final revision.

          • yes. should use DynamoDB everywhere.
          • we do generate javadocs, though I'm not sure the publishing is code. So I'm trying to be consistent.
          • provision burden? You mean allow us to overspecify for peak loads. Makes sense

          BTW, one situation I'm thinking about now: how to handle s3guard as a read-only client. That is, you can get the auth/hon-auth data from others, but you can't write back yourself (indeed, can't write back to the FS)

          Show
          stevel@apache.org Steve Loughran added a comment - thx, I'll go through Mingliang's comments and do a final revision. yes. should use DynamoDB everywhere. we do generate javadocs, though I'm not sure the publishing is code. So I'm trying to be consistent. provision burden? You mean allow us to overspecify for peak loads. Makes sense BTW, one situation I'm thinking about now: how to handle s3guard as a read-only client. That is, you can get the auth/hon-auth data from others, but you can't write back yourself (indeed, can't write back to the FS)
          Hide
          liuml07 Mingliang Liu added a comment -

          provision burden?

          I was thinking that, suppose a user has dozens of buckets in a region, if each bucket has a dedicated DDB table, then the user will have to provision dozens of tables according to each table's peak/idle load. Instead, if she shares the metadata in a single DDB table for all the buckets in that region, she will need to only provision one table capacity according to overall usage. This amortizes the provision burden.

          how to handle s3guard as a read-only client.

          I know IAM role can have fine granularity about READ access to S3 bucket (e.g. "s3:GetObject") and DDB table (e.g. "dynamodb:Query", "dynamodb:Get" etc). This might be considered/operated by the user. But in code, we should not populate the metadata from S3 to DDB in the read-only case.

          Show
          liuml07 Mingliang Liu added a comment - provision burden? I was thinking that, suppose a user has dozens of buckets in a region, if each bucket has a dedicated DDB table, then the user will have to provision dozens of tables according to each table's peak/idle load. Instead, if she shares the metadata in a single DDB table for all the buckets in that region, she will need to only provision one table capacity according to overall usage. This amortizes the provision burden. how to handle s3guard as a read-only client. I know IAM role can have fine granularity about READ access to S3 bucket (e.g. "s3:GetObject") and DDB table (e.g. "dynamodb:Query", "dynamodb:Get" etc). This might be considered/operated by the user. But in code, we should not populate the metadata from S3 to DDB in the read-only case.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Patch 004; add Mingliang's comments

          • DynamoDB everywhere
          • Look @ TODOs and add any new JIRAs
          • explain provisioning better. Also: section on when you wouldn't want to share (security, per-project billing, isolated IO provisioning).

          Rerun all tests; all is well! I'm going with the previous +1 votes once yetus is happy, then do the next big item: prepare a single diff for yetus to review, that is: a single checkstyle/findbugs review of all the added code.

          Show
          stevel@apache.org Steve Loughran added a comment - Patch 004; add Mingliang's comments DynamoDB everywhere Look @ TODOs and add any new JIRAs explain provisioning better. Also: section on when you wouldn't want to share (security, per-project billing, isolated IO provisioning). Rerun all tests; all is well! I'm going with the previous +1 votes once yetus is happy, then do the next big item: prepare a single diff for yetus to review, that is: a single checkstyle/findbugs review of all the added code.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 16 new or modified test files.
                HADOOP-13345 Compile Tests
          +1 mvninstall 14m 20s HADOOP-13345 passed
          +1 compile 0m 21s HADOOP-13345 passed
          +1 checkstyle 0m 16s HADOOP-13345 passed
          +1 mvnsite 0m 23s HADOOP-13345 passed
          +1 findbugs 0m 32s HADOOP-13345 passed
          +1 javadoc 0m 16s HADOOP-13345 passed
                Patch Compile Tests
          +1 mvninstall 0m 20s the patch passed
          +1 compile 0m 18s the patch passed
          +1 javac 0m 18s the patch passed
          -0 checkstyle 0m 13s hadoop-tools/hadoop-aws: The patch generated 22 new + 55 unchanged - 4 fixed = 77 total (was 59)
          +1 mvnsite 0m 21s the patch passed
          -1 whitespace 0m 0s The patch has 5 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 0m 33s the patch passed
          +1 javadoc 0m 12s the patch passed
                Other Tests
          +1 unit 0m 36s hadoop-aws in the patch passed.
          +1 asflicense 0m 15s The patch does not generate ASF License warnings.
          20m 34s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14749
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881459/HADOOP-14749-HADOOP-13345-004.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle
          uname Linux c900fd7fa4dd 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision HADOOP-13345 / b114f24
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/13007/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-aws.txt
          whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/13007/artifact/patchprocess/whitespace-eol.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13007/testReport/
          modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13007/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 16 new or modified test files.       HADOOP-13345 Compile Tests +1 mvninstall 14m 20s HADOOP-13345 passed +1 compile 0m 21s HADOOP-13345 passed +1 checkstyle 0m 16s HADOOP-13345 passed +1 mvnsite 0m 23s HADOOP-13345 passed +1 findbugs 0m 32s HADOOP-13345 passed +1 javadoc 0m 16s HADOOP-13345 passed       Patch Compile Tests +1 mvninstall 0m 20s the patch passed +1 compile 0m 18s the patch passed +1 javac 0m 18s the patch passed -0 checkstyle 0m 13s hadoop-tools/hadoop-aws: The patch generated 22 new + 55 unchanged - 4 fixed = 77 total (was 59) +1 mvnsite 0m 21s the patch passed -1 whitespace 0m 0s The patch has 5 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 0m 33s the patch passed +1 javadoc 0m 12s the patch passed       Other Tests +1 unit 0m 36s hadoop-aws in the patch passed. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 20m 34s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14749 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881459/HADOOP-14749-HADOOP-13345-004.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle uname Linux c900fd7fa4dd 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision HADOOP-13345 / b114f24 Default Java 1.8.0_144 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/13007/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-aws.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/13007/artifact/patchprocess/whitespace-eol.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13007/testReport/ modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13007/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Cancel patch & doing another one with improve CLI docs: titles, text, examples.

          One issue not clear from the current one. How does s3guard prune determine age of entries to delete?

          • file age
          • entry creation date?

          There's no atime value so it's not "since used", is it?

          Show
          stevel@apache.org Steve Loughran added a comment - Cancel patch & doing another one with improve CLI docs: titles, text, examples. One issue not clear from the current one. How does s3guard prune determine age of entries to delete? file age entry creation date? There's no atime value so it's not "since used", is it?
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I know the answer to that now. I also see that you can configure DDB with an explicit TTL. If we added a field for each entry as to when the record itself was created, then we could have AWS TTL do the pruning automatically.

          Also, on the CLI I'm getting told off for an encryption key I don' think I've set

          ./hadoop s3guard import  s3a://hwdev-steve-ireland-new
          2017-08-11 17:56:50,139 INFO Configuration.deprecation: fs.s3a.server-side-encryption-key is deprecated. Instead, use fs.s3a.server-side-encryption.key
          2017-08-11 17:56:50,651 INFO s3guard.S3GuardTool: Metadata store DynamoDBMetadataStore{region=eu-west-1, tableName=hwdev-steve-ireland-new} is initialized.
          Inserted 3 items into Metadata Store
          
          Show
          stevel@apache.org Steve Loughran added a comment - I know the answer to that now. I also see that you can configure DDB with an explicit TTL. If we added a field for each entry as to when the record itself was created, then we could have AWS TTL do the pruning automatically. Also, on the CLI I'm getting told off for an encryption key I don' think I've set ./hadoop s3guard import s3a: //hwdev-steve-ireland- new 2017-08-11 17:56:50,139 INFO Configuration.deprecation: fs.s3a.server-side-encryption-key is deprecated. Instead, use fs.s3a.server-side-encryption.key 2017-08-11 17:56:50,651 INFO s3guard.S3GuardTool: Metadata store DynamoDBMetadataStore{region=eu-west-1, tableName=hwdev-steve-ireland- new } is initialized. Inserted 3 items into Metadata Store
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Patch 005
          Review tune CLI docs & behaviour

          • update CLI docs with examples, clarification
          • DynamoDB init() to raise FNFE if the table isn't there...this is something which the CLIs can use and handle specifically. e.g delete() should downgrade to a no-op. Which it does
          • test for destroy-no-table being no-op
          • tune S3GuardTool tests (inc renaming baae suite Abstract)
          Show
          stevel@apache.org Steve Loughran added a comment - Patch 005 Review tune CLI docs & behaviour update CLI docs with examples, clarification DynamoDB init() to raise FNFE if the table isn't there...this is something which the CLIs can use and handle specifically. e.g delete() should downgrade to a no-op. Which it does test for destroy-no-table being no-op tune S3GuardTool tests (inc renaming baae suite Abstract)
          Hide
          stevel@apache.org Steve Loughran added a comment -

          test status: all done (parallel, threads=12, s3guard, local, auth) in 4:35!!!

          Show
          stevel@apache.org Steve Loughran added a comment - test status: all done (parallel, threads=12, s3guard, local, auth) in 4:35!!!
          Hide
          fabbri Aaron Fabbri added a comment -

          If we added a field for each entry as to when the record itself was created, then we could have AWS TTL do the pruning automatically.

          I think we will want a "entry last written" mod time field in DDB, but I don't think we can use S3's TTL feature without breaking the "all ancestors of any path P in DDB must be present" invariant. I chatted with my friend that works on the DynamoDB team and he did not believe that their TTL deletion feature was strongly ordered enough to guarantee it, even if we could ensure we always wrote ancestors before children. Maybe there is another algorithm I'm not thinking of though.

          I do think we want a v2 prune implementation for dynamo which works better (i.e. actually expires directories properly). I think that the authoritative mode support for dynamodb will be a big motivator for this, as if you are relying on DDB as source of truth for listings, then reliable expiry of stale data becomes more important. I've also been thinking about the online algorithm variant of prune (doing it on demand in client, probabilistically / randomized perhaps, or on access).

          Show
          fabbri Aaron Fabbri added a comment - If we added a field for each entry as to when the record itself was created, then we could have AWS TTL do the pruning automatically. I think we will want a "entry last written" mod time field in DDB, but I don't think we can use S3's TTL feature without breaking the "all ancestors of any path P in DDB must be present" invariant. I chatted with my friend that works on the DynamoDB team and he did not believe that their TTL deletion feature was strongly ordered enough to guarantee it, even if we could ensure we always wrote ancestors before children. Maybe there is another algorithm I'm not thinking of though. I do think we want a v2 prune implementation for dynamo which works better (i.e. actually expires directories properly). I think that the authoritative mode support for dynamodb will be a big motivator for this, as if you are relying on DDB as source of truth for listings, then reliable expiry of stale data becomes more important. I've also been thinking about the online algorithm variant of prune (doing it on demand in client, probabilistically / randomized perhaps, or on access).
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 16s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 17 new or modified test files.
                HADOOP-13345 Compile Tests
          +1 mvninstall 14m 46s HADOOP-13345 passed
          +1 compile 0m 23s HADOOP-13345 passed
          +1 checkstyle 0m 16s HADOOP-13345 passed
          +1 mvnsite 0m 25s HADOOP-13345 passed
          +1 findbugs 0m 31s HADOOP-13345 passed
          +1 javadoc 0m 16s HADOOP-13345 passed
                Patch Compile Tests
          +1 mvninstall 0m 22s the patch passed
          +1 compile 0m 20s the patch passed
          +1 javac 0m 20s the patch passed
          -0 checkstyle 0m 13s hadoop-tools/hadoop-aws: The patch generated 25 new + 54 unchanged - 5 fixed = 79 total (was 59)
          +1 mvnsite 0m 22s the patch passed
          -1 whitespace 0m 0s The patch has 7 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 0m 38s the patch passed
          +1 javadoc 0m 13s the patch passed
                Other Tests
          +1 unit 0m 38s hadoop-aws in the patch passed.
          +1 asflicense 0m 14s The patch does not generate ASF License warnings.
          21m 12s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14749
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881513/HADOOP-14749-HADOOP-13345-005.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle
          uname Linux 38f6cb70d3a6 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision HADOOP-13345 / b114f24
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/13009/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-aws.txt
          whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/13009/artifact/patchprocess/whitespace-eol.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13009/testReport/
          modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13009/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 17 new or modified test files.       HADOOP-13345 Compile Tests +1 mvninstall 14m 46s HADOOP-13345 passed +1 compile 0m 23s HADOOP-13345 passed +1 checkstyle 0m 16s HADOOP-13345 passed +1 mvnsite 0m 25s HADOOP-13345 passed +1 findbugs 0m 31s HADOOP-13345 passed +1 javadoc 0m 16s HADOOP-13345 passed       Patch Compile Tests +1 mvninstall 0m 22s the patch passed +1 compile 0m 20s the patch passed +1 javac 0m 20s the patch passed -0 checkstyle 0m 13s hadoop-tools/hadoop-aws: The patch generated 25 new + 54 unchanged - 5 fixed = 79 total (was 59) +1 mvnsite 0m 22s the patch passed -1 whitespace 0m 0s The patch has 7 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 0m 38s the patch passed +1 javadoc 0m 13s the patch passed       Other Tests +1 unit 0m 38s hadoop-aws in the patch passed. +1 asflicense 0m 14s The patch does not generate ASF License warnings. 21m 12s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14749 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881513/HADOOP-14749-HADOOP-13345-005.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle uname Linux 38f6cb70d3a6 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision HADOOP-13345 / b114f24 Default Java 1.8.0_144 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/13009/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-aws.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/13009/artifact/patchprocess/whitespace-eol.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13009/testReport/ modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13009/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          fabbri Aaron Fabbri added a comment -

          Quick review of the v5 patch:

          +### Security
          +
          +All users of the DynamoDB table must have write access to it. This
          +effectively means they must have write access to the entire object store.
          +
          

          Couldn't you have an IAM role with write access to DynamoDB but only read access for the bucket?

            
           **Warning do not enable any type of failure injection in production.  The
          -following settings are for test development only.**
          -
          -## Inconsistency Injection
          

          No change needed to your patch here.. but my hope for the sub heading was that we will introduce other types of failure injection in the future. We can re-add the main heading when we do.

          -    // TODO
          -    // 1. Add properties query to MetadataStore interface
          -    // supportsAuthoritativeDirectories() or something.
          -    // 2. Add "isNew" flag to MetadataStore.put(DirListingMetadata)
          -    // 3. If #1 is true, assert that directory is still fully cached here.
          -    // assertTrue("Created dir is fully cached", dirMeta.isAuthoritative());
          -
          +    // TODO HADOOP-1475 instrument MetadataStore for asserting & testing
          

          Wrong JIRA # here. Should be HADOOP-14756

          I tweaked that JIRA a bit to capture my original intent here (things have changed a little).

          So +1 after you fix the JIRA #, and consider clarifying bit about requiring write access to buckets.

          Show
          fabbri Aaron Fabbri added a comment - Quick review of the v5 patch: +### Security + +All users of the DynamoDB table must have write access to it. This +effectively means they must have write access to the entire object store. + Couldn't you have an IAM role with write access to DynamoDB but only read access for the bucket? **Warning do not enable any type of failure injection in production. The -following settings are for test development only.** - -## Inconsistency Injection No change needed to your patch here.. but my hope for the sub heading was that we will introduce other types of failure injection in the future. We can re-add the main heading when we do. - // TODO - // 1. Add properties query to MetadataStore interface - // supportsAuthoritativeDirectories() or something. - // 2. Add "isNew" flag to MetadataStore.put(DirListingMetadata) - // 3. If #1 is true, assert that directory is still fully cached here. - // assertTrue("Created dir is fully cached", dirMeta.isAuthoritative()); - + // TODO HADOOP-1475 instrument MetadataStore for asserting & testing Wrong JIRA # here. Should be HADOOP-14756 I tweaked that JIRA a bit to capture my original intent here (things have changed a little). So +1 after you fix the JIRA #, and consider clarifying bit about requiring write access to buckets.
          Hide
          stevel@apache.org Steve Loughran added a comment -
          1. We haven't any tests of IAM role with r/o bucket and r/w of DDB. I did accidentally try to use s3guard and landsat, and don't remember it being a particularly good experience. That's really something we need to test before making any statement about its use. Hence my caution. I will state "this has been undertested and may not work"
          1. I already have throttling in the fault injection in HADOOP-13786., here. Shows how we don't handle it at all well today. It's not an optimistic promise then, more a "forward looking on a future merge of existing code" statement. The hard part isn't this injection, it's reworking every since direct call of the AWS SDK (other than via xfer manager) to handle the error. Hence S3ALambda, which I use for all the committer and upload calls in WriteOperationHelper. We will need to go through every single API GET/HEAD call the same way for resilience.
          Show
          stevel@apache.org Steve Loughran added a comment - We haven't any tests of IAM role with r/o bucket and r/w of DDB. I did accidentally try to use s3guard and landsat, and don't remember it being a particularly good experience. That's really something we need to test before making any statement about its use. Hence my caution. I will state "this has been undertested and may not work" I already have throttling in the fault injection in HADOOP-13786 ., here . Shows how we don't handle it at all well today. It's not an optimistic promise then, more a "forward looking on a future merge of existing code" statement. The hard part isn't this injection, it's reworking every since direct call of the AWS SDK (other than via xfer manager) to handle the error. Hence S3ALambda , which I use for all the committer and upload calls in WriteOperationHelper . We will need to go through every single API GET/HEAD call the same way for resilience.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Note: the throttle events are coming from the failing client. You won't need to bind to a different client for different failure modes, just different options set

          We could rename it now to be the FailingAWSClient and make it a bit more generic,

          Show
          stevel@apache.org Steve Loughran added a comment - Note: the throttle events are coming from the failing client. You won't need to bind to a different client for different failure modes, just different options set We could rename it now to be the FailingAWSClient and make it a bit more generic,
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Patch 006: Aaron's comments. reinstated subheading on Inconsistent Client & reviewed all that text, esp. `` wrapping code snippets. Clarified read only bucket use "test this more". Fixed JIRA reference in TODO field

          This is what I'm about to apply

          Show
          stevel@apache.org Steve Loughran added a comment - Patch 006: Aaron's comments. reinstated subheading on Inconsistent Client & reviewed all that text, esp. `` wrapping code snippets. Clarified read only bucket use "test this more". Fixed JIRA reference in TODO field This is what I'm about to apply
          Hide
          stevel@apache.org Steve Loughran added a comment -

          patch 007 changes to s3guard tool & tests had generated lots of checkstyles. fixed

          test: s3 ireland + -Ds3guard -Ddynamodblocal -Dauth

          Show
          stevel@apache.org Steve Loughran added a comment - patch 007 changes to s3guard tool & tests had generated lots of checkstyles. fixed test: s3 ireland + -Ds3guard -Ddynamodblocal -Dauth
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 17 new or modified test files.
                HADOOP-13345 Compile Tests
          +1 mvninstall 18m 17s HADOOP-13345 passed
          +1 compile 0m 21s HADOOP-13345 passed
          +1 checkstyle 0m 16s HADOOP-13345 passed
          +1 mvnsite 0m 25s HADOOP-13345 passed
          +1 findbugs 0m 33s HADOOP-13345 passed
          +1 javadoc 0m 17s HADOOP-13345 passed
                Patch Compile Tests
          +1 mvninstall 0m 20s the patch passed
          +1 compile 0m 19s the patch passed
          +1 javac 0m 19s the patch passed
          -0 checkstyle 0m 13s hadoop-tools/hadoop-aws: The patch generated 2 new + 54 unchanged - 5 fixed = 56 total (was 59)
          +1 mvnsite 0m 21s the patch passed
          -1 whitespace 0m 0s The patch has 8 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
          +1 xml 0m 2s The patch has no ill-formed XML file.
          +1 findbugs 0m 41s the patch passed
          +1 javadoc 0m 14s the patch passed
                Other Tests
          +1 unit 0m 44s hadoop-aws in the patch passed.
          +1 asflicense 0m 17s The patch does not generate ASF License warnings.
          24m 58s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14749
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881616/HADOOP-14749-HADOOP-13345-007.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle
          uname Linux 93db725ee00d 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision HADOOP-13345 / b114f24
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/13019/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-aws.txt
          whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/13019/artifact/patchprocess/whitespace-eol.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13019/testReport/
          modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13019/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 17 new or modified test files.       HADOOP-13345 Compile Tests +1 mvninstall 18m 17s HADOOP-13345 passed +1 compile 0m 21s HADOOP-13345 passed +1 checkstyle 0m 16s HADOOP-13345 passed +1 mvnsite 0m 25s HADOOP-13345 passed +1 findbugs 0m 33s HADOOP-13345 passed +1 javadoc 0m 17s HADOOP-13345 passed       Patch Compile Tests +1 mvninstall 0m 20s the patch passed +1 compile 0m 19s the patch passed +1 javac 0m 19s the patch passed -0 checkstyle 0m 13s hadoop-tools/hadoop-aws: The patch generated 2 new + 54 unchanged - 5 fixed = 56 total (was 59) +1 mvnsite 0m 21s the patch passed -1 whitespace 0m 0s The patch has 8 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply +1 xml 0m 2s The patch has no ill-formed XML file. +1 findbugs 0m 41s the patch passed +1 javadoc 0m 14s the patch passed       Other Tests +1 unit 0m 44s hadoop-aws in the patch passed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 24m 58s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14749 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881616/HADOOP-14749-HADOOP-13345-007.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle uname Linux 93db725ee00d 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision HADOOP-13345 / b114f24 Default Java 1.8.0_144 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/13019/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-aws.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/13019/artifact/patchprocess/whitespace-eol.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13019/testReport/ modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13019/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          patch 008 checkstyle was complaining about the indentation of a modified line...I think its actually the entire block which needs its indentation tuned.

          Show
          stevel@apache.org Steve Loughran added a comment - patch 008 checkstyle was complaining about the indentation of a modified line...I think its actually the entire block which needs its indentation tuned.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 17 new or modified test files.
                HADOOP-13345 Compile Tests
          +1 mvninstall 15m 13s HADOOP-13345 passed
          +1 compile 0m 21s HADOOP-13345 passed
          +1 checkstyle 0m 17s HADOOP-13345 passed
          +1 mvnsite 0m 25s HADOOP-13345 passed
          +1 findbugs 0m 33s HADOOP-13345 passed
          +1 javadoc 0m 17s HADOOP-13345 passed
                Patch Compile Tests
          +1 mvninstall 0m 20s the patch passed
          +1 compile 0m 20s the patch passed
          +1 javac 0m 20s the patch passed
          -0 checkstyle 0m 14s hadoop-tools/hadoop-aws: The patch generated 1 new + 43 unchanged - 16 fixed = 44 total (was 59)
          +1 mvnsite 0m 24s the patch passed
          -1 whitespace 0m 0s The patch has 8 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 0m 34s the patch passed
          +1 javadoc 0m 12s the patch passed
                Other Tests
          +1 unit 0m 37s hadoop-aws in the patch passed.
          +1 asflicense 0m 13s The patch does not generate ASF License warnings.
          21m 43s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14749
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881621/HADOOP-14749-HADOOP-13345-008.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle
          uname Linux 189b4e1fb9be 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision HADOOP-13345 / b114f24
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/13021/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-aws.txt
          whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/13021/artifact/patchprocess/whitespace-eol.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13021/testReport/
          modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13021/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 17 new or modified test files.       HADOOP-13345 Compile Tests +1 mvninstall 15m 13s HADOOP-13345 passed +1 compile 0m 21s HADOOP-13345 passed +1 checkstyle 0m 17s HADOOP-13345 passed +1 mvnsite 0m 25s HADOOP-13345 passed +1 findbugs 0m 33s HADOOP-13345 passed +1 javadoc 0m 17s HADOOP-13345 passed       Patch Compile Tests +1 mvninstall 0m 20s the patch passed +1 compile 0m 20s the patch passed +1 javac 0m 20s the patch passed -0 checkstyle 0m 14s hadoop-tools/hadoop-aws: The patch generated 1 new + 43 unchanged - 16 fixed = 44 total (was 59) +1 mvnsite 0m 24s the patch passed -1 whitespace 0m 0s The patch has 8 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 0m 34s the patch passed +1 javadoc 0m 12s the patch passed       Other Tests +1 unit 0m 37s hadoop-aws in the patch passed. +1 asflicense 0m 13s The patch does not generate ASF License warnings. 21m 43s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14749 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881621/HADOOP-14749-HADOOP-13345-008.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle uname Linux 189b4e1fb9be 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision HADOOP-13345 / b114f24 Default Java 1.8.0_144 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/13021/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-aws.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/13021/artifact/patchprocess/whitespace-eol.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13021/testReport/ modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13021/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          committed patch 008. Thanks for all the reviews!

          Show
          stevel@apache.org Steve Loughran added a comment - committed patch 008. Thanks for all the reviews!
          Hide
          fabbri Aaron Fabbri added a comment -

          Awesome, thanks for doing this Steve Loughran

          Show
          fabbri Aaron Fabbri added a comment - Awesome, thanks for doing this Steve Loughran

            People

            • Assignee:
              stevel@apache.org Steve Loughran
              Reporter:
              stevel@apache.org Steve Loughran
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 24h
                24h
                Remaining:
                Remaining Estimate - 24h
                24h
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development