impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators
Date Wed, 16 Aug 2017 16:45:02 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message