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-4674: Part 2: port backend exec to BufferPool
Date Tue, 11 Jul 2017 06:48:37 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4674: Part 2: port backend exec to BufferPool
......................................................................


Patch Set 23:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5801/23/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:

Line 209:         &buffer_pool_client_, resource_profile_.min_reservation);
> What I mean is wouldn't it be incorrect to return min_reservation if the cl
Oh yes, you're right. Added documentation for that requirement in the header. It would hit
a DCHECK in ReservationTracker.


http://gerrit.cloudera.org:8080/#/c/5801/25/be/src/exec/hash-table.h
File be/src/exec/hash-table.h:

PS25, Line 532: f th
> double into
Done


http://gerrit.cloudera.org:8080/#/c/5801/23/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

PS23, Line 403: 
> I'm not sure why it's better in the case of cancelation, since the old code
I guess it's better in that most of the resources of a large query will be released even if
one fragment is slow to finish.

It sounds potentially hairy if we do it the wrong way. I don't think we want the Coordinator
to deal with the resources being released asynchronously by the executing fragments. It might
worth though if we also refcounted the QueryState resources and released the coordinator's
resource refcount in Coordinator::ReleaseResources().


-- 
To view, visit http://gerrit.cloudera.org:8080/5801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fc7fe1c04e9dfb1a0c749fb56a5e0f2bf9c6c3e
Gerrit-PatchSet: 23
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message