[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/1832/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user xubo245 commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    @chandrasaripaka Can you fix the CI error?


---

[qiuchenjian-2]

Github user chandrasaripaka commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    > @chandrasaripaka Can you fix the CI error?
   
    @xubo245 , just committed please review and let me know.


---

[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/1710/



---

[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/9970/



---

[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/1921/



---

[qiuchenjian-2]

Github user chandrasaripaka commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    @xubo245 , please merge the PR, after a review


---

[qiuchenjian-2]

Github user xubo245 commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    @chandrasaripaka OK. I will review this PR for detail this week, and plan to merge it recently.


---

[qiuchenjian-2]

Github user xubo245 commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    @chandrasaripaka  I confirmed and reproduced this issue based on carbonData lastest master and spark-2.3.2.


---

[qiuchenjian-2]

Github user chandrasaripaka commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    @xubo245 , so may I know if the PR can be merged  or it needs some rework..we can attach some test logs.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241259379
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/impl/FileFactory.java ---
    @@ -111,14 +112,9 @@ public static DataInputStream getDataInputStream(String path, FileType fileType)
         return getDataInputStream(path, fileType, -1);
       }
     
    -  public static DataInputStream getDataInputStream(String path, FileType fileType,
    --- End diff --
   
    Why do you remove this interface?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241264313
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/impl/FileFactory.java ---
    @@ -432,7 +437,7 @@ public static long getDirectorySize(String filePath) throws IOException {
           case VIEWFS:
           case S3:
             Path path = new Path(filePath);
    -        FileSystem fs = path.getFileSystem(getConfiguration());
    +        FileSystem fs = path.getFileSystem(configuration);
    --- End diff --
   
    Why do you need change this?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241264360
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/impl/FileFactory.java ---
    @@ -470,10 +475,11 @@ public static void createDirectoryAndSetPermission(String directoryPath, FsPermi
         switch (fileType) {
           case S3:
           case HDFS:
    +      case ALLUXIO:
           case VIEWFS:
             try {
               Path path = new Path(directoryPath);
    -          FileSystem fs = path.getFileSystem(getConfiguration());
    +          FileSystem fs = path.getFileSystem(FileFactory.configuration);
    --- End diff --
   
    Why do you need change this?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241264666
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/locks/CarbonLockFactory.java ---
    @@ -99,11 +103,17 @@ public static ICarbonLock getSystemLevelCarbonLockObj(String locFileLocation, St
           case CarbonCommonConstants.CARBON_LOCK_TYPE_LOCAL:
             return new LocalFileLock(lockFileLocation, lockFile);
           case CarbonCommonConstants.CARBON_LOCK_TYPE_ZOOKEEPER:
    -        return new ZooKeeperLocking(lockFileLocation, lockFile);
    +        return new ZooKeeperLocking(locFileLocation, lockFile);
    +
    --- End diff --
   
    no need to add empty Line in here


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241264683
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/locks/CarbonLockFactory.java ---
    @@ -99,11 +103,17 @@ public static ICarbonLock getSystemLevelCarbonLockObj(String locFileLocation, St
           case CarbonCommonConstants.CARBON_LOCK_TYPE_LOCAL:
             return new LocalFileLock(lockFileLocation, lockFile);
           case CarbonCommonConstants.CARBON_LOCK_TYPE_ZOOKEEPER:
    -        return new ZooKeeperLocking(lockFileLocation, lockFile);
    +        return new ZooKeeperLocking(locFileLocation, lockFile);
    +
           case CarbonCommonConstants.CARBON_LOCK_TYPE_HDFS:
    -        return new HdfsFileLock(lockFileLocation, lockFile);
    +        return new HdfsFileLock(locFileLocation, lockFile);
    +
    --- End diff --
   
    no need to add empty Line in here


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241264716
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/locks/CarbonLockFactory.java ---
    @@ -99,11 +103,17 @@ public static ICarbonLock getSystemLevelCarbonLockObj(String locFileLocation, St
           case CarbonCommonConstants.CARBON_LOCK_TYPE_LOCAL:
             return new LocalFileLock(lockFileLocation, lockFile);
           case CarbonCommonConstants.CARBON_LOCK_TYPE_ZOOKEEPER:
    -        return new ZooKeeperLocking(lockFileLocation, lockFile);
    +        return new ZooKeeperLocking(locFileLocation, lockFile);
    +
           case CarbonCommonConstants.CARBON_LOCK_TYPE_HDFS:
    -        return new HdfsFileLock(lockFileLocation, lockFile);
    +        return new HdfsFileLock(locFileLocation, lockFile);
    +
           case CarbonCommonConstants.CARBON_LOCK_TYPE_S3:
    -        return new S3FileLock(lockFileLocation, lockFile);
    +        return new S3FileLock(locFileLocation, lockFile);
    +
    --- End diff --
   
    no need to add empty Line in here, please check other places


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241264812
 
    --- Diff: core/src/test/java/org/apache/carbondata/core/datastore/filesystem/AlluxioCarbonFileTest.java ---
    @@ -20,20 +20,16 @@
     import mockit.Mock;
     import mockit.MockUp;
     import org.apache.hadoop.conf.Configuration;
    -import org.apache.hadoop.fs.FileStatus;
    -import org.apache.hadoop.fs.FileSystem;
    -import org.apache.hadoop.fs.Options;
    -import org.apache.hadoop.fs.Path;
    -import org.apache.hadoop.hdfs.DistributedFileSystem;
    -import org.apache.hadoop.hdfs.web.WebHdfsFileSystem;
    +import org.apache.hadoop.fs.*;
    --- End diff --
   
    Please keep import the detail class, it will import more class if change to *


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241264817
 
    --- Diff: core/src/test/java/org/apache/carbondata/core/datastore/filesystem/AlluxioCarbonFileTest.java ---
    @@ -20,20 +20,16 @@
     import mockit.Mock;
     import mockit.MockUp;
     import org.apache.hadoop.conf.Configuration;
    -import org.apache.hadoop.fs.FileStatus;
    -import org.apache.hadoop.fs.FileSystem;
    -import org.apache.hadoop.fs.Options;
    -import org.apache.hadoop.fs.Path;
    -import org.apache.hadoop.hdfs.DistributedFileSystem;
    -import org.apache.hadoop.hdfs.web.WebHdfsFileSystem;
    +import org.apache.hadoop.fs.*;
    +import org.apache.hadoop.fs.permission.FsPermission;
    +import org.apache.hadoop.util.Progressable;
     import org.junit.AfterClass;
     import org.junit.BeforeClass;
     import org.junit.Test;
     
    -import java.io.File;
    -import java.io.FileNotFoundException;
    -import java.io.FileOutputStream;
    -import java.io.IOException;
    +import java.io.*;
    --- End diff --
   
    Please keep import the detail class, it will import more class if change to *


---

[qiuchenjian-2]

Github user xubo245 commented on the issue:

    https://github.com/apache/carbondata/pull/2161
 
    @chandrasaripaka Please rebase carefully.


---