Andrew Sherman 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 7: (21 comments) Addressing review comments, main changes are: Switched to using ClusterMembershipMgr to get cluster membership. Enhanced ClusterMembershipMgr to add a count of non-quiescing backends to the Snapshot. Added a simple unit test for AdmissionControoler::PoolDisabled. Added metrics for derived configuration values. http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller-test.cc File be/src/scheduling/admission-controller-test.cc: http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller-test.cc@279 PS3, Line 279: // AdmissionController. > Consider breaking this up into a few more targeted tests, I think it makes I have tidied up and added 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: > Comment could be more descriptive. Done http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc@74 PS6, Line 74: QuerySchedule* MakeQuerySchedule(string request_pool_name, TPoolConfig& config, > DCHECK_GT Done http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc@175 PS6, Line 175: } > Extra spaces after /// Done http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc@204 PS6, Line 204: } > Comment is a little uninformative. Maybe "Make an AdmissionController with Done http://gerrit.cloudera.org:8080/#/c/13307/5/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/13307/5/be/src/scheduling/admission-controller.h@663 PS5, Line 663: > nit: Uses a Done http://gerrit.cloudera.org:8080/#/c/13307/5/be/src/scheduling/admission-controller.h@664 PS5, Line 664: pool is configured. : static std::string GetMaxMemForPoolDescription( : const TPoolConfig& pool_config, int64_t latest_cluster_size); > nit: not sure if this detail is relevant there. maybe remove this? Done http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.h@603 PS3, Line 603: > Not a common term but just reviewing IMPALA-8460 drilled into me that the n Ah now I understand what you didn't like. Going with your original suggestion 'latest_cluster_size' http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1396 PS3, Line 1396: uge( > I thought about this more and I think not counting the quiescing backends w Thanks for raising the question.I am now not counting the quiescing backends. http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1444 PS3, Line 1444: tu > should be OR (||) YES! http://gerrit.cloudera.org:8080/#/c/13307/5/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13307/5/be/src/scheduling/admission-controller.cc@1051 PS5, Line 1051: > nit: outdated comment Done 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: metrics_.max_queued_queries_multiple = parent_->metrics_group_->AddDoubleGauge( > I think we need to be a little careful with this being O(cluster size). If Thanks, new implementation uses ClusterMembershipMgr and the value is added to value in ClusterMembershipMgr::Snapshot . 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 i Yes! http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@994 PS5, Line 994: dmission > nit: Impalads Done http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1002 PS5, Line 1002: :param expected_rejection_reason: a string expected to be in the reason for rejection. > nit: by convention, we usually prefix a helper method with a double undersc Done http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1003 PS5, Line 1003: """ > nit: add method comment Done http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1015 PS5, Line 1015: assert num_rejected == 1 > nit: print the profile if this assert fails for easy debugging Done http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1016 PS5, Line 1016: expected_ > nit: perhaps you forgot to remove this line? yes http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1025 PS5, Line 1025: Run some queries, find how many were admitted, queued or rejected, and check that > nit: maybe mention that this is to verify if the admission controller corre Done http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1037 PS5, Line 1037: : impalad = self.cluster.impalads[0] > it seems like we can calculate all these expected values from the num of im Done http://gerrit.cloudera.org:8080/#/c/13307/5/www/admission_controller.tmpl File www/admission_controller.tmpl: http://gerrit.cloudera.org:8080/#/c/13307/5/www/admission_controller.tmpl@335 PS5, Line 335: pool_max_requests > we ll have to update this and other Limits(pool_max_queued, pool_max_mem_re I left these alone and added metrics for what GetMaxRequestsForPool etc return in the Pool Stats section. -- 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: 7 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: Thu, 06 Jun 2019 23:29:36 +0000 Gerrit-HasComments: Yes
