You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pig.apache.org by Cheolsoo Park <pi...@gmail.com> on 2014/05/15 01:49:13 UTC
Review Request 21465: PIG-3918 Implement PPNL for Tez mode (Pig side changes)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21465/
-----------------------------------------------------------
Review request for pig, Daniel Dai and Rohini Palaniswamy.
Bugs: PIG-3918
https://issues.apache.org/jira/browse/PIG-3918
Repository: pig-git
Description
-------
This patch makes PPNL work with Pig-on-Tez:
* Emit event notifications to PPNL in TezLauncher.
* Add a TezJob handle to TezStats. PPNL needs to access to DAG status to report progress.
* Along the way, two bug fixes-
* TezDagBuilder wasn't setting POLoad#lFile while constructing POSimpleTezLoad. This causes NPE in PPNL.
* BaseOperatorPlan#connect() shouldn't add nulls to edge maps. This causes NPE in PPNL.
* Fix unit tests. Some tests were failing after my changes because of ClassCastException from TezScriptState to MRScriptState.
Diffs
-----
src/org/apache/pig/backend/hadoop/executionengine/tez/POSimpleTezLoad.java f5ea1a5
src/org/apache/pig/backend/hadoop/executionengine/tez/TezDagBuilder.java 692cf08
src/org/apache/pig/backend/hadoop/executionengine/tez/TezJob.java da587f8
src/org/apache/pig/backend/hadoop/executionengine/tez/TezJobControl.java de01a9f
src/org/apache/pig/backend/hadoop/executionengine/tez/TezLauncher.java f5a8461
src/org/apache/pig/newplan/BaseOperatorPlan.java d86b60b
src/org/apache/pig/tools/pigstats/tez/TezStats.java f94b954
test/org/apache/pig/test/TestAccumulator.java 16f067a
test/org/apache/pig/test/TestBuiltin.java d33ed89
test/org/apache/pig/test/TestLocalRearrange.java 7ffc872
test/org/apache/pig/test/TestSkewedJoin.java 1ce3350
Diff: https://reviews.apache.org/r/21465/diff/
Testing
-------
Thanks,
Cheolsoo Park
Re: Review Request 21465: PIG-3918 Implement PPNL for Tez mode (Pig side
changes)
Posted by Cheolsoo Park <pi...@gmail.com>.
> On May 15, 2014, 10:17 p.m., Rohini Palaniswamy wrote:
> > src/org/apache/pig/backend/hadoop/executionengine/tez/TezLauncher.java, line 147
> > <https://reviews.apache.org/r/21465/diff/1/?file=582126#file582126line147>
> >
> > Shouldn't it be job.getMapredJobId()?
job.getMapredJobId() returns null in Tez. But I can add getAppId() to TezJob and use it instead. Thanks for pointing this out.
> On May 15, 2014, 10:17 p.m., Rohini Palaniswamy wrote:
> > src/org/apache/pig/backend/hadoop/executionengine/tez/TezLauncher.java, line 172
> > <https://reviews.apache.org/r/21465/diff/1/?file=582126#file582126line172>
> >
> > Can we set id to that of the application (controlledjob.getMapredJobID()) as tts.getName() is already vertex name?
No, this is intentional.
PPNL uses tts.getJobId() to identify MR jobs / Tez vertices in a Pig job. So if I change getJobId() to return AppId in Tez, I have to maintain two code paths in PPNL for MR and Tez.
I'd prefer keeping getJobId() returns vertex name in Tez. That is in fact clearly indicated in TezTaskStats code-
@Override
public String getJobId() {
return (vertexName == null) ? "" : vertexName;
}
- Cheolsoo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21465/#review43167
-----------------------------------------------------------
On May 14, 2014, 11:49 p.m., Cheolsoo Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21465/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 11:49 p.m.)
>
>
> Review request for pig, Daniel Dai and Rohini Palaniswamy.
>
>
> Bugs: PIG-3918
> https://issues.apache.org/jira/browse/PIG-3918
>
>
> Repository: pig-git
>
>
> Description
> -------
>
> This patch makes PPNL work with Pig-on-Tez:
>
> * Emit event notifications to PPNL in TezLauncher.
> * Add a TezJob handle to TezStats. PPNL needs to access to DAG status to report progress.
> * Along the way, two bug fixes-
> * TezDagBuilder wasn't setting POLoad#lFile while constructing POSimpleTezLoad. This causes NPE in PPNL.
> * BaseOperatorPlan#connect() shouldn't add nulls to edge maps. This causes NPE in PPNL.
> * Fix unit tests. Some tests were failing after my changes because of ClassCastException from TezScriptState to MRScriptState.
>
>
> Diffs
> -----
>
> src/org/apache/pig/backend/hadoop/executionengine/tez/POSimpleTezLoad.java f5ea1a5
> src/org/apache/pig/backend/hadoop/executionengine/tez/TezDagBuilder.java 692cf08
> src/org/apache/pig/backend/hadoop/executionengine/tez/TezJob.java da587f8
> src/org/apache/pig/backend/hadoop/executionengine/tez/TezJobControl.java de01a9f
> src/org/apache/pig/backend/hadoop/executionengine/tez/TezLauncher.java f5a8461
> src/org/apache/pig/newplan/BaseOperatorPlan.java d86b60b
> src/org/apache/pig/tools/pigstats/tez/TezStats.java f94b954
> test/org/apache/pig/test/TestAccumulator.java 16f067a
> test/org/apache/pig/test/TestBuiltin.java d33ed89
> test/org/apache/pig/test/TestLocalRearrange.java 7ffc872
> test/org/apache/pig/test/TestSkewedJoin.java 1ce3350
>
> Diff: https://reviews.apache.org/r/21465/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Cheolsoo Park
>
>
Re: Review Request 21465: PIG-3918 Implement PPNL for Tez mode (Pig side
changes)
Posted by Rohini Palaniswamy <ro...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21465/#review43167
-----------------------------------------------------------
src/org/apache/pig/backend/hadoop/executionengine/tez/POSimpleTezLoad.java
<https://reviews.apache.org/r/21465/#comment77231>
Can we remove this constructor?
src/org/apache/pig/backend/hadoop/executionengine/tez/TezJob.java
<https://reviews.apache.org/r/21465/#comment77232>
getDAGProgress
src/org/apache/pig/backend/hadoop/executionengine/tez/TezLauncher.java
<https://reviews.apache.org/r/21465/#comment77233>
Shouldn't it be job.getMapredJobId()?
src/org/apache/pig/backend/hadoop/executionengine/tez/TezLauncher.java
<https://reviews.apache.org/r/21465/#comment77230>
Can we set id to that of the application (controlledjob.getMapredJobID()) as tts.getName() is already vertex name?
- Rohini Palaniswamy
On May 14, 2014, 11:49 p.m., Cheolsoo Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21465/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 11:49 p.m.)
>
>
> Review request for pig, Daniel Dai and Rohini Palaniswamy.
>
>
> Bugs: PIG-3918
> https://issues.apache.org/jira/browse/PIG-3918
>
>
> Repository: pig-git
>
>
> Description
> -------
>
> This patch makes PPNL work with Pig-on-Tez:
>
> * Emit event notifications to PPNL in TezLauncher.
> * Add a TezJob handle to TezStats. PPNL needs to access to DAG status to report progress.
> * Along the way, two bug fixes-
> * TezDagBuilder wasn't setting POLoad#lFile while constructing POSimpleTezLoad. This causes NPE in PPNL.
> * BaseOperatorPlan#connect() shouldn't add nulls to edge maps. This causes NPE in PPNL.
> * Fix unit tests. Some tests were failing after my changes because of ClassCastException from TezScriptState to MRScriptState.
>
>
> Diffs
> -----
>
> src/org/apache/pig/backend/hadoop/executionengine/tez/POSimpleTezLoad.java f5ea1a5
> src/org/apache/pig/backend/hadoop/executionengine/tez/TezDagBuilder.java 692cf08
> src/org/apache/pig/backend/hadoop/executionengine/tez/TezJob.java da587f8
> src/org/apache/pig/backend/hadoop/executionengine/tez/TezJobControl.java de01a9f
> src/org/apache/pig/backend/hadoop/executionengine/tez/TezLauncher.java f5a8461
> src/org/apache/pig/newplan/BaseOperatorPlan.java d86b60b
> src/org/apache/pig/tools/pigstats/tez/TezStats.java f94b954
> test/org/apache/pig/test/TestAccumulator.java 16f067a
> test/org/apache/pig/test/TestBuiltin.java d33ed89
> test/org/apache/pig/test/TestLocalRearrange.java 7ffc872
> test/org/apache/pig/test/TestSkewedJoin.java 1ce3350
>
> Diff: https://reviews.apache.org/r/21465/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Cheolsoo Park
>
>
Re: Review Request 21465: PIG-3918 Implement PPNL for Tez mode (Pig side
changes)
Posted by Cheolsoo Park <pi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21465/
-----------------------------------------------------------
(Updated May 16, 2014, 3:58 p.m.)
Review request for pig, Daniel Dai and Rohini Palaniswamy.
Changes
-------
Incorporate Rohini's comments.
Bugs: PIG-3918
https://issues.apache.org/jira/browse/PIG-3918
Repository: pig-git
Description
-------
This patch makes PPNL work with Pig-on-Tez:
* Emit event notifications to PPNL in TezLauncher.
* Add a TezJob handle to TezStats. PPNL needs to access to DAG status to report progress.
* Along the way, two bug fixes-
* TezDagBuilder wasn't setting POLoad#lFile while constructing POSimpleTezLoad. This causes NPE in PPNL.
* BaseOperatorPlan#connect() shouldn't add nulls to edge maps. This causes NPE in PPNL.
* Fix unit tests. Some tests were failing after my changes because of ClassCastException from TezScriptState to MRScriptState.
Diffs (updated)
-----
src/org/apache/pig/backend/hadoop/executionengine/tez/POSimpleTezLoad.java f5ea1a5
src/org/apache/pig/backend/hadoop/executionengine/tez/TezDagBuilder.java 692cf08
src/org/apache/pig/backend/hadoop/executionengine/tez/TezJob.java da587f8
src/org/apache/pig/backend/hadoop/executionengine/tez/TezJobControl.java de01a9f
src/org/apache/pig/backend/hadoop/executionengine/tez/TezLauncher.java f5a8461
src/org/apache/pig/newplan/BaseOperatorPlan.java d86b60b
src/org/apache/pig/tools/pigstats/tez/TezStats.java f94b954
test/org/apache/pig/test/TestAccumulator.java 16f067a
test/org/apache/pig/test/TestBuiltin.java d33ed89
test/org/apache/pig/test/TestLocalRearrange.java 7ffc872
test/org/apache/pig/test/TestSkewedJoin.java 1ce3350
Diff: https://reviews.apache.org/r/21465/diff/
Testing
-------
Thanks,
Cheolsoo Park