[GitHub] carbondata pull request #1584: [CARBONDATA-1827] Added S3 implementation and...

[qiuchenjian-2]

GitHub user SangeetaGulia opened a pull request:

    https://github.com/apache/carbondata/pull/1584

    [CARBONDATA-1827] Added S3 implementation and TestCases

    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     - [ ] Any interfaces changed?
     No
     - [ ] Any backward compatibility impacted?
     No
     - [ ] Document update required?
    No
     - [x] Testing done
            Please provide details on
            - Whether new unit test cases have been added or why no new tests are required?
    Added new Unit Test Cases.
   
            - How it is tested? Please attach test report.
            - Is it a performance related change? Please attach the performance test report.
            - Any additional information to help reviewers in testing this change.
           
     - [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
   


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/jatin9896/incubator-carbondata feature/s3Implementation

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/carbondata/pull/1584.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1584
   
----
commit bbc2dc340086a66bd9f30f0424a0fe5722ee4c57
Author: SangeetaGulia <[hidden email]>
Date:   2017-09-21T09:26:26Z

    Added S3 implementation and TestCases

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1584
 
    Can one of the admins verify this patch?


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1584
 
    Can one of the admins verify this patch?


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1584
 
    Can one of the admins verify this patch?


---

[qiuchenjian-2]

Github user mohammadshahidkhan commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1584#discussion_r153691909
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -162,6 +161,12 @@
     
       public static final String S3A_PREFIX = "s3a://";
     
    +
    +  /**
    +   * VIEWS3URL_PREFIX
    +   */
    +  public static final String S3URL_PREFIX = "s3a://";
    --- End diff --
   
    S3A_PREFIX  and S3URL_PREFIX both have same constant.
    Please remove once of them.


---

[qiuchenjian-2]

Github user mohammadshahidkhan commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1584#discussion_r153692400
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -1356,6 +1367,35 @@
     
       public static final String CARBON_UPDATE_SYNC_FOLDER_DEFAULT = "/tmp/carbondata";
     
    +  /**
    +   * S3 Constants
    +   */
    +
    +  public static final String S3_ACCESS_KEY = "fs.s3a.access.key";
    --- End diff --
   
    i think its better not to keep the s3 specific property is CarbonCommonConstant
    fs.s3a.access.key, fs.s3a.secret.key, fs.s3a.impl
    these constant already would have been defined in s3 .


---

[qiuchenjian-2]

Github user mohammadshahidkhan commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1584#discussion_r153692578
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/impl/FileFactory.java ---
    @@ -81,24 +77,22 @@ public static FileHolder getFileHolder(FileType fileType) {
           case HDFS:
           case ALLUXIO:
           case VIEWFS:
    -      case S3:
             return new DFSFileHolderImpl();
    +      case S3:
    +        return new S3FileHolderImpl();
           default:
             return new FileHolderImpl();
         }
       }
     
       public static FileType getFileType(String path) {
    -    String lowerPath = path.toLowerCase();
    --- End diff --
   
    Any reason why converting to lower case is removed.


---

[qiuchenjian-2]

Github user mohammadshahidkhan commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1584#discussion_r153692789
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/locks/CarbonLockFactory.java ---
    @@ -52,23 +52,23 @@
        */
       public static ICarbonLock getCarbonLockObj(AbsoluteTableIdentifier absoluteTableIdentifier,
           String lockFile) {
    -    switch (lockTypeConfigured) {
    -      case CarbonCommonConstants.CARBON_LOCK_TYPE_LOCAL:
    --- End diff --
   
    Why swith case change to if else


---

[qiuchenjian-2]

Github user SangeetaGulia commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1584#discussion_r153698343
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/locks/CarbonLockFactory.java ---
    @@ -52,23 +52,23 @@
        */
       public static ICarbonLock getCarbonLockObj(AbsoluteTableIdentifier absoluteTableIdentifier,
           String lockFile) {
    -    switch (lockTypeConfigured) {
    -      case CarbonCommonConstants.CARBON_LOCK_TYPE_LOCAL:
    --- End diff --
   
    This change is to support multiple filesystems on a single carbonsession. So at a time if you work on S3 and local together, it should implicitly understand what lock type it should provide.
   
    So we are identifying lock type based on the type of file and the conditional match falls on two variables:  one is lockTypeConfigured and second, we have to identify what type of file it is?(whether s3, hdfs, local) on the basis of which we will set lockTypeConfigured.
   
    Due to this change now we don't need to specify LOCK_TYPE in carbonProperties explicitly for s3, hdfs and local.


---

[qiuchenjian-2]

Github user SangeetaGulia commented on the issue:

    https://github.com/apache/carbondata/pull/1584
 
    retest this please


---

[qiuchenjian-2]

Github user kunal642 commented on the issue:

    https://github.com/apache/carbondata/pull/1584
 
    retest this please


---

[qiuchenjian-2]

Github user chenliang613 commented on the issue:

    https://github.com/apache/carbondata/pull/1584
 
    add to whitelist


---

[qiuchenjian-2]

Github user SangeetaGulia commented on the issue:

    https://github.com/apache/carbondata/pull/1584
 
    retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1584
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/1610/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1584
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/1613/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1584
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2004/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1584
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2007/



---

[qiuchenjian-2]

Github user SangeetaGulia commented on the issue:

    https://github.com/apache/carbondata/pull/1584
 
    @mohammadshahidkhan Can you please review, we have made the changes.


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1584#discussion_r155455752
 
    --- Diff: core/pom.xml ---
    @@ -94,6 +99,20 @@
             </exclusion>
           </exclusions>
         </dependency>
    +
    +    <!-- https://mvnrepository.com/artifact/org.apache.hadoop/hadoop-aws -->
    +    <dependency>
    +      <groupId>org.apache.hadoop</groupId>
    --- End diff --
   
    Why is this required? Core should not dependent on hadoop, right?


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1584
 
    Build Failed with Spark 2.2.0, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/952/



---