You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by abdelhakim deneche <ad...@gmail.com> on 2015/05/21 14:29:07 UTC
Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73 causes
memory leak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/
-----------------------------------------------------------
Review request for drill, Chris Westin and Jacques Nadeau.
Bugs: DRILL-3147
https://issues.apache.org/jira/browse/DRILL-3147
Repository: drill-git
Description
-------
- FragmentContext.close() waits 100ms before closing the allocator to give enough time to the rpc layer to properly release any batch that was just transfered to this fragment's allocator
- each time a fragment A sends a "receiver finished" to fragment B, fragment B id will be added to FragmentContext.ignoredSenders list
- refactored UnorderedReceiverBatch.informSenders() and MergingRecordBatch.informSenders() by moving this method to FragmentContext
- DataServer.send() uses FragmentContext.ignoredSenders to decide if a batch should be passed to the fragment or discarded right away
- BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
- TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 1cbe886
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java baf9bda
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 684f715
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
Diff: https://reviews.apache.org/r/34541/diff/
Testing
-------
unit tests and tpch100 are passing. Couldn't run functional yet
Thanks,
abdelhakim deneche
Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73
causes memory leak
Posted by abdelhakim deneche <ad...@gmail.com>.
> On May 21, 2015, 5:08 p.m., Sudheesh Katkam wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java, line 417
> > <https://reviews.apache.org/r/34541/diff/2/?file=966656#file966656line417>
> >
> > Is there an alternative implementation? I feel like this method should not be part of FragmentContext.
we don't need this change anymore.
> On May 21, 2015, 5:08 p.m., Sudheesh Katkam wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java, line 68
> > <https://reviews.apache.org/r/34541/diff/2/?file=966661#file966661line68>
> >
> > Unnecessary "synchronized"?
This was done as part of my trials to fix this leak. I will probably revert it back
- abdelhakim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/#review84727
-----------------------------------------------------------
On May 21, 2015, 7:34 p.m., abdelhakim deneche wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34541/
> -----------------------------------------------------------
>
> (Updated May 21, 2015, 7:34 p.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Bugs: DRILL-3147
> https://issues.apache.org/jira/browse/DRILL-3147
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
> - TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34541/diff/
>
>
> Testing
> -------
>
> still need to run the tests!
>
>
> Thanks,
>
> abdelhakim deneche
>
>
Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73
causes memory leak
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/#review84727
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java
<https://reviews.apache.org/r/34541/#comment136065>
Is there an alternative implementation? I feel like this method should not be part of FragmentContext.
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java
<https://reviews.apache.org/r/34541/#comment136067>
Why a list? .contains(...) is linear time.
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java
<https://reviews.apache.org/r/34541/#comment136071>
Unnecessary "synchronized"?
- Sudheesh Katkam
On May 21, 2015, 4:30 p.m., abdelhakim deneche wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34541/
> -----------------------------------------------------------
>
> (Updated May 21, 2015, 4:30 p.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Bugs: DRILL-3147
> https://issues.apache.org/jira/browse/DRILL-3147
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - FragmentContext.close() waits 100ms before closing the allocator to give enough time to the rpc layer to properly release any batch that was just transfered to this fragment's allocator
> - each time a fragment A sends a "receiver finished" to fragment B, fragment B id will be added to FragmentContext.ignoredSenders list
> - refactored UnorderedReceiverBatch.informSenders() and MergingRecordBatch.informSenders() by moving this method to FragmentContext
> - DataServer.send() uses FragmentContext.ignoredSenders to decide if a batch should be passed to the fragment or discarded right away
> - BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
> - TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 1cbe886
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java baf9bda
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 684f715
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34541/diff/
>
>
> Testing
> -------
>
> unit tests and tpch100 are passing. Couldn't run functional yet
>
>
> Thanks,
>
> abdelhakim deneche
>
>
Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73
causes memory leak
Posted by abdelhakim deneche <ad...@gmail.com>.
> On May 21, 2015, 8:18 p.m., Steven Phillips wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java, line 65
> > <https://reviews.apache.org/r/34541/diff/3/?file=966937#file966937line65>
> >
> > There is no need for this to be synchronized. What is you reason for making this change?
> >
> > All access to this method happens through a single thread.
If I'm not mistaken, DataServer.send() will pass a buffer to BaseRawBatchBuffer.enqueue() through FragmentManager.handle(), and BaseRawBatchBuffer.kill() will be called from the FragmentExecutor's thread. Those are different threads right ?
- abdelhakim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/#review84779
-----------------------------------------------------------
On May 21, 2015, 7:34 p.m., abdelhakim deneche wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34541/
> -----------------------------------------------------------
>
> (Updated May 21, 2015, 7:34 p.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Bugs: DRILL-3147
> https://issues.apache.org/jira/browse/DRILL-3147
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
> - TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34541/diff/
>
>
> Testing
> -------
>
> still need to run the tests!
>
>
> Thanks,
>
> abdelhakim deneche
>
>
Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73
causes memory leak
Posted by Steven Phillips <sp...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/#review84779
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java
<https://reviews.apache.org/r/34541/#comment136135>
There is no need for this to be synchronized. What is you reason for making this change?
All access to this method happens through a single thread.
- Steven Phillips
On May 21, 2015, 7:34 p.m., abdelhakim deneche wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34541/
> -----------------------------------------------------------
>
> (Updated May 21, 2015, 7:34 p.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Bugs: DRILL-3147
> https://issues.apache.org/jira/browse/DRILL-3147
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
> - TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34541/diff/
>
>
> Testing
> -------
>
> still need to run the tests!
>
>
> Thanks,
>
> abdelhakim deneche
>
>
Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73
causes memory leak
Posted by abdelhakim deneche <ad...@gmail.com>.
> On May 26, 2015, 6:41 p.m., Sudheesh Katkam wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java, line 53
> > <https://reviews.apache.org/r/34541/diff/4/?file=970092#file970092line53>
> >
> > Do we require a @AfterClass method to change the cluster size back to initial? Or does that not matter?
BaseTestQuery.resetDrillbitCount() is an @AfterClass method that takes care of it
> On May 26, 2015, 6:41 p.m., Sudheesh Katkam wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java, line 62
> > <https://reviews.apache.org/r/34541/diff/4/?file=970092#file970092line62>
> >
> > The test framework already prints out the name of the test. Is this addtional print necessary?
the name will only be displayed once. Printing the iteration # gives a sense of progresssion, especially when repeating the test 1000 times
- abdelhakim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/#review85209
-----------------------------------------------------------
On May 22, 2015, 11:15 p.m., abdelhakim deneche wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34541/
> -----------------------------------------------------------
>
> (Updated May 22, 2015, 11:15 p.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Bugs: DRILL-3147
> https://issues.apache.org/jira/browse/DRILL-3147
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
> - TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java a07f621
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
> exec/java-exec/src/test/resources/tpcds-sf1/q73.sql PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34541/diff/
>
>
> Testing
> -------
>
> still need to run the tests!
>
>
> Thanks,
>
> abdelhakim deneche
>
>
Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73
causes memory leak
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/#review85209
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java
<https://reviews.apache.org/r/34541/#comment136722>
unused import?
exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java
<https://reviews.apache.org/r/34541/#comment136732>
final
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java
<https://reviews.apache.org/r/34541/#comment136730>
Do we require a @AfterClass method to change the cluster size back to initial? Or does that not matter?
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java
<https://reviews.apache.org/r/34541/#comment136728>
The test framework already prints out the name of the test. Is this addtional print necessary?
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java
<https://reviews.apache.org/r/34541/#comment136729>
final
- Sudheesh Katkam
On May 22, 2015, 11:15 p.m., abdelhakim deneche wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34541/
> -----------------------------------------------------------
>
> (Updated May 22, 2015, 11:15 p.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Bugs: DRILL-3147
> https://issues.apache.org/jira/browse/DRILL-3147
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
> - TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java a07f621
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
> exec/java-exec/src/test/resources/tpcds-sf1/q73.sql PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34541/diff/
>
>
> Testing
> -------
>
> still need to run the tests!
>
>
> Thanks,
>
> abdelhakim deneche
>
>
Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73
causes memory leak
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/#review85205
-----------------------------------------------------------
Ship it (non-binding)
- Chris Westin
On May 22, 2015, 4:15 p.m., abdelhakim deneche wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34541/
> -----------------------------------------------------------
>
> (Updated May 22, 2015, 4:15 p.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Bugs: DRILL-3147
> https://issues.apache.org/jira/browse/DRILL-3147
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
> - TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java a07f621
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
> exec/java-exec/src/test/resources/tpcds-sf1/q73.sql PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34541/diff/
>
>
> Testing
> -------
>
> still need to run the tests!
>
>
> Thanks,
>
> abdelhakim deneche
>
>
Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73
causes memory leak
Posted by Jason Altekruse <al...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/#review85986
-----------------------------------------------------------
Ship it!
Ship It!
- Jason Altekruse
On May 27, 2015, 2:19 p.m., abdelhakim deneche wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34541/
> -----------------------------------------------------------
>
> (Updated May 27, 2015, 2:19 p.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Bugs: DRILL-3147
> https://issues.apache.org/jira/browse/DRILL-3147
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
> - TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java a07f621
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
> exec/java-exec/src/test/resources/tpcds-sf1/q73.sql PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34541/diff/
>
>
> Testing
> -------
>
> still need to run the tests!
>
>
> Thanks,
>
> abdelhakim deneche
>
>
Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73
causes memory leak
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/#review85997
-----------------------------------------------------------
Ship it (non-binding)
- Sudheesh Katkam
On May 27, 2015, 2:19 p.m., abdelhakim deneche wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34541/
> -----------------------------------------------------------
>
> (Updated May 27, 2015, 2:19 p.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Bugs: DRILL-3147
> https://issues.apache.org/jira/browse/DRILL-3147
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
> - TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java a07f621
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
> exec/java-exec/src/test/resources/tpcds-sf1/q73.sql PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34541/diff/
>
>
> Testing
> -------
>
> still need to run the tests!
>
>
> Thanks,
>
> abdelhakim deneche
>
>
Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73
causes memory leak
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/
-----------------------------------------------------------
(Updated May 27, 2015, 2:19 p.m.)
Review request for drill, Chris Westin and Jacques Nadeau.
Bugs: DRILL-3147
https://issues.apache.org/jira/browse/DRILL-3147
Repository: drill-git
Description
-------
- BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
- TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java a07f621
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
exec/java-exec/src/test/resources/tpcds-sf1/q73.sql PRE-CREATION
Diff: https://reviews.apache.org/r/34541/diff/
Testing
-------
still need to run the tests!
Thanks,
abdelhakim deneche
Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73
causes memory leak
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/
-----------------------------------------------------------
(Updated May 22, 2015, 11:15 p.m.)
Review request for drill, Chris Westin and Jacques Nadeau.
Changes
-------
adressed review comments
Bugs: DRILL-3147
https://issues.apache.org/jira/browse/DRILL-3147
Repository: drill-git
Description
-------
- BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
- TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java a07f621
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
exec/java-exec/src/test/resources/tpcds-sf1/q73.sql PRE-CREATION
Diff: https://reviews.apache.org/r/34541/diff/
Testing
-------
still need to run the tests!
Thanks,
abdelhakim deneche
Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73
causes memory leak
Posted by Chris Westin <ch...@gmail.com>.
> On May 22, 2015, 11:15 a.m., Chris Westin wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java, line 56
> > <https://reviews.apache.org/r/34541/diff/3/?file=966939#file966939line56>
> >
> > Can we indent this a little better so it's easier to see the nested select separately from the rest of the query, as well as the order by and group by clauses? It might help to condense the select lists onto a single line. For example
> >
> > select .....,
> > more columns, if necessary
> > from ...
> > more of that, if necessary
> > where ...
> > group by ...
> > order by ...
> > limit ...
>
> Sudheesh Katkam wrote:
> You could put the query in a file and use BaseTestQuery.getFile(...)
> For example:
> ```
> final String query = BaseTestQuery.getFile("queries/tpch/09.sql");
> ```
Oh yeah, I like that even better, because then you can format the query nicely without having a bunch of Java String muck (+, etc) in the way.
- Chris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/#review84943
-----------------------------------------------------------
On May 21, 2015, 12:34 p.m., abdelhakim deneche wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34541/
> -----------------------------------------------------------
>
> (Updated May 21, 2015, 12:34 p.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Bugs: DRILL-3147
> https://issues.apache.org/jira/browse/DRILL-3147
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
> - TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34541/diff/
>
>
> Testing
> -------
>
> still need to run the tests!
>
>
> Thanks,
>
> abdelhakim deneche
>
>
Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73
causes memory leak
Posted by Sudheesh Katkam <sk...@maprtech.com>.
> On May 22, 2015, 6:15 p.m., Chris Westin wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java, line 56
> > <https://reviews.apache.org/r/34541/diff/3/?file=966939#file966939line56>
> >
> > Can we indent this a little better so it's easier to see the nested select separately from the rest of the query, as well as the order by and group by clauses? It might help to condense the select lists onto a single line. For example
> >
> > select .....,
> > more columns, if necessary
> > from ...
> > more of that, if necessary
> > where ...
> > group by ...
> > order by ...
> > limit ...
You could put the query in a file and use BaseTestQuery.getFile(...)
For example:
```
final String query = BaseTestQuery.getFile("queries/tpch/09.sql");
```
- Sudheesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/#review84943
-----------------------------------------------------------
On May 21, 2015, 7:34 p.m., abdelhakim deneche wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34541/
> -----------------------------------------------------------
>
> (Updated May 21, 2015, 7:34 p.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Bugs: DRILL-3147
> https://issues.apache.org/jira/browse/DRILL-3147
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
> - TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34541/diff/
>
>
> Testing
> -------
>
> still need to run the tests!
>
>
> Thanks,
>
> abdelhakim deneche
>
>
Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73
causes memory leak
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/#review84943
-----------------------------------------------------------
Steven knows much more about this area of the code than I do, so my comments are mostly stylistic, with one exception.
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java
<https://reviews.apache.org/r/34541/#comment136395>
Can we make this final?
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java
<https://reviews.apache.org/r/34541/#comment136399>
Nothing else in this file is synchronized, so what is this protecting?
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java
<https://reviews.apache.org/r/34541/#comment136401>
final
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java
<https://reviews.apache.org/r/34541/#comment136402>
Can we indent this a little better so it's easier to see the nested select separately from the rest of the query, as well as the order by and group by clauses? It might help to condense the select lists onto a single line. For example
select .....,
more columns, if necessary
from ...
more of that, if necessary
where ...
group by ...
order by ...
limit ...
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java
<https://reviews.apache.org/r/34541/#comment136403>
Spaces around the + operator, please.
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java
<https://reviews.apache.org/r/34541/#comment136404>
Can we move this to BaseTestQuery and make it protected? Other tests could use this.
- Chris Westin
On May 21, 2015, 12:34 p.m., abdelhakim deneche wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34541/
> -----------------------------------------------------------
>
> (Updated May 21, 2015, 12:34 p.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Bugs: DRILL-3147
> https://issues.apache.org/jira/browse/DRILL-3147
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
> - TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34541/diff/
>
>
> Testing
> -------
>
> still need to run the tests!
>
>
> Thanks,
>
> abdelhakim deneche
>
>
Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73
causes memory leak
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/
-----------------------------------------------------------
(Updated May 21, 2015, 7:34 p.m.)
Review request for drill, Chris Westin and Jacques Nadeau.
Changes
-------
reverted back the changes to FragmentContext. We don't actually need to check if the sender is "ignored", now that the rpc layer don't hold references to the buffer, the fragment will properly handle the buffer
Bugs: DRILL-3147
https://issues.apache.org/jira/browse/DRILL-3147
Repository: drill-git
Description (updated)
-------
- BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
- TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
Diff: https://reviews.apache.org/r/34541/diff/
Testing (updated)
-------
still need to run the tests!
Thanks,
abdelhakim deneche
Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73
causes memory leak
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/
-----------------------------------------------------------
(Updated May 21, 2015, 4:30 p.m.)
Review request for drill, Chris Westin and Jacques Nadeau.
Changes
-------
When DataServer.send() passes the ownership of the buffer to the fragment's allocator it will reset the reference count.
FragmentContext.close() no longer needs to wait for the rpc layer to release the buffer
Bugs: DRILL-3147
https://issues.apache.org/jira/browse/DRILL-3147
Repository: drill-git
Description
-------
- FragmentContext.close() waits 100ms before closing the allocator to give enough time to the rpc layer to properly release any batch that was just transfered to this fragment's allocator
- each time a fragment A sends a "receiver finished" to fragment B, fragment B id will be added to FragmentContext.ignoredSenders list
- refactored UnorderedReceiverBatch.informSenders() and MergingRecordBatch.informSenders() by moving this method to FragmentContext
- DataServer.send() uses FragmentContext.ignoredSenders to decide if a batch should be passed to the fragment or discarded right away
- BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
- TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because it requires a large dataset
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 1cbe886
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java baf9bda
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 684f715
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java 11b6cc8
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b770a33
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION
Diff: https://reviews.apache.org/r/34541/diff/
Testing
-------
unit tests and tpch100 are passing. Couldn't run functional yet
Thanks,
abdelhakim deneche