[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...

qiuchenjian-2
GitHub user ajantha-bhat opened a pull request:

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

    [CARBONDATA-2901] Fixed JVM crash in Load scenario when unsafe memory allocation is failed.

    Problem: Jvm crash in Load scenario when unsafe memory allocation is failed.
   
    scenario:
    a) Have many cores while loading. suggested more than 10. [carbon.number.of.cores.while.loading]
    b) Load huge data with local sort, more than 5GB (keeping default unsafe memory manager as 512 MB)
    c) when task failes due to not enough unsafae memory, JVM crashes with SIGSEGV.
   
   
    root casue:
    while sorting, all iterator threads are waiting at UnsafeSortDataRows.addRowBatch as all iterator works on one row page.
    Only one iterator thread will try to allocate memory. Before that it has freed current page in handlePreviousPage().
    When allocate memory failed, row page will still have that old reference. next thread will again use same reference and call handlePreviousPage().
    So, Jvm crashes as freed memory is accessed.
   
   
    solution:
    When allocation failed, set row page reference to null.
    So, that next thread will not do any operation.

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

    $ git pull https://github.com/ajantha-bhat/carbondata master

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

    https://github.com/apache/carbondata/pull/2675.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 #2675
   
----
commit 8ec20643726db944d1ee8bcc0d5e75568888a83c
Author: ajantha-bhat <ajanthabhat@...>
Date:   2018-08-30T14:18:34Z

    [CARBONDATA-2901] Fixed JVM crash in Load scenario when unsafe memory allocation is failed.
   
    Problem: Jvm crash in Load scenario when unsafe memory allocation is
    failed.
   
    scenario:
    a) Have many cores while loading. suggested more than 10.
    [carbon.number.of.cores.while.loading]
    b) Load huge data with local sort, more than 5GB (keeping default unsafe
    memory manager as 512 MB)
    c) when task failes due to not enough unsafae memory, JVM crashes with
    SIGSEGV.
   
    root casue:
    while sorting, all iterator threads are waiting at
    UnsafeSortDataRows.addRowBatch as all iterator works on one row page.
    Only one iterator thread will try to allocate memory. Before that it has
    freed current page in handlePreviousPage().
    When allocate memory failed, row page will still have that old
    reference. next thread will again use same reference and call
    handlePreviousPage().
    So, Jvm crashes as freed memory is accessed.
   
    solution:
    When allocation failed, set row page reference to null.
    So, that next thread will not do any operation.

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...

qiuchenjian-2
Github user ajantha-bhat commented on the issue:

    https://github.com/apache/carbondata/pull/2675
 
    @kumarvishal09 , @ravipesala : please review


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user CarbonDataQA commented on the issue:

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



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user CarbonDataQA commented on the issue:

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



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2675#discussion_r214279843
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/UnsafeSortDataRows.java ---
    @@ -140,19 +143,25 @@ public void initialize() throws MemoryException, CarbonSortKeyAndGroupByExceptio
         semaphore = new Semaphore(parameters.getNumberOfCores());
       }
     
    -  private UnsafeCarbonRowPage createUnsafeRowPage()
    -      throws MemoryException, CarbonSortKeyAndGroupByException {
    -    MemoryBlock baseBlock =
    -        UnsafeMemoryManager.allocateMemoryWithRetry(this.taskId, inMemoryChunkSize);
    -    boolean isMemoryAvailable =
    -        UnsafeSortMemoryManager.INSTANCE.isMemoryAvailable(baseBlock.size());
    -    if (isMemoryAvailable) {
    -      UnsafeSortMemoryManager.INSTANCE.allocateDummyMemory(baseBlock.size());
    -    } else {
    -      // merge and spill in-memory pages to disk if memory is not enough
    -      unsafeInMemoryIntermediateFileMerger.tryTriggerInmemoryMerging(true);
    +  private UnsafeCarbonRowPage createUnsafeRowPage() {
    +    try {
    +      MemoryBlock baseBlock =
    +          UnsafeMemoryManager.allocateMemoryWithRetry(this.taskId, inMemoryChunkSize);
    +      boolean isMemoryAvailable =
    +          UnsafeSortMemoryManager.INSTANCE.isMemoryAvailable(baseBlock.size());
    +      if (isMemoryAvailable) {
    +        UnsafeSortMemoryManager.INSTANCE.allocateDummyMemory(baseBlock.size());
    +      } else {
    +        // merge and spill in-memory pages to disk if memory is not enough
    +        unsafeInMemoryIntermediateFileMerger.tryTriggerInmemoryMerging(true);
    +      }
    +      return new UnsafeCarbonRowPage(tableFieldStat, baseBlock, !isMemoryAvailable, taskId);
    +    } catch (MemoryException | CarbonSortKeyAndGroupByException e) {
    +      // This will set rowPage reference to null. If not set, other threads will use same reference.
    +      // As handlePreviousPage() free the rowPage.
    +      // If not set to null, rowPage will be accessed again after free by other thread.
    +      return null;
    --- End diff --
   
    I think it is better to throw MemoryException to call so caller no need to check whether it is null


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2675#discussion_r214306748
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/UnsafeSortDataRows.java ---
    @@ -140,19 +143,25 @@ public void initialize() throws MemoryException, CarbonSortKeyAndGroupByExceptio
         semaphore = new Semaphore(parameters.getNumberOfCores());
       }
     
    -  private UnsafeCarbonRowPage createUnsafeRowPage()
    -      throws MemoryException, CarbonSortKeyAndGroupByException {
    -    MemoryBlock baseBlock =
    -        UnsafeMemoryManager.allocateMemoryWithRetry(this.taskId, inMemoryChunkSize);
    -    boolean isMemoryAvailable =
    -        UnsafeSortMemoryManager.INSTANCE.isMemoryAvailable(baseBlock.size());
    -    if (isMemoryAvailable) {
    -      UnsafeSortMemoryManager.INSTANCE.allocateDummyMemory(baseBlock.size());
    -    } else {
    -      // merge and spill in-memory pages to disk if memory is not enough
    -      unsafeInMemoryIntermediateFileMerger.tryTriggerInmemoryMerging(true);
    +  private UnsafeCarbonRowPage createUnsafeRowPage() {
    +    try {
    +      MemoryBlock baseBlock =
    +          UnsafeMemoryManager.allocateMemoryWithRetry(this.taskId, inMemoryChunkSize);
    +      boolean isMemoryAvailable =
    +          UnsafeSortMemoryManager.INSTANCE.isMemoryAvailable(baseBlock.size());
    +      if (isMemoryAvailable) {
    +        UnsafeSortMemoryManager.INSTANCE.allocateDummyMemory(baseBlock.size());
    +      } else {
    +        // merge and spill in-memory pages to disk if memory is not enough
    +        unsafeInMemoryIntermediateFileMerger.tryTriggerInmemoryMerging(true);
    +      }
    +      return new UnsafeCarbonRowPage(tableFieldStat, baseBlock, !isMemoryAvailable, taskId);
    +    } catch (MemoryException | CarbonSortKeyAndGroupByException e) {
    +      // This will set rowPage reference to null. If not set, other threads will use same reference.
    +      // As handlePreviousPage() free the rowPage.
    +      // If not set to null, rowPage will be accessed again after free by other thread.
    +      return null;
    --- End diff --
   
    Issue came because of this itself. Throwing exception will not set rowPage reference to null. So, other thread will access this rowPage. But rowPage was already freed from previous thread. hence jvm crash


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user kumarvishal09 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2675#discussion_r214545843
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/UnsafeSortDataRows.java ---
    @@ -127,8 +127,11 @@ public UnsafeSortDataRows(SortParameters parameters,
       /**
        * This method will be used to initialize
        */
    -  public void initialize() throws MemoryException, CarbonSortKeyAndGroupByException {
    +  public void initialize() throws MemoryException {
         this.rowPage = createUnsafeRowPage();
    +    if (this.rowPage == null) {
    --- End diff --
   
    No need to check null case here if it initialize fails throw exception no need to check for null


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user kumarvishal09 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2675#discussion_r214545853
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/UnsafeSortDataRows.java ---
    @@ -140,19 +143,25 @@ public void initialize() throws MemoryException, CarbonSortKeyAndGroupByExceptio
         semaphore = new Semaphore(parameters.getNumberOfCores());
       }
     
    -  private UnsafeCarbonRowPage createUnsafeRowPage()
    -      throws MemoryException, CarbonSortKeyAndGroupByException {
    -    MemoryBlock baseBlock =
    -        UnsafeMemoryManager.allocateMemoryWithRetry(this.taskId, inMemoryChunkSize);
    -    boolean isMemoryAvailable =
    -        UnsafeSortMemoryManager.INSTANCE.isMemoryAvailable(baseBlock.size());
    -    if (isMemoryAvailable) {
    -      UnsafeSortMemoryManager.INSTANCE.allocateDummyMemory(baseBlock.size());
    -    } else {
    -      // merge and spill in-memory pages to disk if memory is not enough
    -      unsafeInMemoryIntermediateFileMerger.tryTriggerInmemoryMerging(true);
    +  private UnsafeCarbonRowPage createUnsafeRowPage() {
    +    try {
    +      MemoryBlock baseBlock =
    +          UnsafeMemoryManager.allocateMemoryWithRetry(this.taskId, inMemoryChunkSize);
    +      boolean isMemoryAvailable =
    +          UnsafeSortMemoryManager.INSTANCE.isMemoryAvailable(baseBlock.size());
    +      if (isMemoryAvailable) {
    +        UnsafeSortMemoryManager.INSTANCE.allocateDummyMemory(baseBlock.size());
    +      } else {
    +        // merge and spill in-memory pages to disk if memory is not enough
    +        unsafeInMemoryIntermediateFileMerger.tryTriggerInmemoryMerging(true);
    +      }
    +      return new UnsafeCarbonRowPage(tableFieldStat, baseBlock, !isMemoryAvailable, taskId);
    +    } catch (MemoryException | CarbonSortKeyAndGroupByException e) {
    --- End diff --
   
    Here we are eating up what type of exception was thrown


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user kumarvishal09 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2675#discussion_r214545865
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/UnsafeSortDataRows.java ---
    @@ -188,13 +197,19 @@ public void addRowBatchWithOutSync(Object[][] rowBatch, int size)
       }
     
       private void addBatch(Object[][] rowBatch, int size) throws CarbonSortKeyAndGroupByException {
    +    if (rowPage == null) {
    --- End diff --
   
    No need to check for null here as this scenario will never occur


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2675#discussion_r214546342
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/UnsafeSortDataRows.java ---
    @@ -127,8 +127,11 @@ public UnsafeSortDataRows(SortParameters parameters,
       /**
        * This method will be used to initialize
        */
    -  public void initialize() throws MemoryException, CarbonSortKeyAndGroupByException {
    +  public void initialize() throws MemoryException {
         this.rowPage = createUnsafeRowPage();
    +    if (this.rowPage == null) {
    --- End diff --
   
    ok. removed it


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2675#discussion_r214546377
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/UnsafeSortDataRows.java ---
    @@ -140,19 +143,25 @@ public void initialize() throws MemoryException, CarbonSortKeyAndGroupByExceptio
         semaphore = new Semaphore(parameters.getNumberOfCores());
       }
     
    -  private UnsafeCarbonRowPage createUnsafeRowPage()
    -      throws MemoryException, CarbonSortKeyAndGroupByException {
    -    MemoryBlock baseBlock =
    -        UnsafeMemoryManager.allocateMemoryWithRetry(this.taskId, inMemoryChunkSize);
    -    boolean isMemoryAvailable =
    -        UnsafeSortMemoryManager.INSTANCE.isMemoryAvailable(baseBlock.size());
    -    if (isMemoryAvailable) {
    -      UnsafeSortMemoryManager.INSTANCE.allocateDummyMemory(baseBlock.size());
    -    } else {
    -      // merge and spill in-memory pages to disk if memory is not enough
    -      unsafeInMemoryIntermediateFileMerger.tryTriggerInmemoryMerging(true);
    +  private UnsafeCarbonRowPage createUnsafeRowPage() {
    +    try {
    +      MemoryBlock baseBlock =
    +          UnsafeMemoryManager.allocateMemoryWithRetry(this.taskId, inMemoryChunkSize);
    +      boolean isMemoryAvailable =
    +          UnsafeSortMemoryManager.INSTANCE.isMemoryAvailable(baseBlock.size());
    +      if (isMemoryAvailable) {
    +        UnsafeSortMemoryManager.INSTANCE.allocateDummyMemory(baseBlock.size());
    +      } else {
    +        // merge and spill in-memory pages to disk if memory is not enough
    +        unsafeInMemoryIntermediateFileMerger.tryTriggerInmemoryMerging(true);
    +      }
    +      return new UnsafeCarbonRowPage(tableFieldStat, baseBlock, !isMemoryAvailable, taskId);
    +    } catch (MemoryException | CarbonSortKeyAndGroupByException e) {
    --- End diff --
   
    But caller always throw CarbonSortKeyAndGroupByException.
   
    but still modified code as per suggestion.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2675#discussion_r214546428
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/UnsafeSortDataRows.java ---
    @@ -188,13 +197,19 @@ public void addRowBatchWithOutSync(Object[][] rowBatch, int size)
       }
     
       private void addBatch(Object[][] rowBatch, int size) throws CarbonSortKeyAndGroupByException {
    +    if (rowPage == null) {
    --- End diff --
   
    when other iterator enters , it will be made null by previous iterator in case of failure. hence it is required.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user CarbonDataQA commented on the issue:

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



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2675
 
    Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/165/



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user CarbonDataQA commented on the issue:

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



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user CarbonDataQA commented on the issue:

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



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user kumarvishal09 commented on the issue:

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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user jackylk commented on the issue:

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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user asfgit closed the pull request at:

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


---