You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by ilooner <gi...@git.apache.org> on 2018/04/20 23:58:09 UTC
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
GitHub user ilooner opened a pull request:
https://github.com/apache/drill/pull/1234
DRILL-5927: Fixed memory leak in TestBsonRecordReader, and sped up th…
…e test.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ilooner/drill DRILL-5927
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1234.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 #1234
----
commit 7d431e20c9cdb0038116131aecdc26a1f60e8775
Author: Timothy Farkas <ti...@...>
Date: 2018-04-20T23:52:59Z
DRILL-5927: Fixed memory leak in TestBsonRecordReader, and sped up the test.
----
---
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/1234#discussion_r183846974
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/bson/TestBsonRecordReader.java ---
@@ -272,6 +276,9 @@ public static void cleanUp() {
} catch (Exception e) {
}
+
+ buffer.close();
--- End diff --
k thx
---
[GitHub] drill issue #1234: DRILL-5927: Fixed memory leak in TestBsonRecordReader, an...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on the issue:
https://github.com/apache/drill/pull/1234
@parthchandra Please review
---
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1234#discussion_r183562766
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/bson/TestBsonRecordReader.java ---
@@ -272,6 +276,9 @@ public static void cleanUp() {
} catch (Exception e) {
}
+
+ buffer.close();
--- End diff --
buffer.release()?
---
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1234#discussion_r183741516
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/bson/TestBsonRecordReader.java ---
@@ -49,17 +50,20 @@
import org.junit.BeforeClass;
import org.junit.Test;
-public class TestBsonRecordReader extends BaseTestQuery {
+public class TestBsonRecordReader {
+ private static BufferAllocator allocator;
--- End diff --
What is a reason `allocator` and other variables are initialized in `@BeforeClass`? Can they be initialized in `@Before` instead?
---
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/1234#discussion_r183608064
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/bson/TestBsonRecordReader.java ---
@@ -49,17 +50,20 @@
import org.junit.BeforeClass;
import org.junit.Test;
-public class TestBsonRecordReader extends BaseTestQuery {
+public class TestBsonRecordReader {
+ private static BufferAllocator allocator;
--- End diff --
JUnit requires tests annotated with @BeforeClass and @AfterClass to be static so these variables have to be static. Currently the Drill unit tests do not support concurrent execution of methods within a test class within the same process. Drill has surefire launch multiple test processes and the the test classes to execute are divided among the test processes. Within each forked surefire process tests are executed sequentially, so this will not be an issue.
---
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/1234#discussion_r183611338
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/bson/TestBsonRecordReader.java ---
@@ -49,17 +50,20 @@
import org.junit.BeforeClass;
import org.junit.Test;
-public class TestBsonRecordReader extends BaseTestQuery {
+public class TestBsonRecordReader {
+ private static BufferAllocator allocator;
private static VectorContainerWriter writer;
private static TestOutputMutator mutator;
+ private static DrillBuf buffer;
private static BsonRecordReader bsonReader;
@BeforeClass
public static void setUp() {
- BufferAllocator bufferAllocator = getDrillbitContext().getAllocator();
- mutator = new TestOutputMutator(bufferAllocator);
+ allocator = new RootAllocator(100_000_000);
--- End diff --
After testing just now, the lowest I could push this was 9,000,000 bytes. Going to 8,000,000 bytes causes an OOM. It looks like the BsonRecordReader allocates a bunch of value vectors. I will adjust this to 9,000,000.
```
org.apache.drill.exec.exception.OutOfMemoryException: Unable to allocate buffer of size 8192 due to memory limit (800000). Current allocation: 795648
at org.apache.drill.exec.memory.BaseAllocator.buffer(BaseAllocator.java:236)
at org.apache.drill.exec.memory.BaseAllocator.buffer(BaseAllocator.java:211)
at org.apache.drill.exec.vector.UInt1Vector.reallocRaw(UInt1Vector.java:262)
at org.apache.drill.exec.vector.UInt1Vector.reAlloc(UInt1Vector.java:251)
at org.apache.drill.exec.vector.UInt1Vector$Mutator.setSafe(UInt1Vector.java:480)
at org.apache.drill.exec.vector.NullableVarCharVector$Mutator.setSafe(NullableVarCharVector.java:608)
at org.apache.drill.exec.vector.complex.impl.NullableVarCharWriterImpl.write(NullableVarCharWriterImpl.java:110)
at org.apache.drill.exec.store.bson.BsonRecordReader.writeString(BsonRecordReader.java:276)
at org.apache.drill.exec.store.bson.BsonRecordReader.writeBinary(BsonRecordReader.java:205)
at org.apache.drill.exec.store.bson.BsonRecordReader.writeToListOrMap(BsonRecordReader.java:117)
at org.apache.drill.exec.store.bson.BsonRecordReader.write(BsonRecordReader.java:75)
at org.apache.drill.exec.store.bson.TestBsonRecordReader.testBinaryTypes(TestBsonRecordReader.java:244)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.lang.reflect.Method.invoke(Method.java:498)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.lang.reflect.Method.invoke(Method.java:498)
at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
```
---
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1234#discussion_r183779660
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/bson/TestBsonRecordReader.java ---
@@ -49,17 +50,20 @@
import org.junit.BeforeClass;
import org.junit.Test;
-public class TestBsonRecordReader extends BaseTestQuery {
+public class TestBsonRecordReader {
+ private static BufferAllocator allocator;
private static VectorContainerWriter writer;
private static TestOutputMutator mutator;
+ private static DrillBuf buffer;
private static BsonRecordReader bsonReader;
@BeforeClass
public static void setUp() {
- BufferAllocator bufferAllocator = getDrillbitContext().getAllocator();
- mutator = new TestOutputMutator(bufferAllocator);
+ allocator = new RootAllocator(9_000_00);
--- End diff --
It does not sound right as there is not much data used in the test. I'd suggest filing a JIRA to look into why it takes 900 KB or almost 1 MB to hold data and using unlimited allocator (the same as before).
---
[GitHub] drill issue #1234: DRILL-5927: Fixed memory leak in TestBsonRecordReader, an...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on the issue:
https://github.com/apache/drill/pull/1234
@vrozov addressed comments
---
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1234
---
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/1234#discussion_r183608559
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/bson/TestBsonRecordReader.java ---
@@ -272,6 +276,9 @@ public static void cleanUp() {
} catch (Exception e) {
}
+
+ buffer.close();
--- End diff --
Currently close() just calls release(). I will update to use release(), but in general what is the best practice regarding when to call close() and when to call release()?
---
[GitHub] drill issue #1234: DRILL-5927: Fixed memory leak in TestBsonRecordReader, an...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on the issue:
https://github.com/apache/drill/pull/1234
@parthchandra Please let me know if you have any comments.
---
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1234#discussion_r183747252
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/bson/TestBsonRecordReader.java ---
@@ -272,6 +276,9 @@ public static void cleanUp() {
} catch (Exception e) {
}
+
+ buffer.close();
--- End diff --
IMO `close()` should be used within try-with-resources only (no explicit calls to `close()`).
---
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/1234#discussion_r183956964
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/bson/TestBsonRecordReader.java ---
@@ -45,21 +46,24 @@
import org.bson.BsonTimestamp;
import org.bson.BsonWriter;
import org.bson.types.ObjectId;
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
+import org.junit.After;
+import org.junit.Before;
import org.junit.Test;
-public class TestBsonRecordReader extends BaseTestQuery {
- private static VectorContainerWriter writer;
- private static TestOutputMutator mutator;
- private static BsonRecordReader bsonReader;
+public class TestBsonRecordReader {
+ private BufferAllocator allocator;
+ private VectorContainerWriter writer;
+ private TestOutputMutator mutator;
+ private DrillBuf buffer;
+ private BsonRecordReader bsonReader;
- @BeforeClass
- public static void setUp() {
- BufferAllocator bufferAllocator = getDrillbitContext().getAllocator();
- mutator = new TestOutputMutator(bufferAllocator);
+ @Before
+ public void setUp() {
+ allocator = new RootAllocator(400_000);
--- End diff --
@vrozov Made the changes
---
[GitHub] drill issue #1234: DRILL-5927: Fixed memory leak in TestBsonRecordReader, an...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1234
LGTM
---
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1234#discussion_r183564004
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/bson/TestBsonRecordReader.java ---
@@ -49,17 +50,20 @@
import org.junit.BeforeClass;
import org.junit.Test;
-public class TestBsonRecordReader extends BaseTestQuery {
+public class TestBsonRecordReader {
+ private static BufferAllocator allocator;
--- End diff --
What is a reason to define `BufferAllocator` and other fields as static? What if tests are executed in parallel?
---
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1234#discussion_r183563218
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/bson/TestBsonRecordReader.java ---
@@ -49,17 +50,20 @@
import org.junit.BeforeClass;
import org.junit.Test;
-public class TestBsonRecordReader extends BaseTestQuery {
+public class TestBsonRecordReader {
+ private static BufferAllocator allocator;
private static VectorContainerWriter writer;
private static TestOutputMutator mutator;
+ private static DrillBuf buffer;
private static BsonRecordReader bsonReader;
@BeforeClass
public static void setUp() {
- BufferAllocator bufferAllocator = getDrillbitContext().getAllocator();
- mutator = new TestOutputMutator(bufferAllocator);
+ allocator = new RootAllocator(100_000_000);
--- End diff --
Is it necessary to reserve 100MB for allocator? Does it allocate more than 1K?
---
[GitHub] drill issue #1234: DRILL-5927: Fixed memory leak in TestBsonRecordReader, an...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on the issue:
https://github.com/apache/drill/pull/1234
@vrozov addressed comments
---
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1234#discussion_r184549758
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/bson/TestBsonRecordReader.java ---
@@ -45,21 +47,24 @@
import org.bson.BsonTimestamp;
import org.bson.BsonWriter;
import org.bson.types.ObjectId;
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
+import org.junit.After;
+import org.junit.Before;
import org.junit.Test;
-public class TestBsonRecordReader extends BaseTestQuery {
- private static VectorContainerWriter writer;
- private static TestOutputMutator mutator;
- private static BsonRecordReader bsonReader;
+public class TestBsonRecordReader {
+ private BufferAllocator allocator;
+ private VectorContainerWriter writer;
+ private TestOutputMutator mutator;
+ private BufferManager bufferManager;
+ private BsonRecordReader bsonReader;
- @BeforeClass
- public static void setUp() {
- BufferAllocator bufferAllocator = getDrillbitContext().getAllocator();
- mutator = new TestOutputMutator(bufferAllocator);
+ @Before
+ public void setUp() {
+ allocator = new RootAllocator(Long.MAX_VALUE);
--- End diff --
Any reason why you had to change to RootAllocator? Should not really be necessary.
---
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1234#discussion_r183865249
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/bson/TestBsonRecordReader.java ---
@@ -45,21 +46,24 @@
import org.bson.BsonTimestamp;
import org.bson.BsonWriter;
import org.bson.types.ObjectId;
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
+import org.junit.After;
+import org.junit.Before;
import org.junit.Test;
-public class TestBsonRecordReader extends BaseTestQuery {
- private static VectorContainerWriter writer;
- private static TestOutputMutator mutator;
- private static BsonRecordReader bsonReader;
+public class TestBsonRecordReader {
+ private BufferAllocator allocator;
+ private VectorContainerWriter writer;
+ private TestOutputMutator mutator;
+ private DrillBuf buffer;
+ private BsonRecordReader bsonReader;
- @BeforeClass
- public static void setUp() {
- BufferAllocator bufferAllocator = getDrillbitContext().getAllocator();
- mutator = new TestOutputMutator(bufferAllocator);
+ @Before
+ public void setUp() {
+ allocator = new RootAllocator(400_000);
--- End diff --
Two minor suggestions:
- use `RootAllocator` with an unlimited amount of memory as limiting it to 400K does not test `BsonRecordReader` and depends more on how Vectors are allocated.
- instead of working with `DrillBuf` directly, use `BufferManager`.
---
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/1234#discussion_r183846385
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/bson/TestBsonRecordReader.java ---
@@ -49,17 +50,20 @@
import org.junit.BeforeClass;
import org.junit.Test;
-public class TestBsonRecordReader extends BaseTestQuery {
+public class TestBsonRecordReader {
+ private static BufferAllocator allocator;
--- End diff --
Initializing in Before and closing in After works. Changed the variables to be non static as well.
---
[GitHub] drill pull request #1234: DRILL-5927: Fixed memory leak in TestBsonRecordRea...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/1234#discussion_r183846152
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/bson/TestBsonRecordReader.java ---
@@ -49,17 +50,20 @@
import org.junit.BeforeClass;
import org.junit.Test;
-public class TestBsonRecordReader extends BaseTestQuery {
+public class TestBsonRecordReader {
+ private static BufferAllocator allocator;
private static VectorContainerWriter writer;
private static TestOutputMutator mutator;
+ private static DrillBuf buffer;
private static BsonRecordReader bsonReader;
@BeforeClass
public static void setUp() {
- BufferAllocator bufferAllocator = getDrillbitContext().getAllocator();
- mutator = new TestOutputMutator(bufferAllocator);
+ allocator = new RootAllocator(9_000_00);
--- End diff --
After addressing review comments and recompiling I observed different behavoir. Now the tests pass with 400kb on the root allocator. But reducing it to 300kb causes an IOB. I have filed an issue here https://issues.apache.org/jira/browse/DRILL-6352
---