You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pig.apache.org by Rohini Palaniswamy <ro...@gmail.com> on 2012/10/02 20:23:51 UTC
Re: Review Request: PIG-2898: allow to run pig e2e tests in parallel mode.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7053/#review12079
-----------------------------------------------------------
Just few minor comments.
I ran the patch on branch-0.10 after reverting the commented out test in steaming_local.conf. It passed fine in Hadoop 1.0.2 but hung in hadoop 0.23. With the previous patch changing to -Dmapreduce.cluster.local.dir it was passing in Hadoop 0.23. I am trying to debug that.
http://svn.apache.org/repos/asf/pig/trunk/test/e2e/harness/test_harness.pl
<https://reviews.apache.org/r/7053/#comment25774>
Can't we do this by getting $ENV{'HADOOP_MAPRED_LOCAL_DIR'} in local.conf? Or is there some reason for doing it this way?
http://svn.apache.org/repos/asf/pig/trunk/test/e2e/harness/TestDriver.pm
<https://reviews.apache.org/r/7053/#comment25777>
Any reason for this length based formatting? In fact it makes the string even more longer than before. Just curious to know why this is being done.
http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/build.xml
<https://reviews.apache.org/r/7053/#comment25741>
Default /tmp/hadoop/mapred/local/?
http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/conf/local.conf
<https://reviews.apache.org/r/7053/#comment25740>
Same as prev 2 comments. Just repeating it in context. Can this be ENV{HADOOP_MAPRED_LOCAL_DIR} and define /tmp/hadoop/mapred/local as the default in build.xml similar to other variables in this conf?
http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/drivers/TestDriverPig.pm
<https://reviews.apache.org/r/7053/#comment25793>
1024m instead of 1025m
http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/drivers/TestDriverPig.pm
<https://reviews.apache.org/r/7053/#comment25739>
Nice to see someone fix this one. Has always bothered my eyes while looking at the logs too see it appended so many times :). Can we remove the code altogether instead of commenting out.
http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/tests/streaming_local.conf
<https://reviews.apache.org/r/7053/#comment25738>
Instead of commenting this out in this patch, we can create a new jira so that this test can be fixed. TestStreaming unit tests also are timing out in trunk. So there might be a bug with streaming that needs to be fixed before 0.11 is released.
The test runs fine in 0.10 in seq mode. Removing this will also make the patch apply cleanly on 0.10 else we need to create a patch for 0.10.
- Rohini Palaniswamy
On Sept. 28, 2012, 12:20 p.m., Ivan Veselovsky wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7053/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2012, 12:20 p.m.)
>
>
> Review request for pig and Rohini Palaniswamy.
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/PIG-2898 for details.
>
>
> This addresses bug https://issues.apache.org/jira/browse/PIG-2898.
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/PIG-2898
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/pig/trunk/test/e2e/harness/TestDriver.pm 1390615
> http://svn.apache.org/repos/asf/pig/trunk/test/e2e/harness/test_harness.pl 1390615
> http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/build.xml 1390615
> http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/conf/local.conf 1390615
> http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/deployers/ExistingClusterDeployer.pm 1390615
> http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/drivers/TestDriverPig.pm 1390615
> http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/tests/streaming_local.conf 1390615
>
> Diff: https://reviews.apache.org/r/7053/diff/
>
>
> Testing
> -------
>
> Tested e2e tests execution in both sequential (default) and parallel modes.
> The test run duration measurement data (in dependency on the fork parameters) will be available soon.
>
>
> Thanks,
>
> Ivan Veselovsky
>
>
Re: Review Request: PIG-2898: allow to run pig e2e tests in parallel mode.
Posted by Ivan Veselovsky <iv...@griddynamics.com>.
> On Oct. 2, 2012, 6:23 p.m., Rohini Palaniswamy wrote:
> > http://svn.apache.org/repos/asf/pig/trunk/test/e2e/harness/TestDriver.pm, lines 89-95
> > <https://reviews.apache.org/r/7053/diff/2/?file=160775#file160775line89>
> >
> > Any reason for this length based formatting? In fact it makes the string even more longer than before. Just curious to know why this is being done.
The reason is to have no horizontal shifts in the result table. With the formatting we have
..... PASSED: 1 FAILED: 0 SKIPPED: 0 ....
.....
..... PASSED: 999 FAILED: 15 SKIPPED: 18 ....
, while without the formatting we would have
..... PASSED: 1 FAILED: 0 SKIPPED: 0 ....
.....
..... PASSED: 999 FAILED: 15 SKIPPED: 18 ....
> On Oct. 2, 2012, 6:23 p.m., Rohini Palaniswamy wrote:
> > http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/drivers/TestDriverPig.pm, line 378
> > <https://reviews.apache.org/r/7053/diff/2/?file=160780#file160780line378>
> >
> > 1024m instead of 1025m
1025 was set for debug purposes: this allows to know exactly where the setting comes from. :)
Fixed to be 1024 back.
> On Oct. 2, 2012, 6:23 p.m., Rohini Palaniswamy wrote:
> > http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/drivers/TestDriverPig.pm, lines 383-385
> > <https://reviews.apache.org/r/7053/diff/2/?file=160780#file160780line383>
> >
> > Nice to see someone fix this one. Has always bothered my eyes while looking at the logs too see it appended so many times :). Can we remove the code altogether instead of commenting out.
done.
> On Oct. 2, 2012, 6:23 p.m., Rohini Palaniswamy wrote:
> > http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/tests/streaming_local.conf, lines 179-189
> > <https://reviews.apache.org/r/7053/diff/2/?file=160781#file160781line179>
> >
> > Instead of commenting this out in this patch, we can create a new jira so that this test can be fixed. TestStreaming unit tests also are timing out in trunk. So there might be a bug with streaming that needs to be fixed before 0.11 is released.
> >
> > The test runs fine in 0.10 in seq mode. Removing this will also make the patch apply cleanly on 0.10 else we need to create a patch for 0.10.
Actually this change was included into the patch just by accident: i just commented this test to avoid the hangup.
The issue with this test seems to be out of scope of the parallel e2e task, so, sure, I exclude this change from the patch.
> On Oct. 2, 2012, 6:23 p.m., Rohini Palaniswamy wrote:
> > http://svn.apache.org/repos/asf/pig/trunk/test/e2e/harness/test_harness.pl, lines 292-297
> > <https://reviews.apache.org/r/7053/diff/1-2/?file=152854#file152854line292>
> >
> > Can't we do this by getting $ENV{'HADOOP_MAPRED_LOCAL_DIR'} in local.conf? Or is there some reason for doing it this way?
The only reason is backward compatibility: if somebody had non-ant script to run test_harness.pl, that script has no HADOOP_MAPRED_LOCAL_DIR env variable set, so, here we provide the default in the local.conf.
I re-implemented this in the following way: the default in local.conf is present, but used only conditionally, if the corresponding ENV key is not defiled. And the ENV value is passed through ant variable, which, in turn, also has reasonable default.
> On Oct. 2, 2012, 6:23 p.m., Rohini Palaniswamy wrote:
> > http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/build.xml, line 251
> > <https://reviews.apache.org/r/7053/diff/2/?file=160777#file160777line251>
> >
> > Default /tmp/hadoop/mapred/local/?
please see the 1st comment in this review above.
> On Oct. 2, 2012, 6:23 p.m., Rohini Palaniswamy wrote:
> > http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/conf/local.conf, line 61
> > <https://reviews.apache.org/r/7053/diff/2/?file=160778#file160778line61>
> >
> > Same as prev 2 comments. Just repeating it in context. Can this be ENV{HADOOP_MAPRED_LOCAL_DIR} and define /tmp/hadoop/mapred/local as the default in build.xml similar to other variables in this conf?
please see the 1st comment in this review above.
- Ivan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7053/#review12079
-----------------------------------------------------------
On Sept. 28, 2012, 12:20 p.m., Ivan Veselovsky wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7053/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2012, 12:20 p.m.)
>
>
> Review request for pig and Rohini Palaniswamy.
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/PIG-2898 for details.
>
>
> This addresses bug https://issues.apache.org/jira/browse/PIG-2898.
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/PIG-2898
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/pig/trunk/test/e2e/harness/TestDriver.pm 1390615
> http://svn.apache.org/repos/asf/pig/trunk/test/e2e/harness/test_harness.pl 1390615
> http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/build.xml 1390615
> http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/conf/local.conf 1390615
> http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/deployers/ExistingClusterDeployer.pm 1390615
> http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/drivers/TestDriverPig.pm 1390615
> http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/tests/streaming_local.conf 1390615
>
> Diff: https://reviews.apache.org/r/7053/diff/
>
>
> Testing
> -------
>
> Tested e2e tests execution in both sequential (default) and parallel modes.
> The test run duration measurement data (in dependency on the fork parameters) will be available soon.
>
>
> Thanks,
>
> Ivan Veselovsky
>
>