----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27169/#review58901 -----------------------------------------------------------
http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java <https://reviews.apache.org/r/27169/#comment100063> Shall we put in a synchonized block? Also we can check here if we are already in spill to avoid repeated spill(). http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java <https://reviews.apache.org/r/27169/#comment100308> We shall not wait here until spill done, this will hold gc hook and does not seems do any good. Can we just do a synchronous aggregation and quit the hook? http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/impl/util/SpillableMemoryManager.java <https://reviews.apache.org/r/27169/#comment100309> Can we introduce an interface instead of using "instanceof POPartialAgg" here? http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestPOPartialAgg.java <https://reviews.apache.org/r/27169/#comment100310> The test fail, my first result is (2,(-1)), and second result is (1,(120)) http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestPOPartialAgg.java <https://reviews.apache.org/r/27169/#comment100311> The test fail for me, I also get (2,(-1)) in this loop. Should we move #339 after this loop? - Daniel Dai On Oct. 24, 2014, 9:59 p.m., Rohini Palaniswamy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27169/ > ----------------------------------------------------------- > > (Updated Oct. 24, 2014, 9:59 p.m.) > > > Review request for pig. > > > Bugs: PIG-3979 > https://issues.apache.org/jira/browse/PIG-3979 > > > Repository: pig > > > Description > ------- > > Fixed a couple of issues with POPartialAgg > - Made the spill of POPartialAgg synchronous so that System.gc() in > SpillableMemoryManager actually frees up memory. > - Avoid lot of redundant aggregateSecondLevel() calls > - Fixed the SpillableMemoryManager to not invoke extraGC if POPartialAgg > - Made variables transient which are not required to be serialized in the > plan > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java > 1633956 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/impl/util/SpillableMemoryManager.java > 1633956 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestPOPartialAgg.java > 1633956 > > Diff: https://reviews.apache.org/r/27169/diff/ > > > Testing > ------- > > Unit tests added to TestPOPartialAgg. Ran couple of manual e2e tests to check > behaviour. > > > Thanks, > > Rohini Palaniswamy > >
