Review Board 1.7.22


Review request for PIG-1883-2.patch

Review Request #683 - Created May 2, 2011 and updated

Alan Gates
PIG-1883
Reviewers
pig
pig
This is Laukik's patch for PIG-1883

 
Posted (May 2, 2011, 9:40 p.m.)
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?
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.
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.
You have tabs here and some other spots.  Please make sure you use 4 spaces rather than tabs.
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.
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.