[GitHub] carbondata pull request #1455: [CARBONDATA-1624]Set the default value of 'ca...

[qiuchenjian-2]

Github user jackylk commented on the issue:

    https://github.com/apache/carbondata/pull/1455
 
    Please do not delete the PR template, it is useful for reviewer and committer to maintain this PR later. Please add it to the description of this PR:
   
    ```
     - [X] Any interfaces changed?
       No
   
     - [X] Any backward compatibility impacted?
       No
   
     - [X] Document update required?
       Yes, the XXX part document need to be updated
   
     - [X] Testing done
       A new testcase for XX is added
         
     - [X] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
        NA
    ```


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1455#discussion_r148946384
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/management/LoadTableCommand.scala ---
    @@ -84,6 +84,10 @@ case class LoadTableCommand(
     
         val carbonProperty: CarbonProperties = CarbonProperties.getInstance()
         carbonProperty.addProperty("zookeeper.enable.lock", "false")
    +    carbonProperty.addProperty(CarbonCommonConstants.NUM_CORES_LOADING,
    +        carbonProperty.getProperty(CarbonCommonConstants.NUM_CORES_LOADING,
    +            Math.min(sparkSession.sparkContext.conf.getInt("spark.executor.cores", 1),
    +                CarbonCommonConstants.NUM_CORES_MAX_VAL).toString()))
    --- End diff --
   
    Instead of using `NUM_CORES_MAX_VAL`
    You can modify `NUM_CORES_DEFAULT_VAL` to 32 directly, so that user does not need to provide `NUM_CORES_DEFAULT_VAL`


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1455#discussion_r148946439
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/management/LoadTableCommand.scala ---
    @@ -84,6 +84,10 @@ case class LoadTableCommand(
     
         val carbonProperty: CarbonProperties = CarbonProperties.getInstance()
         carbonProperty.addProperty("zookeeper.enable.lock", "false")
    +    carbonProperty.addProperty(CarbonCommonConstants.NUM_CORES_LOADING,
    +        carbonProperty.getProperty(CarbonCommonConstants.NUM_CORES_LOADING,
    +            Math.min(sparkSession.sparkContext.conf.getInt("spark.executor.cores", 1),
    +                CarbonCommonConstants.NUM_CORES_MAX_VAL).toString()))
    --- End diff --
   
    The logic of this code block will be:
   
    ```
    // TODO: describe why doing this
    val numCoresLoading =
         Math.min(
             sparkSession.sparkContext.conf.getInt("spark.executor.cores", 1),
             carbonProperty.getProperty(CarbonCommonConstants.NUM_CORES_LOADING, NUM_CORES_DEFAULT_VAL)
         )
        // update the property with new value
        carbonProperty.addProperty(CarbonCommonConstants.NUM_CORES_LOADING, numCoresLoading)
    ```


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1455#discussion_r148946497
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/CarbonProperties.java ---
    @@ -672,6 +672,9 @@ public int getNumberOfCores() {
               .getProperty(CarbonCommonConstants.NUM_CORES_LOADING,
                   CarbonCommonConstants.NUM_CORES_DEFAULT_VAL));
    --- End diff --
   
    Since now we always update the CarbonCommonConstants.NUM_CORES_LOADING value in LoadCommand, we should not use default value here. You can use `String getProperty(String key)` directly.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1455#discussion_r148963119
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/management/LoadTableCommand.scala ---
    @@ -84,6 +84,10 @@ case class LoadTableCommand(
     
         val carbonProperty: CarbonProperties = CarbonProperties.getInstance()
         carbonProperty.addProperty("zookeeper.enable.lock", "false")
    +    carbonProperty.addProperty(CarbonCommonConstants.NUM_CORES_LOADING,
    +        carbonProperty.getProperty(CarbonCommonConstants.NUM_CORES_LOADING,
    +            Math.min(sparkSession.sparkContext.conf.getInt("spark.executor.cores", 1),
    +                CarbonCommonConstants.NUM_CORES_MAX_VAL).toString()))
    --- End diff --
   
    Can't modify *NUM_CORES_DEFAULT_VAL* to 32 directly, there are some places to use *NUM_CORES_DEFAULT_VAL*, for example:
    in org.apache.carbondata.core.datastore.BlockIndexStore.getAll:
    `try {`
    `      numberOfCores = Integer.parseInt(CarbonProperties.getInstance()`
    `          .getProperty(CarbonCommonConstants.NUM_CORES,`
    `              CarbonCommonConstants.NUM_CORES_DEFAULT_VAL));`
    `    } catch (NumberFormatException e) {`
    `      numberOfCores = Integer.parseInt(CarbonCommonConstants.NUM_CORES_DEFAULT_VAL);`
    `    }`
   
    32 is too big.
   



---

[qiuchenjian-2]

Github user zzcclp commented on the issue:

    https://github.com/apache/carbondata/pull/1455
 
    @jackylk please review.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1455#discussion_r148987672
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/management/LoadTableCommand.scala ---
    @@ -84,6 +84,32 @@ case class LoadTableCommand(
     
         val carbonProperty: CarbonProperties = CarbonProperties.getInstance()
         carbonProperty.addProperty("zookeeper.enable.lock", "false")
    +
    +    val numCoresLoading =
    +      try {
    +        Integer.parseInt(CarbonProperties.getInstance()
    +            .getProperty(CarbonCommonConstants.NUM_CORES_LOADING,
    +                CarbonCommonConstants.NUM_CORES_MAX_VAL.toString()))
    +      } catch {
    +        case exc: NumberFormatException =>
    +          LOGGER.error("Configured value for property " + CarbonCommonConstants.NUM_CORES_LOADING
    +              + " is wrong. ")
    +          CarbonCommonConstants.NUM_CORES_MAX_VAL
    +      }
    +    // Get the minimum value of 'spark.executor.cores' and NUM_CORES_LOADING,
    +    // If user set the NUM_CORES_LOADING, it can't exceed the value of 'spark.executor.cores';
    +    // If user doesn't set the NUM_CORES_LOADING, it will use the value of 'spark.executor.cores',
    +    // but the value can't exceed the value of NUM_CORES_MAX_VAL,
    +    // NUM_CORES_LOADING's default value is NUM_CORES_MAX_VAL;
    +    val newNumCoresLoading =
    +      Math.min(
    +          sparkSession.sparkContext.conf.getInt("spark.executor.cores", 1),
    +          numCoresLoading
    +      )
    +    // update the property with new value
    +    carbonProperty.addProperty(CarbonCommonConstants.NUM_CORES_LOADING,
    +        newNumCoresLoading.toString())
    +
    --- End diff --
   
    I think you can set the `spark.task.cpus` here so that spark will know carbon is using more cores for one task.


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1455
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1452/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1455#discussion_r148997065
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/management/LoadTableCommand.scala ---
    @@ -84,6 +84,32 @@ case class LoadTableCommand(
     
         val carbonProperty: CarbonProperties = CarbonProperties.getInstance()
         carbonProperty.addProperty("zookeeper.enable.lock", "false")
    +
    +    val numCoresLoading =
    +      try {
    +        Integer.parseInt(CarbonProperties.getInstance()
    +            .getProperty(CarbonCommonConstants.NUM_CORES_LOADING,
    +                CarbonCommonConstants.NUM_CORES_MAX_VAL.toString()))
    +      } catch {
    +        case exc: NumberFormatException =>
    +          LOGGER.error("Configured value for property " + CarbonCommonConstants.NUM_CORES_LOADING
    +              + " is wrong. ")
    +          CarbonCommonConstants.NUM_CORES_MAX_VAL
    +      }
    +    // Get the minimum value of 'spark.executor.cores' and NUM_CORES_LOADING,
    +    // If user set the NUM_CORES_LOADING, it can't exceed the value of 'spark.executor.cores';
    +    // If user doesn't set the NUM_CORES_LOADING, it will use the value of 'spark.executor.cores',
    +    // but the value can't exceed the value of NUM_CORES_MAX_VAL,
    +    // NUM_CORES_LOADING's default value is NUM_CORES_MAX_VAL;
    +    val newNumCoresLoading =
    +      Math.min(
    +          sparkSession.sparkContext.conf.getInt("spark.executor.cores", 1),
    +          numCoresLoading
    +      )
    +    // update the property with new value
    +    carbonProperty.addProperty(CarbonCommonConstants.NUM_CORES_LOADING,
    +        newNumCoresLoading.toString())
    +
    --- End diff --
   
    I think it is unnecessary, If do so, it will affect other jobs and reduce the task parallelism, right?


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user zzcclp commented on the issue:

    https://github.com/apache/carbondata/pull/1455
 
    @jackylk please review, thanks.


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user zzcclp commented on the issue:

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


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1455
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1461/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user zzcclp commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1455
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/834/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1455
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1467/



---