Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.7.2
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: fs/s3
    • Labels:
      None

      Description

      S3A.create(Path,FsPermission,boolean,int,short,long,Progressable) is not checking to see if it's being asked to overwrite a directory. It could easily do so, and should throw an error in this case.

      There is a test-case for this in AbstractFSContractTestBase, but it's being skipped because S3A is a blobstore. However, both the Azure and Swift file systems make this test, and the new S3 one should as well.

        Issue Links

          Activity

          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #10144 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10144/)
          HADOOP-13188 S3A file-create should throw error rather than overwrite (stevel: rev 86ae218893d018638e937c2528c8e84336254da7)

          • hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
          • hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/contract/s3a/TestS3AContractCreate.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #10144 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10144/ ) HADOOP-13188 S3A file-create should throw error rather than overwrite (stevel: rev 86ae218893d018638e937c2528c8e84336254da7) hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/contract/s3a/TestS3AContractCreate.java
          Hide
          stevel@apache.org Steve Loughran added a comment -

          forgot this had been +1'd. retested and committed

          Show
          stevel@apache.org Steve Loughran added a comment - forgot this had been +1'd. retested and committed
          Hide
          stevel@apache.org Steve Loughran added a comment -

          thanks, I 'll commit it over the weekend.

          if you get S3a.toString() on your FS instance, you'll get the full io stats printed out.Be interesting to see the changes.

          I don't think you should be seeing any perf diff. There's a call to exists(path), which is getFileStatus in a try/catch block. Same overhead, simply without check for the path referring to a directory if it is there

          Show
          stevel@apache.org Steve Loughran added a comment - thanks, I 'll commit it over the weekend. if you get S3a.toString() on your FS instance, you'll get the full io stats printed out.Be interesting to see the changes. I don't think you should be seeing any perf diff. There's a call to exists(path), which is getFileStatus in a try/catch block. Same overhead, simply without check for the path referring to a directory if it is there
          Hide
          raviprak Ravi Prakash added a comment - - edited

          Looks good to me. +1. FWIW, I was concerned about the performance penalty.
          So I uploaded 16 files (using hadoop fs -put <directory_toupload> s3a://<aws_access_key>:<aws_secret_key>@bucket/path). With the patch it took

          real	2m16.564s
          user	0m11.571s
          sys	0m0.582s
          

          Without the patch:

          real	2m5.481s
          user	0m10.811s
          sys	0m0.472s

          So its not that bad a degradation.. Still pretty bad performance though, but that is clearly another JIRA

          Show
          raviprak Ravi Prakash added a comment - - edited Looks good to me. +1. FWIW, I was concerned about the performance penalty. So I uploaded 16 files (using hadoop fs -put <directory_toupload> s3a://<aws_access_key>:<aws_secret_key>@bucket/path ). With the patch it took real 2m16.564s user 0m11.571s sys 0m0.582s Without the patch: real 2m5.481s user 0m10.811s sys 0m0.472s So its not that bad a degradation.. Still pretty bad performance though, but that is clearly another JIRA
          Hide
          stevel@apache.org Steve Loughran added a comment -

          HADOOP-13221 relates to this; a fix from that needs to merge in a check for the dest path being a directory too

          Show
          stevel@apache.org Steve Loughran added a comment - HADOOP-13221 relates to this; a fix from that needs to merge in a check for the dest path being a directory too
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 11m 11s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 6m 36s branch-2 passed
          +1 compile 0m 11s branch-2 passed with JDK v1.8.0_91
          +1 compile 0m 12s branch-2 passed with JDK v1.7.0_101
          +1 checkstyle 0m 14s branch-2 passed
          +1 mvnsite 0m 18s branch-2 passed
          +1 mvneclipse 0m 15s branch-2 passed
          +1 findbugs 0m 29s branch-2 passed
          +1 javadoc 0m 12s branch-2 passed with JDK v1.8.0_91
          +1 javadoc 0m 14s branch-2 passed with JDK v1.7.0_101
          +1 mvninstall 0m 13s the patch passed
          +1 compile 0m 9s the patch passed with JDK v1.8.0_91
          +1 javac 0m 9s the patch passed
          +1 compile 0m 11s the patch passed with JDK v1.7.0_101
          +1 javac 0m 11s the patch passed
          -1 checkstyle 0m 11s hadoop-tools/hadoop-aws: The patch generated 1 new + 14 unchanged - 0 fixed = 15 total (was 14)
          +1 mvnsite 0m 15s the patch passed
          +1 mvneclipse 0m 11s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 39s the patch passed
          +1 javadoc 0m 9s the patch passed with JDK v1.8.0_91
          +1 javadoc 0m 12s the patch passed with JDK v1.7.0_101
          +1 unit 0m 10s hadoop-aws in the patch passed with JDK v1.8.0_91.
          +1 unit 0m 10s hadoop-aws in the patch passed with JDK v1.7.0_101.
          +1 asflicense 0m 17s The patch does not generate ASF License warnings.
          23m 40s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:babe025
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12805883/HADOOP-13188-branch-2-001.patch
          JIRA Issue HADOOP-13188
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux b2968ed95eaf 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision branch-2 / ce886ff
          Default Java 1.7.0_101
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_91 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/9567/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-aws.txt
          JDK v1.7.0_101 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9567/testReport/
          modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9567/console
          Powered by Apache Yetus 0.4.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 11m 11s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 36s branch-2 passed +1 compile 0m 11s branch-2 passed with JDK v1.8.0_91 +1 compile 0m 12s branch-2 passed with JDK v1.7.0_101 +1 checkstyle 0m 14s branch-2 passed +1 mvnsite 0m 18s branch-2 passed +1 mvneclipse 0m 15s branch-2 passed +1 findbugs 0m 29s branch-2 passed +1 javadoc 0m 12s branch-2 passed with JDK v1.8.0_91 +1 javadoc 0m 14s branch-2 passed with JDK v1.7.0_101 +1 mvninstall 0m 13s the patch passed +1 compile 0m 9s the patch passed with JDK v1.8.0_91 +1 javac 0m 9s the patch passed +1 compile 0m 11s the patch passed with JDK v1.7.0_101 +1 javac 0m 11s the patch passed -1 checkstyle 0m 11s hadoop-tools/hadoop-aws: The patch generated 1 new + 14 unchanged - 0 fixed = 15 total (was 14) +1 mvnsite 0m 15s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 39s the patch passed +1 javadoc 0m 9s the patch passed with JDK v1.8.0_91 +1 javadoc 0m 12s the patch passed with JDK v1.7.0_101 +1 unit 0m 10s hadoop-aws in the patch passed with JDK v1.8.0_91. +1 unit 0m 10s hadoop-aws in the patch passed with JDK v1.7.0_101. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 23m 40s Subsystem Report/Notes Docker Image:yetus/hadoop:babe025 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12805883/HADOOP-13188-branch-2-001.patch JIRA Issue HADOOP-13188 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux b2968ed95eaf 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision branch-2 / ce886ff Default Java 1.7.0_101 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_91 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/9567/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-aws.txt JDK v1.7.0_101 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9567/testReport/ modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9567/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Patch 001

          1. removed the overidden test case which was skipping the contract test
          2. verified that the base test failed
          3. patched S3AFileSystem to check for dest being a dir
          4. verified that with that change, the test passes

          Full test run completed against S3 Ireland

          Tests run: 225, Failures: 0, Errors: 0, Skipped: 5
          
          [INFO] ------------------------------------------------------------------------
          [INFO] BUILD SUCCESS
          [INFO] ------------------------------------------------------------------------
          [INFO] Total time: 17:24 min
          [INFO] Finished at: 2016-05-24T15:02:47+01:00
          [INFO] Final Memory: 17M/284M
          [INFO] ------------------------------------------------------------------------
          
          Show
          stevel@apache.org Steve Loughran added a comment - Patch 001 removed the overidden test case which was skipping the contract test verified that the base test failed patched S3AFileSystem to check for dest being a dir verified that with that change, the test passes Full test run completed against S3 Ireland Tests run: 225, Failures: 0, Errors: 0, Skipped: 5 [INFO] ------------------------------------------------------------------------ [INFO] BUILD SUCCESS [INFO] ------------------------------------------------------------------------ [INFO] Total time: 17:24 min [INFO] Finished at: 2016-05-24T15:02:47+01:00 [INFO] Final Memory: 17M/284M [INFO] ------------------------------------------------------------------------
          Hide
          raymie Raymie Stata added a comment -

          Steve Loughran found the following test-cases being skipped that probably shouldn't be:

          2016-05-20 11:03:07,144 INFO  contract.AbstractFSContractTestBase (AbstractFSContractTestBase.java:describe(240)) - verify trying to create a file over a non-empty dir fails
          2016-05-20 11:03:10,615 INFO  contract.ContractTestUtils (ContractTestUtils.java:skip(425)) - Skipping: Object store allows a file to overwrite a directory
          
          2016-05-20 11:03:14,992 INFO  contract.AbstractFSContractTestBase (AbstractFSContractTestBase.java:setup(172)) - Test filesystem = s3a://hwdev-steve implemented by S3AFileSystem{uri=s3a://hwdev-steve, workingDir=s3a://hwdev-steve/user/stevel, partSize=104857600, enableMultiObjectsDelete=true, maxKeys=5000, readAhead=65536, blockSize=33554432, multiPartThreshold=2147483647, statistics {0 bytes read, 266 bytes written, 25 read ops, 0 large read ops, 11 write ops}, metrics {{Context=S3AFileSystem} {FileSystemId=0b9a969b-9c2e-48da-9bce-4599ae8a2cb9-hwdev-steve} {fsURI=s3a://hwdev-steve/} {files_created=3} {files_copied=0} {files_copied_bytes=0} {files_deleted=3} {directories_created=3} {directories_deleted=0} {ignored_errors=3} {streamReadOperations=0} {streamForwardSeekOperations=0} {streamBytesRead=0} {streamSeekOperations=0} {streamReadExceptions=0} {streamOpened=0} {streamReadOperationsIncomplete=0} {streamAborted=0} {streamReadFullyOperations=0} {streamClosed=0} {streamBytesSkippedOnSeek=0} {streamCloseOperations=0} {streamBytesBackwardsOnSeek=0} {streamBackwardSeekOperations=0} }}
          2016-05-20 11:03:16,381 INFO  contract.ContractTestUtils (ContractTestUtils.java:skip(425)) - Skipping: blobstores can't distinguish empty directories from files
          
          Show
          raymie Raymie Stata added a comment - Steve Loughran found the following test-cases being skipped that probably shouldn't be: 2016-05-20 11:03:07,144 INFO contract.AbstractFSContractTestBase (AbstractFSContractTestBase.java:describe(240)) - verify trying to create a file over a non-empty dir fails 2016-05-20 11:03:10,615 INFO contract.ContractTestUtils (ContractTestUtils.java:skip(425)) - Skipping: Object store allows a file to overwrite a directory 2016-05-20 11:03:14,992 INFO contract.AbstractFSContractTestBase (AbstractFSContractTestBase.java:setup(172)) - Test filesystem = s3a://hwdev-steve implemented by S3AFileSystem{uri=s3a://hwdev-steve, workingDir=s3a://hwdev-steve/user/stevel, partSize=104857600, enableMultiObjectsDelete=true, maxKeys=5000, readAhead=65536, blockSize=33554432, multiPartThreshold=2147483647, statistics {0 bytes read, 266 bytes written, 25 read ops, 0 large read ops, 11 write ops}, metrics {{Context=S3AFileSystem} {FileSystemId=0b9a969b-9c2e-48da-9bce-4599ae8a2cb9-hwdev-steve} {fsURI=s3a://hwdev-steve/} {files_created=3} {files_copied=0} {files_copied_bytes=0} {files_deleted=3} {directories_created=3} {directories_deleted=0} {ignored_errors=3} {streamReadOperations=0} {streamForwardSeekOperations=0} {streamBytesRead=0} {streamSeekOperations=0} {streamReadExceptions=0} {streamOpened=0} {streamReadOperationsIncomplete=0} {streamAborted=0} {streamReadFullyOperations=0} {streamClosed=0} {streamBytesSkippedOnSeek=0} {streamCloseOperations=0} {streamBytesBackwardsOnSeek=0} {streamBackwardSeekOperations=0} }} 2016-05-20 11:03:16,381 INFO contract.ContractTestUtils (ContractTestUtils.java:skip(425)) - Skipping: blobstores can't distinguish empty directories from files

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development