Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10365 )
Change subject: IMPALA-6035: Add query options to limit thread reservation ...................................................................... Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/10365/7/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/10365/7/be/src/scheduling/admission-controller.cc@426 PS7, Line 426: max_thread_reservation = make_pair(&e.first, e.second.thread_reservation); I think the use of pairs hurts readability (since fields don't have meaningful names) but given that the new pair is declared right here, I don't feel strongly in this case. So if you and Bikram feel this is clearest, leave it. http://gerrit.cloudera.org:8080/#/c/10365/7/be/src/scheduling/admission-controller.cc@432 PS7, Line 432: chedule->query_options().__isset.buffer_pool_limit : && schedule->query_options() query_opts http://gerrit.cloudera.org:8080/#/c/10365/7/be/src/scheduling/admission-controller.cc@440 PS7, Line 440: else if Preexisting, but I think it's non-obvious why this is "else if" with the buffer pool option check. I had to do a double take. How about adding a brief explanation (maybe just reference InitBufferPoolState())? http://gerrit.cloudera.org:8080/#/c/10365/7/be/src/scheduling/admission-controller.cc@461 PS7, Line 461: else if given that these aren't really mutually exclusive (unlike the buffer pool limit vs mem_limit thing), how about just use 'if' here? The "return true" pattern is used throughout the exclude downstream checks. http://gerrit.cloudera.org:8080/#/c/10365/7/be/src/scheduling/query-schedule.h File be/src/scheduling/query-schedule.h: http://gerrit.cloudera.org:8080/#/c/10365/7/be/src/scheduling/query-schedule.h@49 PS7, Line 49: input to a BackendState input to AdmissionController and a BackendState (especially given that the thread reservation has no execution-side counterpart) -- To view, visit http://gerrit.cloudera.org:8080/10365 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b5bbbdad5cd6b24442eb6c99a4d38c2ad710007 Gerrit-Change-Number: 10365 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 30 May 2018 21:53:19 +0000 Gerrit-HasComments: Yes
