Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 )
Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. ...................................................................... Patch Set 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc File be/src/scheduling/admission-controller-test.cc: http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc@71 PS6, Line 71: /// Build a QuerySchedule object. Comment could be more descriptive. http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc@74 PS6, Line 74: DCHECK(num_hosts > 0); DCHECK_GT http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc@175 PS6, Line 175: /// Check the calculations made by GetMaxQueuedForPool and GetMaxRequestsForPool are Extra spaces after /// http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc@204 PS6, Line 204: /// Make an AdmissionController Comment is a little uninformative. Maybe "Make an AdmissionController with some dummy parameters" or something like that. http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller.h@583 PS6, Line 583: int64_t cluster_size_snapshot, bool admit_from_queue, string* not_admitted_reason); We generally use std::string in .h since the rule is that headers shouldn't import things into the global namespace. Some headers we depend on don't follow this rule, so std::string got into the global namespace somehow, but we shouldn't depend on that. http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller.cc@1390 PS6, Line 1390: int64_t AdmissionController::GetClusterSize() { I think we need to be a little careful with this being O(cluster size). If it was used in the wrong context the runtime could end up blowing up. I think it's ok with the current usage pattern, but it might be worth caching this value in ClusterMembershipMgr::Snapshot to avoid repeated recomputations. This is kind-of premature optimisation, but I think any perf issues might rear their head in inconvenient situations, e.g. very high query concurrency. http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller.cc@1445 PS6, Line 1445: && Should this be ||? Since we shouldn't be able to submit queries to a pool if either it has 0 request or 0 memory. http://gerrit.cloudera.org:8080/#/c/13307/6/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/13307/6/common/thrift/ImpalaInternalService.thrift@695 PS6, Line 695: : inconsistent whitespace before colon -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 03 Jun 2019 23:24:13 +0000 Gerrit-HasComments: Yes
