Dan Hecht has posted comments on this change. Change subject: IMPALA-4674: Part 2: port backend exec to BufferPool ......................................................................
Patch Set 23: (4 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); > No. I'm not sure that it's a useful check either - if the ExecNode implemen What I mean is wouldn't it be incorrect to return min_reservation if the client doesn't have it? Is it legal to call ReservationTracker::TransferReservationTo() with bytes > current reservation? what happens in that case? http://gerrit.cloudera.org:8080/#/c/5801/23/be/src/exec/hash-table.h File be/src/exec/hash-table.h: PS23, Line 625: Status* status > I tried to do this a while back with BTS::AddRow() but it was a can of worm That's really unfortunate. Probably worth revisiting at some point, but not now. 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 http://gerrit.cloudera.org:8080/#/c/5801/23/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: PS23, Line 403: > In most cases it's probably a lot better, since it holds onto unclaimed ini I'm not sure why it's better in the case of cancelation, since the old code would release all buffered block mgr blocks at time of fragment instance cancel. I think we should basically do that but it should be more generally just an atomic count of executing fragments, and when the last fragment instance completes, ReleaseResources() is called. The current callsite of ReleaseResources() seems wrong -- the refcount for the QS should only keep the control structures around, not the resources. And regarding runtime filters, why should those resources live beyond the last running fragment instance? Assuming we could go forward with that, maybe best to separate this into a different commit that goes in first, to isolate it. -- 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
