[GitHub] carbondata pull request #2397: [HOTFIX]ADD carbon.load.sortMemory.spill.perc...

[qiuchenjian-2]

GitHub user ndwangsen opened a pull request:

    https://github.com/apache/carbondata/pull/2397

    [HOTFIX]ADD carbon.load.sortMemory.spill.percentage parameter  invalid value check

    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     - [ ] Any interfaces changed?
     NA
     - [ ] Any backward compatibility impacted?
     NO
     - [ ] Document update required?
    NO
     - [ ] Testing done
            Please provide details on
            - Whether new unit test cases have been added or why no new tests are required?
            Testing done UT
            - How it is tested? Please attach test report.
            add example for it
            - Is it a performance related change? Please attach the performance test report.
              NA
            - Any additional information to help reviewers in testing this change.
            NA
     - [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
           NO


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/ndwangsen/incubator-carbondata bugfix_dts12160

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/carbondata/pull/2397.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2397
   
----
commit 736c571ce911374d8cde16f9d6f64b310984f5e9
Author: ndwangsen <luffy.wang@...>
Date:   2018-06-22T04:02:36Z

    ADD carbon.load.sortMemory.spill.percentage parameter  invalid value
    check

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2397
 
    Can one of the admins verify this patch?


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2397
 
    Can one of the admins verify this patch?


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2397
 
    Can one of the admins verify this patch?


---

[qiuchenjian-2]

Github user chenliang613 commented on the issue:

    https://github.com/apache/carbondata/pull/2397
 
    add to whitelist


---

[qiuchenjian-2]

Github user chenliang613 commented on the issue:

    https://github.com/apache/carbondata/pull/2397
 
    Can you provide more background? why need to add this parameter?


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2397
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5298/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2397#discussion_r197371819
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonLoadOptionConstants.java ---
    @@ -144,8 +144,8 @@
        * If the sort memory is insufficient, spill inmemory pages to disk.
        * The total amount of pages is at most the specified percentage of total sort memory. Default
        * value 0 means that no pages will be spilled and the newly incoming pages will be spilled,
    -   * whereas value 1 means that all pages will be spilled and newly incoming pages will be loaded
    -   * into sort memory.
    +   * whereas value 100 means that all pages will be spilled and newly incoming pages will be loaded
    +   * into sort memory,Other percentage values range 0-100.
    --- End diff --
   
    add a space before 'Other'


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2397#discussion_r197372024
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/integration/spark/testsuite/dataload/TestLoadDataWithUnsafeMemory.scala ---
    @@ -64,6 +67,8 @@ class TestLoadDataWithUnsafeMemory extends QueryTest
           .addProperty(CarbonCommonConstants.UNSAFE_WORKING_MEMORY_IN_MB, "512")
         CarbonProperties.getInstance()
           .addProperty(CarbonCommonConstants.OFFHEAP_SORT_CHUNK_SIZE_IN_MB, "512")
    +    CarbonProperties.getInstance()
    +      .addProperty(CarbonLoadOptionConstants.CARBON_LOAD_SORT_MEMORY_SPILL_PERCENTAGE, "-1")
    --- End diff --
   
    why use "-1" here?


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ndwangsen closed the pull request at:

    https://github.com/apache/carbondata/pull/2397


---

[qiuchenjian-2]

GitHub user ndwangsen reopened a pull request:

    https://github.com/apache/carbondata/pull/2397

    [CARBONDATA-2644][Dataload]ADD carbon.load.sortMemory.spill.percentage parameter  invalid value check

    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     - [x] Any interfaces changed?
     NA
     - [x] Any backward compatibility impacted?
     NO
     - [x] Document update required?
    NO
     - [x] Testing done
            Please provide details on
            - Whether new unit test cases have been added or why no new tests are required?
            Testing done UT
            - How it is tested? Please attach test report.
            add example for it
            - Is it a performance related change? Please attach the performance test report.
              NA
            - Any additional information to help reviewers in testing this change.
            NA
     - [x] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
           NO


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/ndwangsen/incubator-carbondata bugfix_dts12160

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/carbondata/pull/2397.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2397
   
----
commit 736c571ce911374d8cde16f9d6f64b310984f5e9
Author: ndwangsen <luffy.wang@...>
Date:   2018-06-22T04:02:36Z

    ADD carbon.load.sortMemory.spill.percentage parameter  invalid value
    check

----


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2397#discussion_r197688936
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonLoadOptionConstants.java ---
    @@ -144,8 +144,8 @@
        * If the sort memory is insufficient, spill inmemory pages to disk.
        * The total amount of pages is at most the specified percentage of total sort memory. Default
        * value 0 means that no pages will be spilled and the newly incoming pages will be spilled,
    -   * whereas value 1 means that all pages will be spilled and newly incoming pages will be loaded
    -   * into sort memory.
    +   * whereas value 100 means that all pages will be spilled and newly incoming pages will be loaded
    +   * into sort memory,Other percentage values range 0-100.
    --- End diff --
   
    ok


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2397#discussion_r197688967
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/integration/spark/testsuite/dataload/TestLoadDataWithUnsafeMemory.scala ---
    @@ -64,6 +67,8 @@ class TestLoadDataWithUnsafeMemory extends QueryTest
           .addProperty(CarbonCommonConstants.UNSAFE_WORKING_MEMORY_IN_MB, "512")
         CarbonProperties.getInstance()
           .addProperty(CarbonCommonConstants.OFFHEAP_SORT_CHUNK_SIZE_IN_MB, "512")
    +    CarbonProperties.getInstance()
    +      .addProperty(CarbonLoadOptionConstants.CARBON_LOAD_SORT_MEMORY_SPILL_PERCENTAGE, "-1")
    --- End diff --
   
    test the range of 0-100


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2397#discussion_r197690211
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/merger/UnsafeIntermediateMerger.java ---
    @@ -88,6 +88,10 @@ public UnsafeIntermediateMerger(SortParameters parameters) {
               CarbonLoadOptionConstants.CARBON_LOAD_SORT_MEMORY_SPILL_PERCENTAGE,
               CarbonLoadOptionConstants.CARBON_LOAD_SORT_MEMORY_SPILL_PERCENTAGE_DEFAULT);
           spillPercentage = Integer.valueOf(spillPercentageStr);
    +      if (spillPercentage > 100 || spillPercentage < 0) {
    --- End diff --
   
    One suggestion: add a function, called 'getSortMemorySpillPercentage', in 'CarbonProperties' to get the value of 'CARBON_LOAD_SORT_MEMORY_SPILL_PERCENTAGE', and validate the value in this function. We don't need to validate this value again if other place need to use this config, just need to call 'getSortMemorySpillPercentage'.


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2397
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5349/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2397#discussion_r199041131
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/merger/UnsafeIntermediateMerger.java ---
    @@ -88,6 +88,10 @@ public UnsafeIntermediateMerger(SortParameters parameters) {
               CarbonLoadOptionConstants.CARBON_LOAD_SORT_MEMORY_SPILL_PERCENTAGE,
               CarbonLoadOptionConstants.CARBON_LOAD_SORT_MEMORY_SPILL_PERCENTAGE_DEFAULT);
           spillPercentage = Integer.valueOf(spillPercentageStr);
    +      if (spillPercentage > 100 || spillPercentage < 0) {
    --- End diff --
   
    property validation can be validated in `CarbonProperties.validateAndLoadDefaultProperties`, please add it there. And I agree with @zzcclp , you can add a function in `CarbonProperties` to get the value of this property, just like `getInsertIntoDatasetStorageLevel`


---