Tim Armstrong has posted comments on this change. Change subject: IMPALA-5681: release reservation from blocking operators ......................................................................
Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/7619/5/be/src/exec/exec-node.h File be/src/exec/exec-node.h: PS5, Line 238: excess of the node's initial reservation > why do we need to hold on to the initial reservation if this should only be Yeah. The real requirement is that at least the initial reservation has to be handed back to InitialReservations. It would be correct if ReleaseUnusedReservation() reduced it below the minimum reservation by handing back the memory to InitialReservations (as opposed to just releasing it). It seemed simplest for now to avoid that complication in ReleaseUnusedReservation() Documented the invariant in ClaimBufferReservation(). http://gerrit.cloudera.org:8080/#/c/7619/5/be/src/exec/sort-node.cc File be/src/exec/sort-node.cc: PS5, Line 106: returned_buffer_ > so is that an optimization, or is it needed for correctness? It's an optimization to avoid calling this logic each iteration. Line 113: DCHECK(status.ok()) << "Should not fail - no runs were spilled. " > to make sure I understand - that's because there should be no unpinned (dir Yep. Added a DCHECK here and in the agg. http://gerrit.cloudera.org:8080/#/c/7619/5/be/src/exec/sort-node.h File be/src/exec/sort-node.h: PS5, Line 72: last > does that mean "final" or "previous"? Done -- To view, visit http://gerrit.cloudera.org:8080/7619 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
