Tim Armstrong 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: (6 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 meaning I personally liked the more compact code. It does seem like a pattern that could be misused though. My tendency is to leave it as-is. 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 Done 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 bu Done 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 l Done 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 Done http://gerrit.cloudera.org:8080/#/c/10365/4/testdata/workloads/functional-query/queries/QueryTest/resource-limits.test File testdata/workloads/functional-query/queries/QueryTest/resource-limits.test: http://gerrit.cloudera.org:8080/#/c/10365/4/testdata/workloads/functional-query/queries/QueryTest/resource-limits.test@1 PS4, Line 1: ==== > Can you rename the file to "thread-limits.test"? Done -- 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: Thu, 31 May 2018 23:22:14 +0000 Gerrit-HasComments: Yes
