Bikramjeet Vig 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) lgtm, jut a few minor nits 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 though) maybe use "largest_...." as a prefix for both min_mem_reservation_bytes and thread_reservation, just to differentiate between the use of max/min vs largest/smallest and avoid prefixes like max_min_... 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 both max_min_mem_reservation and max_thread_reservation. I can also add my proc mem limit logic in the same for-loop below using the same pair (to get the smallest proc mem limit among backends) 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 typedef std::map<TNetworkAddress, BackendExecParams> PerBackendExecParams; 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"? -- 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: Wed, 16 May 2018 17:49:27 +0000 Gerrit-HasComments: Yes
