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 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/10365/4/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/10365/4/be/src/scheduling/admission-controller.cc@414 PS4, Line 414: int64_t max_min_mem_reservation_bytes = -1; > (yet another rename nit for this variable. Dont feel too strongly about it Done http://gerrit.cloudera.org:8080/#/c/10365/4/be/src/scheduling/admission-controller.cc@414 PS4, Line 414: int64_t max_min_mem_reservation_bytes = -1; : const TNetworkAddress* max_min_mem_reservation_bytes_addr = nullptr; : int64_t cluster_min_mem_reservation_bytes = 0; : int64_t max_thread_reservation = 0; : const TNetworkAddress* max_thread_reservation_addr = nullptr; : int64_t cluster_thread_reservation = 0; > it it dosent hurt readability then we can use a pair<address,int64_t> for b Done http://gerrit.cloudera.org:8080/#/c/10365/4/be/src/scheduling/admission-controller.cc@424 PS4, Line 424: e.second.instance_params[0] > can use e.first instead of this since Done http://gerrit.cloudera.org:8080/#/c/10365/4/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/10365/4/be/src/service/query-options.cc@672 PS4, Line 672: Only -1 > do we need both 0 and -1 to represent "no limit"? Ideally no, but I wanted to keep it consistent with other options that behave like this. I guess one alternative is to make "0" mean literally zero and have it reject all queries :). But I think that might cause more confusion. -- 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: 4 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Sat, 19 May 2018 00:03:38 +0000 Gerrit-HasComments: Yes
