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

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241290215
 
    --- 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 --
   
    Oh thats a rebase mistake..I think need to rebase again.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241625448
 
    --- 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 --
   
    ok, please rebase asap


---

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



---

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



---

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



---

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



---

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



---

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



---

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



---

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



---

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



---

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241939242
 
    --- 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 --
   
    Resolved.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241976975
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/filesystem/AbstractDFSCarbonFile.java ---
    @@ -550,12 +550,10 @@ public DataOutputStream getDataOutputStreamUsingAppend(String path, FileFactory.
         if (null != fileStatus && fileStatus.isDirectory()) {
    --- End diff --
   
    Now the `pathFilter` is not used here? it may impact the callers of this method.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241977147
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/filesystem/AlluxioCarbonFile.java ---
    @@ -94,14 +93,9 @@ public CarbonFile getParentFile() {
       public boolean renameForce(String changeToName) {
         FileSystem fs;
         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);
    -        return true;
    -      } else {
    -        return false;
    -      }
    +      fs = fileStatus.getPath().getFileSystem(hadoopConf);
    +      fs.delete(new Path(changeToName), true);
    +      return fs.rename(fileStatus.getPath(), new Path(changeToName));
    --- End diff --
   
    It's a risky operation if 98 is failed then no way to get the file which is deleted in 97 line.  I think we should find a way to do it in single transaction.  Any better way @jackylk ?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241977202
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/impl/FileFactory.java ---
    @@ -43,7 +44,7 @@
        * LOGGER
        */
       private static final Logger LOGGER =
    -      LogServiceFactory.getLogService(FileFactory.class.getName());
    +          LogServiceFactory.getLogService(FileFactory.class.getName());
    --- End diff --
   
    There are lots of unnecessary format changes, please remove those changes and keep only the changes required for this PR


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241977467
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/locks/AlluxioFileLock.java ---
    @@ -0,0 +1,112 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.carbondata.core.locks;
    +
    +import java.io.DataOutputStream;
    +import java.io.IOException;
    +
    +import org.apache.carbondata.common.logging.LogServiceFactory;
    +import org.apache.carbondata.core.datastore.filesystem.AlluxioCarbonFile;
    +import org.apache.carbondata.core.datastore.impl.FileFactory;
    +import org.apache.carbondata.core.metadata.AbsoluteTableIdentifier;
    +import org.apache.carbondata.core.util.path.CarbonTablePath;
    +
    +import org.apache.log4j.Logger;
    +
    +/**
    + * This class is used to handle the S3 File locking.
    + * This is acheived using the concept of acquiring the data out stream using Append option.
    + */
    +public class AlluxioFileLock extends AbstractCarbonLock {
    --- End diff --
   
    why not use `HdfsFileLock` directly?, this class looks exactly same as `HdfsFileLock`


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241980353
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/filesystem/AbstractDFSCarbonFile.java ---
    @@ -550,12 +550,10 @@ public DataOutputStream getDataOutputStreamUsingAppend(String path, FileFactory.
         if (null != fileStatus && fileStatus.isDirectory()) {
    --- End diff --
   
    Missed in rebase.. Resolving this now.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241980383
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/filesystem/AlluxioCarbonFile.java ---
    @@ -94,14 +93,9 @@ public CarbonFile getParentFile() {
       public boolean renameForce(String changeToName) {
         FileSystem fs;
         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);
    -        return true;
    -      } else {
    -        return false;
    -      }
    +      fs = fileStatus.getPath().getFileSystem(hadoopConf);
    +      fs.delete(new Path(changeToName), true);
    +      return fs.rename(fileStatus.getPath(), new Path(changeToName));
    --- End diff --
   
    I am removing the delete..as kindle refer to the rename operation in Alluxio..it conflicts the way delete is present..which is handled in the alluxio version from 1.7.1


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2161#discussion_r241980433
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/impl/FileFactory.java ---
    @@ -43,7 +44,7 @@
        * LOGGER
        */
       private static final Logger LOGGER =
    -      LogServiceFactory.getLogService(FileFactory.class.getName());
    +          LogServiceFactory.getLogService(FileFactory.class.getName());
    --- End diff --
   
    Removing the unnecessary formats..thanks for bringing up this.


---