Dan Hecht 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 used after we're done spilling? is that because we need to hold on to that for a later operator that we'll "transfer" it to? if so, do we document this invariant (that the operator must hold on to the initial reservation until Close())? 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? 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 (dirty) pages? worth dchecking directly that no pages for this client are unpinned, to verify the HasSpilledRuns() logic? (since failure on this path would be rare to exercise). 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"? -- 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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
