----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/683/#review631 -----------------------------------------------------------
This doesn't lend itself well to automated testing. Any thoughts on how to test how the new progress indicator does versus the existing one? Have you run any initial tests to measure this? trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/Launcher.java <https://reviews.apache.org/r/683/#comment1275> I don't understand the logic here. Why is it 0% done if ANY job is waiting, etc.? Some of the jobs may be done and some partially done and some not even started. trunk/src/org/apache/pig/impl/plan/OperatorPlan.java <https://reviews.apache.org/r/683/#comment1276> This code shouldn't be in OperatorPlan. We want to keep that as clean as possible. Instead you should build a new Walker type that can do this calculation. trunk/src/org/apache/pig/impl/plan/OperatorPlan.java <https://reviews.apache.org/r/683/#comment1277> You have tabs here and some other spots. Please make sure you use 4 spaces rather than tabs. trunk/src/org/apache/pig/tools/pigstats/PigProgressNotificationListener.java <https://reviews.apache.org/r/683/#comment1278> Why is a separate method needed here? When users turn on the new progress indicator I assume they don't get the old one too. Given that the interfaces are the same it seems one method should suffice here. trunk/src/org/apache/pig/tools/pigstats/ScriptState.java <https://reviews.apache.org/r/683/#comment1279> It looks like this comment got attached to the run method. Also, the method has only one parameter, but two are listed in the comment. - Alan On 2011-05-02 20:41:04, Alan Gates wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/683/ > ----------------------------------------------------------- > > (Updated 2011-05-02 20:41:04) > > > Review request for pig. > > > Summary > ------- > > This is Laukik's patch for PIG-1883 > > > This addresses bug PIG-1883. > https://issues.apache.org/jira/browse/PIG-1883 > > > Diffs > ----- > > trunk/src/org/apache/pig/Main.java 1097661 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/Launcher.java > 1097661 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/MapReduceLauncher.java > 1097661 > trunk/src/org/apache/pig/impl/plan/OperatorPlan.java 1097661 > trunk/src/org/apache/pig/scripting/SyncProgressNotificationAdaptor.java > 1097661 > > trunk/src/org/apache/pig/tools/pigstats/PigProgressNotificationListener.java > 1097661 > trunk/src/org/apache/pig/tools/pigstats/ScriptState.java 1097661 > trunk/test/org/apache/pig/test/TestOperatorPlan.java 1097661 > > Diff: https://reviews.apache.org/r/683/diff > > > Testing > ------- > > > Thanks, > > Alan > >
