You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@carbondata.apache.org by ajantha-bhat <gi...@git.apache.org> on 2018/08/30 14:22:42 UTC
[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...
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 <aj...@...>
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.
----
---
[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/
---
[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...
Posted by kumarvishal09 <gi...@git.apache.org>.
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
---
[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...
Posted by ajantha-bhat <gi...@git.apache.org>.
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.
---
[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/
---
[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...
Posted by kumarvishal09 <gi...@git.apache.org>.
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
---
[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...
Posted by ajantha-bhat <gi...@git.apache.org>.
Github user ajantha-bhat commented on the issue:
https://github.com/apache/carbondata/pull/2675
@kumarvishal09 , @ravipesala : please review
---
[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...
Posted by jackylk <gi...@git.apache.org>.
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
---
[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/
---
[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/
---
[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...
Posted by kumarvishal09 <gi...@git.apache.org>.
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
---
[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/
---
[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...
Posted by ajantha-bhat <gi...@git.apache.org>.
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
---
[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/
---
[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...
Posted by jackylk <gi...@git.apache.org>.
Github user jackylk commented on the issue:
https://github.com/apache/carbondata/pull/2675
LGTM
---
[GitHub] carbondata issue #2675: [CARBONDATA-2901] Fixed JVM crash in Load scenario w...
Posted by kumarvishal09 <gi...@git.apache.org>.
Github user kumarvishal09 commented on the issue:
https://github.com/apache/carbondata/pull/2675
LGTM
---
[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/carbondata/pull/2675
---
[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...
Posted by ajantha-bhat <gi...@git.apache.org>.
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
---
[GitHub] carbondata pull request #2675: [CARBONDATA-2901] Fixed JVM crash in Load sce...
Posted by ajantha-bhat <gi...@git.apache.org>.
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.
---