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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
