Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5644: Reject queries if min reservation is too large ......................................................................
Patch Set 4: (8 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 bu Done PS4, Line 53: minimum > i think that should be deleted, i.e. you might want to ask this for a poten Done PS4, Line 57: GetMemLimitFromBufferReservation > the name makes it sound like an inverse of GetBufferReservationLimitFromMem Done http://gerrit.cloudera.org:8080/#/c/7678/4/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: PS4, Line 153: if (query_options().__isset.buffer_pool_limit : && query_options().buffer_pool_limit > 0) { : max_reservation = query_options().buffer_pool_limit; : } Question for Tim & Dan: Should this really be exclusive w/ checking the mem limit? Don't we want: max_reservation = mem_limit == -1 ? max_int64 : ResUtil::GetBufferReservationLimitFromMemLimit(mem_limit) if (buffer_pool_limit is set): max_reservation = min(query_options.buffer_pool_limit, max_reservation) 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?) Done Line 125: "more information."; > both of these two reasons aren't really admission control specific, right? That's right, at least not as we currently think about admission control. It could go somewhere else but I thought it made sense to do all of our resource-related query 'rejecting' in one place, and I think it makes sense for admission control to be that place. I also don't think people should be running with AC disabled anymore. It's better to leave AC enabled but w/ unlimited limits (which is the default behavior). I think there shouldn't be many users with AC explicitly disabled anymore. Maybe I'll mark those flags as deprecated and remove in Impala 3.0? PS4, Line 428: buffer_reservation_limit > might be nice to name this consistently with query-state.cc naming, to make Done PS4, Line 429: GetBufferReservationLimitFromMemLimit > what about the case of buffer_pool_limit set explicitly? Do you mean something besides what gets handled on l416-l424? -- 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
