[GitHub] carbondata pull request #1177: [CARBONDATA-1281] Support multiple temp dirs ...

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1177
 
    Build Success with Spark 1.6, Please check CI http://144.76.159.231:8080/job/ApacheCarbonPRBuilder/566/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1177#discussion_r128666796
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/spark/util/SparkUtil4Test.scala ---
    @@ -0,0 +1,33 @@
    +/*
    + * 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.spark.util
    +
    +import org.apache.spark.SparkConf
    +
    +/**
    + * This class is for accessing utils in spark package for tests
    + */
    +object SparkUtil4Test {
    --- End diff --
   
    CANNOT.
   
    The class `SparkUtil4Test` is created to access the functions provided by  class `Utils` in Spark whose effective scope is package private.
   
    The new class `SparkUtil4Test` and test class are in different packages, so they cannot co-exist in one file.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1177
 
    Build Failed with Spark 1.6, Please check CI http://144.76.159.231:8080/job/ApacheCarbonPRBuilder/567/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1177
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/3161/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

    https://github.com/apache/carbondata/pull/1177
 
    the commits are updated
   
    retest this please


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1177
 
    Build Success with Spark 1.6, Please check CI http://144.76.159.231:8080/job/ApacheCarbonPRBuilder/568/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1177
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/3162/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1177
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/3163/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1177
 
    Build Success with Spark 1.6, Please check CI http://144.76.159.231:8080/job/ApacheCarbonPRBuilder/569/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1177
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/3164/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1177
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/3165/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1177
 
    Build Success with Spark 1.6, Please check CI http://144.76.159.231:8080/job/ApacheCarbonPRBuilder/570/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

    https://github.com/apache/carbondata/pull/1177
 
    Sorry to consume so many builds here. It seems the jenkins project is commit triggered, in company I can only edit/commit files online one by one instead of pusing to Github in batch directly.
   
    @chenliang613 please review again. I fixed all the comment except this: https://github.com/apache/carbondata/pull/1177#discussion_r128666796
   



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1177#discussion_r129061078
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/newflow/sort/unsafe/UnsafeSortDataRows.java ---
    @@ -306,7 +309,9 @@ private void writeData(UnsafeCarbonRowPage rowPage, File file)
        * This method will be used to delete sort temp location is it is exites
        */
       public void deleteSortLocationIfExists() {
    -    CarbonDataProcessorUtil.deleteSortLocationIfExists(parameters.getTempFileLocation());
    +    for (String loc : parameters.getTempFileLocation()) {
    --- End diff --
   
    could have changed the method signature of CarbonDataProcessorUtil.deleteSortLocationIfExists to take list


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1177#discussion_r129062284
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/newflow/steps/DataWriterBatchProcessorStepImpl.java ---
    @@ -58,12 +58,14 @@ public DataWriterBatchProcessorStepImpl(CarbonDataLoadConfiguration configuratio
         child.initialize();
       }
     
    -  private String getStoreLocation(CarbonTableIdentifier tableIdentifier, String partitionId) {
    -    String storeLocation = CarbonDataProcessorUtil
    +  private String[] getStoreLocation(CarbonTableIdentifier tableIdentifier, String partitionId) {
    +    String[] storeLocation = CarbonDataProcessorUtil
             .getLocalDataFolderLocation(tableIdentifier.getDatabaseName(),
                 tableIdentifier.getTableName(), String.valueOf(configuration.getTaskNo()), partitionId,
                 configuration.getSegmentId() + "", false);
    -    new File(storeLocation).mkdirs();
    +    for (String loc : storeLocation) {
    --- End diff --
   
    can create a utility method, Same operations repeating in multiple functions


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1177#discussion_r129057078
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/newflow/sort/impl/ParallelReadMergeSorterWithBucketingImpl.java ---
    @@ -185,12 +188,17 @@ private boolean processRowToNextStep(SortDataRows[] sortDataRows, SortParameters
       }
     
       private void setTempLocation(SortParameters parameters) {
    -    String carbonDataDirectoryPath = CarbonDataProcessorUtil
    +    String[] carbonDataDirectoryPath = CarbonDataProcessorUtil
             .getLocalDataFolderLocation(parameters.getDatabaseName(),
                 parameters.getTableName(), parameters.getTaskNo(),
                 parameters.getPartitionID(), parameters.getSegmentId(), false);
    -    parameters.setTempFileLocation(
    -        carbonDataDirectoryPath + File.separator + CarbonCommonConstants.SORT_TEMP_FILE_LOCATION);
    +    String[] tmpLocs = new String[carbonDataDirectoryPath.length];
    --- End diff --
   
    Can move to common utility function


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1177#discussion_r129057384
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/newflow/sort/impl/UnsafeParallelReadMergeSorterWithBucketingImpl.java ---
    @@ -168,11 +171,15 @@ private boolean processRowToNextStep(UnsafeSortDataRows[] sortDataRows, SortPara
       }
     
       private void setTempLocation(SortParameters parameters) {
    -    String carbonDataDirectoryPath = CarbonDataProcessorUtil
    +    String[] carbonDataDirectoryPath = CarbonDataProcessorUtil
             .getLocalDataFolderLocation(parameters.getDatabaseName(), parameters.getTableName(),
                 parameters.getTaskNo(), parameters.getPartitionID(), parameters.getSegmentId(), false);
    -    parameters.setTempFileLocation(
    -        carbonDataDirectoryPath + File.separator + CarbonCommonConstants.SORT_TEMP_FILE_LOCATION);
    +    String[] tmpLoc = new String[carbonDataDirectoryPath.length];
    --- End diff --
   
    can move to utility function


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1177#discussion_r129195768
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/merger/RowResultMergerProcessor.java ---
    @@ -57,12 +57,15 @@
           LogServiceFactory.getLogService(RowResultMergerProcessor.class.getName());
     
       public RowResultMergerProcessor(String databaseName,
    -      String tableName, SegmentProperties segProp, String tempStoreLocation,
    +      String tableName, SegmentProperties segProp, String[] tempStoreLocation,
           CarbonLoadModel loadModel, CompactionType compactionType) {
         this.segprop = segProp;
    -    if (!new File(tempStoreLocation).mkdirs()) {
    -      LOGGER.error("Error while new File(tempStoreLocation).mkdirs() ");
    +    for (String temLoc : tempStoreLocation) {
    +      if (!new File(temLoc).mkdirs()) {
    +        LOGGER.error("Error while new File(tempStoreLocation).mkdirs() ");
    --- End diff --
   
    the log should be LOGGER.error("Error while new File(tempLoc.mkdirs() ");


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1177#discussion_r129202230
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/merger/RowResultMergerProcessor.java ---
    @@ -57,12 +57,15 @@
           LogServiceFactory.getLogService(RowResultMergerProcessor.class.getName());
     
       public RowResultMergerProcessor(String databaseName,
    -      String tableName, SegmentProperties segProp, String tempStoreLocation,
    +      String tableName, SegmentProperties segProp, String[] tempStoreLocation,
           CarbonLoadModel loadModel, CompactionType compactionType) {
         this.segprop = segProp;
    -    if (!new File(tempStoreLocation).mkdirs()) {
    -      LOGGER.error("Error while new File(tempStoreLocation).mkdirs() ");
    +    for (String temLoc : tempStoreLocation) {
    +      if (!new File(temLoc).mkdirs()) {
    +        LOGGER.error("Error while new File(tempStoreLocation).mkdirs() ");
    --- End diff --
   
    remove new File(tempStoreLocation).mkdirs()
    can log the temLoc


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1177#discussion_r129204565
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/newflow/steps/CarbonRowDataWriterProcessorStepImpl.java ---
    @@ -112,9 +115,11 @@ private String getStoreLocation(CarbonTableIdentifier tableIdentifier, String pa
           isNoDictionaryDimensionColumn =
               CarbonDataProcessorUtil.getNoDictionaryMapping(configuration.getDataFields());
           measureDataType = configuration.getMeasureDataType();
    +      //choose a tmp location randomly
    --- End diff --
   
    why we moved to a local variable?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---