Dan Hecht has posted comments on this change. Change subject: IMPALA-5644: Reject queries if min reservation is too large ......................................................................
Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/7678/4/be/src/runtime/bufferpool/reservation-util.h File be/src/runtime/bufferpool/reservation-util.h: PS4, Line 44: buffer reservation limi is it clear that this is talking about the query limit as opposed to the buffer-pool wide limit? I guess maybe, since we refer to the latter as the "buffer pool limit", but maybe worth clarifying. PS4, Line 53: minimum i think that should be deleted, i.e. you might want to ask this for a potential initial reservation that's larger than the minimum. PS4, Line 57: GetMemLimitFromBufferReservation the name makes it sound like an inverse of GetBufferReservationLimitFromMemLimit(), but it's a little different conceptually. How about GetMinMemLimitFromBufferReservation or GetRequiredMemLimitFromBufferReservation... also, if you want to shorten, I think removing the word "buffer" from the name would be okay. http://gerrit.cloudera.org:8080/#/c/7678/4/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: PS4, Line 119: greater than at least? (off by one?) and are we suggesting that mem_limit be set to this value? If so, maybe say that more explicitly: Set mem_limit to at least $2? Line 125: "more information."; both of these two reasons aren't really admission control specific, right? i'm okay with having them here though, but what happens when admission control is disabled? it fails later to get the initial reservation, I guess? PS4, Line 428: buffer_reservation_limit might be nice to name this consistently with query-state.cc naming, to make it clear we're talking about the same thing, which I think calls it max_reservation (or rename that one). PS4, Line 429: GetBufferReservationLimitFromMemLimit what about the case of buffer_pool_limit set explicitly? -- To view, visit http://gerrit.cloudera.org:8080/7678 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
