You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Yuliya Feldman <yu...@yahoo.com> on 2015/02/17 09:31:17 UTC
Review Request 31107: Ability to make PartitionSender multithreaded -
useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/
-----------------------------------------------------------
Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
Bugs: DRILL-2210
https://issues.apache.org/jira/browse/DRILL-2210
Repository: drill-git
Description
-------
In addition to description
Fixed few classes that did not handle multithreading well
Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java f09acaa
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 4292c09
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java faa8546
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java aa0a5ad
Diff: https://reviews.apache.org/r/31107/diff/
Testing
-------
Still need to provide Unit Tests.
Functional tests are passing
Performance tests were run and look promising for some queries
Thanks,
Yuliya Feldman
Re: Review Request 31107: Ability to make PartitionSender
multithreaded - useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/#review73593
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
<https://reviews.apache.org/r/31107/#comment119992>
You could use Thread.currentThread() once here, and then reuse.
- Chris Westin
On Feb. 20, 2015, 5:39 p.m., Yuliya Feldman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31107/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2015, 5:39 p.m.)
>
>
> Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
>
>
> Bugs: DRILL-2210
> https://issues.apache.org/jira/browse/DRILL-2210
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> In addition to description
>
> Fixed few classes that did not handle multithreading well
> Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
> Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java f09acaa
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 4292c09
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java faa8546
> exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java aa0a5ad
>
> Diff: https://reviews.apache.org/r/31107/diff/
>
>
> Testing
> -------
>
> Still need to provide Unit Tests.
>
> Functional tests are passing
>
> Performance tests were run and look promising for some queries
>
>
> Thanks,
>
> Yuliya Feldman
>
>
Re: Review Request 31107: Ability to make PartitionSender
multithreaded - useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/#review75779
-----------------------------------------------------------
Just a few minor suggestions.
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/partitionsender/TestPartitionSender.java
<https://reviews.apache.org/r/31107/#comment123054>
Can divisor and longTail be final?
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/partitionsender/TestPartitionSender.java
<https://reviews.apache.org/r/31107/#comment123055>
No need for a local, just throw this.
- Chris Westin
On March 9, 2015, 9:31 a.m., Yuliya Feldman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31107/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 9:31 a.m.)
>
>
> Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
>
>
> Bugs: DRILL-2210
> https://issues.apache.org/jira/browse/DRILL-2210
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> In addition to description
>
> Fixed few classes that did not handle multithreading well
> Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
> Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/CodeCompiler.java 7cc350e
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java e413921
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/IteratorValidator.java 64cf7c5
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java a23bd7a
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 71ffd41
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Materializer.java 961b603
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java bbfbbcb
> exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 0fb10ff
> exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java 3d3e96f
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 99c6ab8
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java 4aaaa78
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/partitionsender/TestPartitionSender.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31107/diff/
>
>
> Testing
> -------
>
> Still need to provide Unit Tests.
>
> Functional tests are passing
>
> Performance tests were run and look promising for some queries
>
>
> Thanks,
>
> Yuliya Feldman
>
>
Re: Review Request 31107: Ability to make PartitionSender
multithreaded - useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
Posted by Yuliya Feldman <yu...@gmail.com>.
> On March 15, 2015, 1:28 p.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java, line 85
> > <https://reviews.apache.org/r/31107/diff/7/?file=889340#file889340line85>
> >
> > returning null here seems weird. When would that happen?
Since there can be > 1 partitioners current partitioner may not correspond to the index. There is a method in PartitionerDecorator that loops over partitioners and return one that matches the index. I will add comments to the method.
> On March 15, 2015, 1:28 p.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java, line 234
> > <https://reviews.apache.org/r/31107/diff/7/?file=889333#file889333line234>
> >
> > shouldn't this parameter be instanceCount? instanceNumber seems like an index.
will do
> On March 15, 2015, 1:28 p.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java, line 70
> > <https://reviews.apache.org/r/31107/diff/7/?file=889334#file889334line70>
> >
> > missing description of what isClean means.
will do
- Yuliya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/#review76510
-----------------------------------------------------------
On March 9, 2015, 9:31 a.m., Yuliya Feldman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31107/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 9:31 a.m.)
>
>
> Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
>
>
> Bugs: DRILL-2210
> https://issues.apache.org/jira/browse/DRILL-2210
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> In addition to description
>
> Fixed few classes that did not handle multithreading well
> Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
> Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/CodeCompiler.java 7cc350e
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java e413921
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/IteratorValidator.java 64cf7c5
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java a23bd7a
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 71ffd41
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Materializer.java 961b603
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java bbfbbcb
> exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 0fb10ff
> exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java 3d3e96f
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 99c6ab8
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java 4aaaa78
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/partitionsender/TestPartitionSender.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31107/diff/
>
>
> Testing
> -------
>
> Still need to provide Unit Tests.
>
> Functional tests are passing
>
> Performance tests were run and look promising for some queries
>
>
> Thanks,
>
> Yuliya Feldman
>
>
Re: Review Request 31107: Ability to make PartitionSender
multithreaded - useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/#review76510
-----------------------------------------------------------
Ship it!
Few little comments. Otherwise, looks good to ship.
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java
<https://reviews.apache.org/r/31107/#comment124062>
shouldn't this parameter be instanceCount? instanceNumber seems like an index.
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java
<https://reviews.apache.org/r/31107/#comment124063>
missing description of what isClean means.
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java
<https://reviews.apache.org/r/31107/#comment124064>
returning null here seems weird. When would that happen?
- Jacques Nadeau
On March 9, 2015, 4:31 p.m., Yuliya Feldman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31107/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 4:31 p.m.)
>
>
> Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
>
>
> Bugs: DRILL-2210
> https://issues.apache.org/jira/browse/DRILL-2210
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> In addition to description
>
> Fixed few classes that did not handle multithreading well
> Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
> Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/CodeCompiler.java 7cc350e
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java e413921
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/IteratorValidator.java 64cf7c5
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java a23bd7a
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 71ffd41
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Materializer.java 961b603
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java bbfbbcb
> exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 0fb10ff
> exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java 3d3e96f
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 99c6ab8
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java 4aaaa78
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/partitionsender/TestPartitionSender.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31107/diff/
>
>
> Testing
> -------
>
> Still need to provide Unit Tests.
>
> Functional tests are passing
>
> Performance tests were run and look promising for some queries
>
>
> Thanks,
>
> Yuliya Feldman
>
>
Re: Review Request 31107: Ability to make PartitionSender
multithreaded - useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/#review76711
-----------------------------------------------------------
Ship it!
Ship It!
- Jacques Nadeau
On March 17, 2015, 4:17 a.m., Yuliya Feldman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31107/
> -----------------------------------------------------------
>
> (Updated March 17, 2015, 4:17 a.m.)
>
>
> Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
>
>
> Bugs: DRILL-2210
> https://issues.apache.org/jira/browse/DRILL-2210
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> In addition to description
>
> Fixed few classes that did not handle multithreading well
> Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
> Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/CodeCompiler.java 7cc350e
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 108f5bb
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/IteratorValidator.java 64cf7c5
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java ccbd289
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 1d9088a
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Materializer.java 9b0944e
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java abbc910
> exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 0fb10ff
> exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java d821af8
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 99c6ab8
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java 4aaaa78
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/partitionsender/TestPartitionSender.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31107/diff/
>
>
> Testing
> -------
>
> Still need to provide Unit Tests.
>
> Functional tests are passing
>
> Performance tests were run and look promising for some queries
>
>
> Thanks,
>
> Yuliya Feldman
>
>
Re: Review Request 31107: Ability to make PartitionSender
multithreaded - useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
Posted by Yuliya Feldman <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/
-----------------------------------------------------------
(Updated March 16, 2015, 9:17 p.m.)
Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
Changes
-------
Latest (hopefully)
Addressing review comments
changed default settings for threads
added cost to metrics
added additional param to completely overwrite # of threads
Bugs: DRILL-2210
https://issues.apache.org/jira/browse/DRILL-2210
Repository: drill-git
Description
-------
In addition to description
Fixed few classes that did not handle multithreading well
Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/compile/CodeCompiler.java 7cc350e
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 108f5bb
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/IteratorValidator.java 64cf7c5
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java ccbd289
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 1d9088a
exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Materializer.java 9b0944e
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java abbc910
exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 0fb10ff
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java d821af8
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 99c6ab8
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java 4aaaa78
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/partitionsender/TestPartitionSender.java PRE-CREATION
Diff: https://reviews.apache.org/r/31107/diff/
Testing
-------
Still need to provide Unit Tests.
Functional tests are passing
Performance tests were run and look promising for some queries
Thanks,
Yuliya Feldman
Re: Review Request 31107: Ability to make PartitionSender
multithreaded - useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
Posted by Yuliya Feldman <yu...@gmail.com>.
> On March 15, 2015, 8:18 p.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java, line 85
> > <https://reviews.apache.org/r/31107/diff/7/?file=889340#file889340line85>
> >
> > Just to confirm, this will only be called in rare cases, right? E.g. not for every record or even every batch.
it is used only by the method you have your comment on efficiency which is used only by receivingFragmentFinished()
> On March 15, 2015, 8:18 p.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java, line 106
> > <https://reviews.apache.org/r/31107/diff/7/?file=889339#file889339line106>
> >
> > I really think you should make this logic more efficient. Why not just have a direct array of outgoing batches by index?
I am not sure what exactly you mean, but I think it is a confusion with name of the method - it should have been called getOutgoingBatch (not getOutgoingBatches) - decorator for one in PartitionerTemplate. Since now we have list of partitioners and each of them has list of outgingbatches we need to get right partitioner first.
Corresponding method in PartitionerTemplate may not be needed - since all the logic can be done inside this one. To get precise Partitioner based just on the index may not be always precise because I am trying to balance it out and so some partitioners will have an extra OutgoingBatch - like partitioner0 has batches[1,2], partitioner1 has batches[3] - if I have 2 threads/partitioners and just 3 destinations/outgoing batches.
Let's discuss if I still miss something
- Yuliya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/#review76516
-----------------------------------------------------------
On March 9, 2015, 9:31 a.m., Yuliya Feldman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31107/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 9:31 a.m.)
>
>
> Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
>
>
> Bugs: DRILL-2210
> https://issues.apache.org/jira/browse/DRILL-2210
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> In addition to description
>
> Fixed few classes that did not handle multithreading well
> Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
> Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/CodeCompiler.java 7cc350e
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java e413921
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/IteratorValidator.java 64cf7c5
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java a23bd7a
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 71ffd41
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Materializer.java 961b603
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java bbfbbcb
> exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 0fb10ff
> exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java 3d3e96f
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 99c6ab8
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java 4aaaa78
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/partitionsender/TestPartitionSender.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31107/diff/
>
>
> Testing
> -------
>
> Still need to provide Unit Tests.
>
> Functional tests are passing
>
> Performance tests were run and look promising for some queries
>
>
> Thanks,
>
> Yuliya Feldman
>
>
Re: Review Request 31107: Ability to make PartitionSender
multithreaded - useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/#review76516
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
<https://reviews.apache.org/r/31107/#comment124080>
I really think you should make this logic more efficient. Why not just have a direct array of outgoing batches by index?
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java
<https://reviews.apache.org/r/31107/#comment124081>
Just to confirm, this will only be called in rare cases, right? E.g. not for every record or even every batch.
- Jacques Nadeau
On March 9, 2015, 4:31 p.m., Yuliya Feldman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31107/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 4:31 p.m.)
>
>
> Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
>
>
> Bugs: DRILL-2210
> https://issues.apache.org/jira/browse/DRILL-2210
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> In addition to description
>
> Fixed few classes that did not handle multithreading well
> Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
> Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/compile/CodeCompiler.java 7cc350e
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java e413921
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/IteratorValidator.java 64cf7c5
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java a23bd7a
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 71ffd41
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Materializer.java 961b603
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java bbfbbcb
> exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 0fb10ff
> exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java 3d3e96f
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 99c6ab8
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java 4aaaa78
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/partitionsender/TestPartitionSender.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31107/diff/
>
>
> Testing
> -------
>
> Still need to provide Unit Tests.
>
> Functional tests are passing
>
> Performance tests were run and look promising for some queries
>
>
> Thanks,
>
> Yuliya Feldman
>
>
Re: Review Request 31107: Ability to make PartitionSender
multithreaded - useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
Posted by Yuliya Feldman <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/
-----------------------------------------------------------
(Updated March 9, 2015, 9:31 a.m.)
Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
Changes
-------
More unit tests for failure scenarios
Optimized more partitioners distribution algorithm
Fixed OperatorStats merging metrics
Bugs: DRILL-2210
https://issues.apache.org/jira/browse/DRILL-2210
Repository: drill-git
Description
-------
In addition to description
Fixed few classes that did not handle multithreading well
Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/compile/CodeCompiler.java 7cc350e
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java e413921
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/IteratorValidator.java 64cf7c5
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java a23bd7a
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 71ffd41
exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Materializer.java 961b603
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java bbfbbcb
exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 0fb10ff
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java 3d3e96f
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 99c6ab8
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java 4aaaa78
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/partitionsender/TestPartitionSender.java PRE-CREATION
Diff: https://reviews.apache.org/r/31107/diff/
Testing
-------
Still need to provide Unit Tests.
Functional tests are passing
Performance tests were run and look promising for some queries
Thanks,
Yuliya Feldman
Re: Review Request 31107: Ability to make PartitionSender
multithreaded - useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
Posted by Yuliya Feldman <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/
-----------------------------------------------------------
(Updated March 6, 2015, 2:59 p.m.)
Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
Changes
-------
Added Unit tests
Added number of threads as a metric to PartitionSender operator
Fixed partitioners distribution algorithm to do even distribution between threads
Bugs: DRILL-2210
https://issues.apache.org/jira/browse/DRILL-2210
Repository: drill-git
Description
-------
In addition to description
Fixed few classes that did not handle multithreading well
Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/compile/CodeCompiler.java 7cc350e
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java e413921
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/IteratorValidator.java 64cf7c5
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java a23bd7a
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 71ffd41
exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Materializer.java 961b603
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java bbfbbcb
exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 0fb10ff
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java 3d3e96f
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 99c6ab8
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java 4aaaa78
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/partitionsender/TestPartitionSender.java PRE-CREATION
Diff: https://reviews.apache.org/r/31107/diff/
Testing
-------
Still need to provide Unit Tests.
Functional tests are passing
Performance tests were run and look promising for some queries
Thanks,
Yuliya Feldman
Re: Review Request 31107: Ability to make PartitionSender
multithreaded - useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
Posted by Yuliya Feldman <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/
-----------------------------------------------------------
(Updated Feb. 26, 2015, 7:19 p.m.)
Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
Changes
-------
Added calculation of number of threads based on the cost (number of rows).
Formula is: cost/slicetarget/#senders/threadfactor
threadfactor is set to 4 by default
Additional config param is max number of threads - by default set to 32
Will need to play around with those params to figure out good combination
Bugs: DRILL-2210
https://issues.apache.org/jira/browse/DRILL-2210
Repository: drill-git
Description
-------
In addition to description
Fixed few classes that did not handle multithreading well
Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/compile/CodeCompiler.java 7cc350e
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java e413921
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/IteratorValidator.java 64cf7c5
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java f09acaa
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 4292c09
exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Materializer.java 961b603
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java faa8546
exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 83a89df
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java aa0a5ad
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 99c6ab8
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java 4aaaa78
Diff: https://reviews.apache.org/r/31107/diff/
Testing
-------
Still need to provide Unit Tests.
Functional tests are passing
Performance tests were run and look promising for some queries
Thanks,
Yuliya Feldman
Re: Review Request 31107: Ability to make PartitionSender
multithreaded - useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
Posted by Yuliya Feldman <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/
-----------------------------------------------------------
(Updated Feb. 25, 2015, 11:24 p.m.)
Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
Changes
-------
Changes based on latest code review with Jacques and Venki
1. Reuse of ExecutorService from WorkManager
2. Not using anonymous objects
3. Not using callables in favor of runnables
4. other small corrections
Bugs: DRILL-2210
https://issues.apache.org/jira/browse/DRILL-2210
Repository: drill-git
Description
-------
In addition to description
Fixed few classes that did not handle multithreading well
Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/compile/CodeCompiler.java 7cc350e
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java e413921
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java f09acaa
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 4292c09
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java faa8546
exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 83a89df
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java aa0a5ad
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 99c6ab8
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java 4aaaa78
Diff: https://reviews.apache.org/r/31107/diff/
Testing
-------
Still need to provide Unit Tests.
Functional tests are passing
Performance tests were run and look promising for some queries
Thanks,
Yuliya Feldman
Re: Review Request 31107: Ability to make PartitionSender
multithreaded - useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
Posted by Yuliya Feldman <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/
-----------------------------------------------------------
(Updated Feb. 23, 2015, 3:29 p.m.)
Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
Changes
-------
Addressing review comments
Bugs: DRILL-2210
https://issues.apache.org/jira/browse/DRILL-2210
Repository: drill-git
Description
-------
In addition to description
Fixed few classes that did not handle multithreading well
Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java f09acaa
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 4292c09
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java faa8546
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java aa0a5ad
Diff: https://reviews.apache.org/r/31107/diff/
Testing
-------
Still need to provide Unit Tests.
Functional tests are passing
Performance tests were run and look promising for some queries
Thanks,
Yuliya Feldman
Re: Review Request 31107: Ability to make PartitionSender
multithreaded - useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
Posted by Yuliya Feldman <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/
-----------------------------------------------------------
(Updated Feb. 20, 2015, 5:39 p.m.)
Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
Changes
-------
Changes based on first round of code review
Bugs: DRILL-2210
https://issues.apache.org/jira/browse/DRILL-2210
Repository: drill-git
Description
-------
In addition to description
Fixed few classes that did not handle multithreading well
Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java f09acaa
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 4292c09
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java faa8546
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java aa0a5ad
Diff: https://reviews.apache.org/r/31107/diff/
Testing
-------
Still need to provide Unit Tests.
Functional tests are passing
Performance tests were run and look promising for some queries
Thanks,
Yuliya Feldman
Re: Review Request 31107: Ability to make PartitionSender
multithreaded - useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
Posted by Yuliya Feldman <yu...@yahoo.com>.
> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> >
Thank you very much Chris for review - see my comments to your comments
> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java, line 69
> > <https://reviews.apache.org/r/31107/diff/1/?file=866190#file866190line69>
> >
> > Why not call this(original, false) to avoid duplicating the code?
oops - forgot to remove this method - will do
> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java, line 115
> > <https://reviews.apache.org/r/31107/diff/1/?file=866190#file866190line115>
> >
> > how about
> >
> > final IntLongOpenHashmap fromMetrics = from.longMetrics;
> > final long[] fromValues = fromMetrics.values;
> > for(int i : fromMetrics.keys) {
> > final long value = fromValues[i];
> > longMetrics.putOrAdd(i, value, value);
> > }
> >
> > This avoids multiple evaluation of the member accesses on every pass of the loop, and multiple bounds checks within the loop body.
> >
> > Similar improvements can be made to the doubleMetrics loop below.
OK
> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java, line 32
> > <https://reviews.apache.org/r/31107/diff/1/?file=866191#file866191line32>
> >
> > I'd make these final too, while you're here.
OK
> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java, line 286
> > <https://reviews.apache.org/r/31107/diff/1/?file=866192#file866192line286>
> >
> > Reuse the id variable from above (which looks like it should be made final).
yes - definitely
> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java, line 46
> > <https://reviews.apache.org/r/31107/diff/1/?file=866194#file866194line46>
> >
> > Could this be final?
Don't see a reason why not, but will double check
> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java, line 164
> > <https://reviews.apache.org/r/31107/diff/1/?file=866194#file866194line164>
> >
> > Call Thread.currentThread() once at the beginning, and then reuse.
It is a beginnging - as I am within callable. I am trying to get the name of the thread that executes "Callable" to include name of it's parent thread (tname).
some parts can be reused but not the full name
> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java, line 166
> > <https://reviews.apache.org/r/31107/diff/1/?file=866194#file866194line166>
> >
> > Would it make sense to restore the original thread name here? If so, wrap the iface.execute() in a try and restore the name in a finally.
There not much point in restoring original state, as those threads are only used to call "callable" and so each next thread will rename it to something it needs every time
> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java, line 178
> > <https://reviews.apache.org/r/31107/diff/1/?file=866194#file866194line178>
> >
> > Is IOException the best choice here? Would RuntimeException make more sense? Either way, would a message be helpful in case there are problems? How would a user know what to do if this exception fired?
Since method that evenually calls this one throws only IOException I am throwing it. Will add message
> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java, line 47
> > <https://reviews.apache.org/r/31107/diff/1/?file=866191#file866191line47>
> >
> > This should do something other than swallow InterruptedException. See http://www.ibm.com/developerworks/library/j-jtp05236/ .
Not that I touched this code, I need to see what should be done here.
- Yuliya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/#review73233
-----------------------------------------------------------
On Feb. 17, 2015, 12:31 a.m., Yuliya Feldman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31107/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2015, 12:31 a.m.)
>
>
> Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
>
>
> Bugs: DRILL-2210
> https://issues.apache.org/jira/browse/DRILL-2210
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> In addition to description
>
> Fixed few classes that did not handle multithreading well
> Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
> Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java f09acaa
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 4292c09
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java faa8546
> exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java aa0a5ad
>
> Diff: https://reviews.apache.org/r/31107/diff/
>
>
> Testing
> -------
>
> Still need to provide Unit Tests.
>
> Functional tests are passing
>
> Performance tests were run and look promising for some queries
>
>
> Thanks,
>
> Yuliya Feldman
>
>
Re: Review Request 31107: Ability to make PartitionSender
multithreaded - useful in case of LocalExchange being enabled,
as it allows to deal with high volume of incoming data
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31107/#review73233
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java
<https://reviews.apache.org/r/31107/#comment119501>
Why not call this(original, false) to avoid duplicating the code?
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java
<https://reviews.apache.org/r/31107/#comment119502>
how about
final IntLongOpenHashmap fromMetrics = from.longMetrics;
final long[] fromValues = fromMetrics.values;
for(int i : fromMetrics.keys) {
final long value = fromValues[i];
longMetrics.putOrAdd(i, value, value);
}
This avoids multiple evaluation of the member accesses on every pass of the loop, and multiple bounds checks within the loop body.
Similar improvements can be made to the doubleMetrics loop below.
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java
<https://reviews.apache.org/r/31107/#comment119503>
I'd make these final too, while you're here.
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java
<https://reviews.apache.org/r/31107/#comment119504>
This should do something other than swallow InterruptedException. See http://www.ibm.com/developerworks/library/j-jtp05236/ .
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java
<https://reviews.apache.org/r/31107/#comment119506>
Reuse the id variable from above (which looks like it should be made final).
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
<https://reviews.apache.org/r/31107/#comment119507>
Could this be final?
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
<https://reviews.apache.org/r/31107/#comment119508>
Call part.getStats() once at the beginning, and reuse the local throughout.
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
<https://reviews.apache.org/r/31107/#comment119510>
part.getStats() once.
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
<https://reviews.apache.org/r/31107/#comment119511>
Call Thread.currentThread() once at the beginning, and then reuse.
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
<https://reviews.apache.org/r/31107/#comment119513>
Would it make sense to restore the original thread name here? If so, wrap the iface.execute() in a try and restore the name in a finally.
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
<https://reviews.apache.org/r/31107/#comment119514>
Is IOException the best choice here? Would RuntimeException make more sense? Either way, would a message be helpful in case there are problems? How would a user know what to do if this exception fired?
- Chris Westin
On Feb. 17, 2015, 12:31 a.m., Yuliya Feldman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31107/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2015, 12:31 a.m.)
>
>
> Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
>
>
> Bugs: DRILL-2210
> https://issues.apache.org/jira/browse/DRILL-2210
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> In addition to description
>
> Fixed few classes that did not handle multithreading well
> Added/Changed some Stats behavior to allow stats merge from multiple threads, since again this class is not suitable to be used in multithreaded environment
> Introduced new decorator class to handle multi thrteading (or not) to minimize changes to ParitionSenderRootExec class
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 7af7b65
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java f09acaa
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 5ed9c39
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 4292c09
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java faa8546
> exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java aa0a5ad
>
> Diff: https://reviews.apache.org/r/31107/diff/
>
>
> Testing
> -------
>
> Still need to provide Unit Tests.
>
> Functional tests are passing
>
> Performance tests were run and look promising for some queries
>
>
> Thanks,
>
> Yuliya Feldman
>
>