[GitHub] carbondata pull request #2841: [WIP] Unsafe fallback to heap and unsafe quer...

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2841#discussion_r228692174
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryManager.java ---
    @@ -77,21 +77,12 @@
           if (takenSize < defaultSize) {
             takenSize = defaultSize;
             LOGGER.warn(String.format(
    -            "It is not recommended to set unsafe working memory size less than %sMB,"
    +            "It is not recommended to set offheap working memory size less than %sMB,"
                     + " so setting default value to %d",
                 CarbonCommonConstants.UNSAFE_WORKING_MEMORY_IN_MB_DEFAULT, defaultSize));
           }
           takenSize = takenSize * 1024 * 1024;
         } else {
    -      long maxMemory = Runtime.getRuntime().maxMemory() * 60 / 100;
    -      if (takenSize == 0L) {
    -        takenSize = maxMemory;
    -      } else {
    -        takenSize = takenSize * 1024 * 1024;
    -        if (takenSize > maxMemory) {
    -          takenSize = maxMemory;
    -        }
    -      }
    --- End diff --
   
    Please add a comment why memory limit is not required to take for ONHEAP


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2841#discussion_r228692328
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryManager.java ---
    @@ -125,13 +117,24 @@ private synchronized MemoryBlock allocateMemory(MemoryType memoryType, String ta
           }
           listOfMemoryBlock.add(memoryBlock);
           if (LOGGER.isDebugEnabled()) {
    -        LOGGER.debug(String.format("Creating working Memory block (%s) with size %d."
    +        LOGGER.debug(String.format("Creating Offheap working Memory block (%s) with size %d."
                     + " Total memory used %d Bytes, left %d Bytes.",
                 memoryBlock.toString(), memoryBlock.size(), memoryUsed, totalMemory - memoryUsed));
           }
    -      return memoryBlock;
    +    } else {
    +      memoryBlock = MemoryAllocator.HEAP.allocate(memoryRequested);
    +      Set<MemoryBlock> listOfMemoryBlock = taskIdToMemoryBlockMap.get(taskId);
    +      if (null == listOfMemoryBlock) {
    +        listOfMemoryBlock = new HashSet<>();
    +        taskIdToMemoryBlockMap.put(taskId, listOfMemoryBlock);
    --- End diff --
   
    Is it required to keep tracking of onheap blocks in map.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2841#discussion_r228692401
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryManager.java ---
    @@ -125,13 +117,24 @@ private synchronized MemoryBlock allocateMemory(MemoryType memoryType, String ta
           }
           listOfMemoryBlock.add(memoryBlock);
           if (LOGGER.isDebugEnabled()) {
    -        LOGGER.debug(String.format("Creating working Memory block (%s) with size %d."
    +        LOGGER.debug(String.format("Creating Offheap working Memory block (%s) with size %d."
                     + " Total memory used %d Bytes, left %d Bytes.",
                 memoryBlock.toString(), memoryBlock.size(), memoryUsed, totalMemory - memoryUsed));
           }
    -      return memoryBlock;
    +    } else {
    +      memoryBlock = MemoryAllocator.HEAP.allocate(memoryRequested);
    +      Set<MemoryBlock> listOfMemoryBlock = taskIdToMemoryBlockMap.get(taskId);
    +      if (null == listOfMemoryBlock) {
    +        listOfMemoryBlock = new HashSet<>();
    +        taskIdToMemoryBlockMap.put(taskId, listOfMemoryBlock);
    --- End diff --
   
    And also please change the `taskIdToMemoryBlockMap` to `taskIdToOffHeapBlockMap`


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2841#discussion_r228694119
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryManager.java ---
    @@ -77,21 +77,12 @@
           if (takenSize < defaultSize) {
             takenSize = defaultSize;
             LOGGER.warn(String.format(
    -            "It is not recommended to set unsafe working memory size less than %sMB,"
    +            "It is not recommended to set offheap working memory size less than %sMB,"
                     + " so setting default value to %d",
                 CarbonCommonConstants.UNSAFE_WORKING_MEMORY_IN_MB_DEFAULT, defaultSize));
           }
           takenSize = takenSize * 1024 * 1024;
         } else {
    -      long maxMemory = Runtime.getRuntime().maxMemory() * 60 / 100;
    -      if (takenSize == 0L) {
    -        takenSize = maxMemory;
    -      } else {
    -        takenSize = takenSize * 1024 * 1024;
    -        if (takenSize > maxMemory) {
    -          takenSize = maxMemory;
    -        }
    -      }
    --- End diff --
   
    add comment


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2841#discussion_r228694135
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryManager.java ---
    @@ -125,13 +117,24 @@ private synchronized MemoryBlock allocateMemory(MemoryType memoryType, String ta
           }
           listOfMemoryBlock.add(memoryBlock);
           if (LOGGER.isDebugEnabled()) {
    -        LOGGER.debug(String.format("Creating working Memory block (%s) with size %d."
    +        LOGGER.debug(String.format("Creating Offheap working Memory block (%s) with size %d."
                     + " Total memory used %d Bytes, left %d Bytes.",
                 memoryBlock.toString(), memoryBlock.size(), memoryUsed, totalMemory - memoryUsed));
           }
    -      return memoryBlock;
    +    } else {
    +      memoryBlock = MemoryAllocator.HEAP.allocate(memoryRequested);
    +      Set<MemoryBlock> listOfMemoryBlock = taskIdToMemoryBlockMap.get(taskId);
    +      if (null == listOfMemoryBlock) {
    +        listOfMemoryBlock = new HashSet<>();
    +        taskIdToMemoryBlockMap.put(taskId, listOfMemoryBlock);
    --- End diff --
   
    This is not required updated the same


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2841#discussion_r228694154
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryManager.java ---
    @@ -125,13 +117,24 @@ private synchronized MemoryBlock allocateMemory(MemoryType memoryType, String ta
           }
           listOfMemoryBlock.add(memoryBlock);
           if (LOGGER.isDebugEnabled()) {
    -        LOGGER.debug(String.format("Creating working Memory block (%s) with size %d."
    +        LOGGER.debug(String.format("Creating Offheap working Memory block (%s) with size %d."
                     + " Total memory used %d Bytes, left %d Bytes.",
                 memoryBlock.toString(), memoryBlock.size(), memoryUsed, totalMemory - memoryUsed));
           }
    -      return memoryBlock;
    +    } else {
    +      memoryBlock = MemoryAllocator.HEAP.allocate(memoryRequested);
    +      Set<MemoryBlock> listOfMemoryBlock = taskIdToMemoryBlockMap.get(taskId);
    +      if (null == listOfMemoryBlock) {
    +        listOfMemoryBlock = new HashSet<>();
    +        taskIdToMemoryBlockMap.put(taskId, listOfMemoryBlock);
    --- End diff --
   
    updated the variable name


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2841
 
    LGTM


---

[qiuchenjian-2]

Github user asfgit closed the pull request at:

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


---