[GitHub] carbondata pull request #2836: [CARBONDATA-3027] Increase unsafe working mem...

[qiuchenjian-2]

GitHub user xubo245 opened a pull request:

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

    [CARBONDATA-3027] Increase unsafe working memory default size and add log file for SDK

    [CARBONDATA-3027] Increase unsafe working memory default size and add log file for SDK
   
    1.Increase unsafe working memory default size from 512m to 2048m
    2.add log file for SDK
   
    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     - [ ] Any interfaces changed?
     No
     - [ ] Any backward compatibility impacted?
     No
     - [ ] Document update required?
    No
     - [ ] Testing done
         No
     - [ ] 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/xubo245/carbondata CARBONDATA-3027MemoryException

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

    https://github.com/apache/carbondata/pull/2836.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 #2836
   
----
commit 90ea4ff1f73ac04cb035618a70b1458049fa0e86
Author: xubo245 <xubo29@...>
Date:   2018-10-19T03:50:40Z

    [CARBONDATA-3027] Increase unsafe working memory default size and add log file for SDK
    1.Increase unsafe working memory default size from 512m to 2048m
    2.add log file for SDK

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2836
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/877/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2836
 
    Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9142/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2836
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/1075/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2836#discussion_r226544482
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -1234,7 +1234,7 @@
     
       @CarbonProperty
       public static final String UNSAFE_WORKING_MEMORY_IN_MB = "carbon.unsafe.working.memory.in.mb";
    -  public static final String UNSAFE_WORKING_MEMORY_IN_MB_DEFAULT = "512";
    +  public static final String UNSAFE_WORKING_MEMORY_IN_MB_DEFAULT = "2048";
    --- End diff --
   
    What will happen if user do not have that much memory?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2836#discussion_r226548201
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -1234,7 +1234,7 @@
     
       @CarbonProperty
       public static final String UNSAFE_WORKING_MEMORY_IN_MB = "carbon.unsafe.working.memory.in.mb";
    -  public static final String UNSAFE_WORKING_MEMORY_IN_MB_DEFAULT = "512";
    +  public static final String UNSAFE_WORKING_MEMORY_IN_MB_DEFAULT = "2048";
    --- End diff --
   
    I don't know.


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2836
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/878/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2836
 
    Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9143/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2836
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/1076/



---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

    https://github.com/apache/carbondata/pull/2836
 
    so we should not change this value without better knowledge about it.
    If you really want to change it, the corresponding documents are needed to be changed too


---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

    https://github.com/apache/carbondata/pull/2836
 
    so we should not change this value without better knowledge about it.
    If you really want to change it, the corresponding documents are needed to be changed too


---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

    https://github.com/apache/carbondata/pull/2836
 
    so we should not change this value without better knowledge about it.
    If you really want to change it, the corresponding documents are needed to be changed too


---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

    https://github.com/apache/carbondata/pull/2836
 
    so we should not change this default value without better knowledge about it.
   
    If you really want to change it, the corresponding documents are needed to be changed at the same time.
   
    If you just encounter some error in your test, you can set/unset this property value to avoid modifying the default value.


---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

    https://github.com/apache/carbondata/pull/2836
 
    so we should not change this default value without better knowledge about it.
   
    If you really want to change it, the corresponding documents are needed to be changed at the same time.
   
    If you just encounter some error in your test, you can set/unset this property value to avoid modifying the default value.


---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

    https://github.com/apache/carbondata/pull/2836
 
    so we should not change this value without better knowledge about it.
    If you really want to change it, the corresponding documents are needed to be changed at the same time.
   
    If you just counter some failures in your test, you can set/unset this property value to avoid modifying the default value.


---

[qiuchenjian-2]

Github user jackylk commented on the issue:

    https://github.com/apache/carbondata/pull/2836
 
    Can you describe a issue you encounter using the original default value?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2836#discussion_r227622952
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -1234,7 +1234,7 @@
     
       @CarbonProperty
       public static final String UNSAFE_WORKING_MEMORY_IN_MB = "carbon.unsafe.working.memory.in.mb";
    -  public static final String UNSAFE_WORKING_MEMORY_IN_MB_DEFAULT = "512";
    +  public static final String UNSAFE_WORKING_MEMORY_IN_MB_DEFAULT = "1024";
    --- End diff --
   
    Can you describe the issue you encountered using the original default value?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2836#discussion_r227623050
 
    --- Diff: store/sdk/src/main/resources/log4j.properties ---
    @@ -0,0 +1,11 @@
    +# Root logger option
    +log4j.rootLogger=INFO,stdout
    +
    +
    +# Redirect log messages to console
    +log4j.appender.debug=org.apache.log4j.RollingFileAppender
    +log4j.appender.stdout=org.apache.log4j.ConsoleAppender
    +log4j.appender.stdout.Target=System.out
    +log4j.appender.stdout.layout=org.apache.log4j.PatternLayout
    +log4j.appender.stdout.layout.ConversionPattern=%d{yyyy-MM-dd HH:mm:ss} %-5p %c{1}:%L - %m%n
    --- End diff --
   
    Now we are using log4j Logger directly, you can add %C also in the pattern to print the class name


---

[qiuchenjian-2]

Github user xubo245 commented on the issue:

    https://github.com/apache/carbondata/pull/2836
 
    @xuchuanyin @jackylk I added some describe in ahead about this issue.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2836#discussion_r229170138
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -1234,7 +1234,7 @@
     
       @CarbonProperty
       public static final String UNSAFE_WORKING_MEMORY_IN_MB = "carbon.unsafe.working.memory.in.mb";
    -  public static final String UNSAFE_WORKING_MEMORY_IN_MB_DEFAULT = "512";
    +  public static final String UNSAFE_WORKING_MEMORY_IN_MB_DEFAULT = "1024";
    --- End diff --
   
    ok, I described the issue in ahead


---