[GitHub] carbondata pull request #2161: Adding Alluxio Supporty

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2112/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    Build Failed  with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10156/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1901/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1903/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2113/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10157/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r243732085
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -960,7 +960,7 @@ private CarbonCommonConstants() {
        * If set to GLOBAL_SORT, the sorting scope is bigger and one index tree per task will be
        * created, thus loading is slower but query is faster.
        */
    -  public static final String LOAD_SORT_SCOPE_DEFAULT = "LOCAL_SORT";
    +  public static final String LOAD_SORT_SCOPE_DEFAULT = "NO_SORT";
    --- End diff --
   
    why should change this default value?
   
    Some more advices:
    1. blank changes in this file is unnecessary.
    2. too many commits in this PR, better to merge them into if history does not matter. refer to command `git rebase` to merge the commits into one
   
    Hope this PR can be merged soon


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1912/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2123/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10166/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r243773875
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -960,7 +960,7 @@ private CarbonCommonConstants() {
        * If set to GLOBAL_SORT, the sorting scope is bigger and one index tree per task will be
        * created, thus loading is slower but query is faster.
        */
    -  public static final String LOAD_SORT_SCOPE_DEFAULT = "LOCAL_SORT";
    +  public static final String LOAD_SORT_SCOPE_DEFAULT = "NO_SORT";
    --- End diff --
   
    resolved


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1916/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2127/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10170/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r243806487
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -960,7 +960,7 @@ private CarbonCommonConstants() {
        * If set to GLOBAL_SORT, the sorting scope is bigger and one index tree per task will be
        * created, thus loading is slower but query is faster.
        */
    -  public static final String LOAD_SORT_SCOPE_DEFAULT = "LOCAL_SORT";
    +  public static final String LOAD_SORT_SCOPE_DEFAULT = "NO_SORT";
    --- End diff --
   
    Actually thats the change coming from Master branch..when I rebased it. Please confirm..and I have squashed into one pull request.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r243819959
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -960,7 +960,7 @@ private CarbonCommonConstants() {
        * If set to GLOBAL_SORT, the sorting scope is bigger and one index tree per task will be
        * created, thus loading is slower but query is faster.
        */
    -  public static final String LOAD_SORT_SCOPE_DEFAULT = "LOCAL_SORT";
    +  public static final String LOAD_SORT_SCOPE_DEFAULT = "NO_SORT";
    --- End diff --
   
    OK


---

[qiuchenjian-2]

Github user xubo245 commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    @jackylk @ravipesala Please review it again.


---

[qiuchenjian-2]

Github user chandrasaripaka commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    @ravipesala , @jackylk can we merge the PR?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r244004960
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/filesystem/AlluxioCarbonFile.java ---
    @@ -90,21 +97,82 @@ public CarbonFile getParentFile() {
         return null == parent ? null : new AlluxioCarbonFile(parent);
       }
     
    +  /**
    +   * <p>RenameForce of the fileName for the AlluxioFileSystem Implementation.
    +   * Involves by opening a {@link FSDataInputStream} from the existing path and copy
    +   * bytes to {@link FSDataOutputStream}.
    +   * </p>
    +   * <p>Close the output and input streams only after the files have been written
    +   * Also check for the existence of the changed path and then delete the previous Path.
    +   * The No of Bytes that can be read is controlled by {@literal io.file.buffer.size},
    +   * where the default value is 4096.</p>
    +   * @param changeToName
    +   * @return
    +   */
       @Override
       public boolean renameForce(String changeToName) {
    -    FileSystem fs;
    +    FileSystem fs = null;
    +    FSDataOutputStream fsdos = null;
    +    FSDataInputStream fsdis = null;
         try {
    -      fs = fileStatus.getPath().getFileSystem(FileFactory.getConfiguration());
    -      if (fs instanceof DistributedFileSystem) {
    -        ((DistributedFileSystem) fs).rename(fileStatus.getPath(), new Path(changeToName),
    -            org.apache.hadoop.fs.Options.Rename.OVERWRITE);
    +      Path actualPath = fileStatus.getPath();
    +      Path changedPath = new Path(changeToName);
    +      fs = actualPath.getFileSystem(hadoopConf);
    +      fsdos = fs.create(changedPath, true);
    +      fsdis = fs.open(actualPath);
    +      if (null != fsdis && null != fsdos) {
    +        IOUtils.copyBytes(fsdis, fsdos, hadoopConf, true);
             return true;
    --- End diff --
   
    I think what you do in the finally block should move to here and fileStatus should be assigned to point to the newly created file.



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r244005006
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/filesystem/AlluxioCarbonFile.java ---
    @@ -90,21 +97,82 @@ public CarbonFile getParentFile() {
         return null == parent ? null : new AlluxioCarbonFile(parent);
       }
     
    +  /**
    +   * <p>RenameForce of the fileName for the AlluxioFileSystem Implementation.
    +   * Involves by opening a {@link FSDataInputStream} from the existing path and copy
    +   * bytes to {@link FSDataOutputStream}.
    +   * </p>
    +   * <p>Close the output and input streams only after the files have been written
    +   * Also check for the existence of the changed path and then delete the previous Path.
    +   * The No of Bytes that can be read is controlled by {@literal io.file.buffer.size},
    +   * where the default value is 4096.</p>
    +   * @param changeToName
    +   * @return
    +   */
       @Override
       public boolean renameForce(String changeToName) {
    -    FileSystem fs;
    +    FileSystem fs = null;
    +    FSDataOutputStream fsdos = null;
    +    FSDataInputStream fsdis = null;
         try {
    -      fs = fileStatus.getPath().getFileSystem(FileFactory.getConfiguration());
    -      if (fs instanceof DistributedFileSystem) {
    -        ((DistributedFileSystem) fs).rename(fileStatus.getPath(), new Path(changeToName),
    -            org.apache.hadoop.fs.Options.Rename.OVERWRITE);
    +      Path actualPath = fileStatus.getPath();
    +      Path changedPath = new Path(changeToName);
    +      fs = actualPath.getFileSystem(hadoopConf);
    +      fsdos = fs.create(changedPath, true);
    +      fsdis = fs.open(actualPath);
    +      if (null != fsdis && null != fsdos) {
    +        IOUtils.copyBytes(fsdis, fsdos, hadoopConf, true);
             return true;
    -      } else {
    -        return false;
           }
    +      return false;
         } catch (IOException e) {
           LOGGER.error("Exception occured: " + e.getMessage());
           return false;
    +    } finally {
    +      try {
    +        if (null != fsdis && null != fsdos) {
    +          if (fs.exists(new Path(changeToName))) {
    +            fs.delete(fileStatus.getPath(), true);
    --- End diff --
   
    Here the fileStatus is not updated, I suspect there will be potential issue


---