Bikramjeet Vig 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: (15 comments) 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: Use a nit: Uses a http://gerrit.cloudera.org:8080/#/c/13307/5/be/src/scheduling/admission-controller.h@664 PS5, Line 664: This is based on the : /// max_requests_estimate limit and the current queue size. We will attempt to : /// dequeue up to this number of requests until reaching the per-pool memory limit nit: not sure if this detail is relevant there. maybe remove this? 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: _size_snapshot, > The ClusterMembershipMgr that will come in IMPALA-8460 has a method GetSnap Not a common term but just reviewing IMPALA-8460 drilled into me that the notion that a snapshot is like a snapshot of some (meta)data. how about cluster_snapshot_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: > I originally used the cluster size from QuerySchedule but hit problems with I thought about this more and I think not counting the quiescing backends would be the better alternative since in this case the queries scheduled to run on those backends would not hold up the rest of the queue and in case queries scheduled on lesser num of backends get overadmitted, we can consider those an extension of the case where a query is only scheduled on a subset of backends (another eg would be query only scheduled on coord). http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1444 PS3, Line 1444: rn > should be OR (||) <bump> 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: absolute limit nit: outdated comment 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: Impalada nit: Impalads http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1002 PS5, Line 1002: def check_admission_by_memory(self, expected_admission, expected_rejection_reason=None): nit: by convention, we usually prefix a helper method with a double underscore ( __ ) http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1003 PS5, Line 1003: query = "select * from functional.alltypesagg order by int_col limit 1" nit: add method comment http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1003 PS5, Line 1003: query = "select * from functional.alltypesagg order by int_col limit 1" might be useful to set the mem_limit too to make this less flaky in case the estimates change in the future. http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1015 PS5, Line 1015: assert expected_rejection_reason in rejected_reasons[0] nit: print the profile if this assert fails for easy debugging http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1016 PS5, Line 1016: yybecause nit: perhaps you forgot to remove this line? 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 then compare nit: maybe mention that this is to verify if the admission controller correctly enforces query count limits http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1037 PS5, Line 1037: assert (expected_num_admitted + expected_num_queued + : expected_num_rejected) == NUM_QUERIES it seems like we can calculate all these expected values from the num of impalads in the cluster, maybe that is the only variable we need to pass to this methods since the logic for expected values is incorporated in this method itself (as the request_pool is constant) . 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_resources) value to something like what GetMaxRequestsForPool returns. -- 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 18:13:07 +0000 Gerrit-HasComments: Yes
