impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators
Date Wed, 16 Aug 2017 00:54:55 GMT
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 <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