Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12244 )
Change subject: IMPALA-8092: Add an admission controller debug page ...................................................................... Patch Set 4: Code-Review+1 (5 comments) Hi Bikram, This looks good mostly, I don't have any experience with css/tmpl. Just reading through that part made sense to me but I am not entirely sure about it. Rest of change makes sense. Thanks, Pooja http://gerrit.cloudera.org:8080/#/c/12244/4/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/12244/4/be/src/scheduling/admission-controller.h@251 PS4, Line 251: int64_t I might be missing something, but why are we using int64_t here and in the ResourceUtilization structure? Given the memory consumption is always non-negative would it make sense to use uint64_t instead? http://gerrit.cloudera.org:8080/#/c/12244/4/be/src/scheduling/admission-controller.h@264 PS4, Line 264: that Nit: that -> to which (Not super critical, I initially had some trouble understanding the context). http://gerrit.cloudera.org:8080/#/c/12244/4/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/12244/4/be/src/scheduling/admission-controller.cc@44 PS4, Line 44: const int64_t AdmissionController::PoolStats::HISTOGRAM_NUM_OF_BINS = 128; : const int64_t AdmissionController::PoolStats::HISTOGRAM_BIN_SIZE = 1024L * 1024L * 1024L; Would it help if these were tunable parameters? (eg. startup flags) Since different clusters and workloads would have varying ranges of query memory requirements, it would help if there was some flexibility in making these ranges more/less granular as required. Just a question/suggestion. This could probably be a followup. http://gerrit.cloudera.org:8080/#/c/12244/4/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/12244/4/be/src/service/impala-http-handler.cc@882 PS4, Line 882: int64_t mem_limit; : int64_t mem_limit_for_admission; Similar question as before since num_backends can be unsigned. I am wondering why we're storing these mem limits as signed. http://gerrit.cloudera.org:8080/#/c/12244/4/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/12244/4/tests/webserver/test_web_pages.py@383 PS4, Line 383: """Sanity check for the admission debug page's http end points (both admission and : reset stats end points).""" Would it be possible to create a long running query which takes up all the resources in a pool and then add submit another query to it. That way the test could verify that the queued queries are displayed as expected and also the queuing reason is added to the page. -- To view, visit http://gerrit.cloudera.org:8080/12244 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iff055d9709ea1bcc2f492adcde92241b6149f766 Gerrit-Change-Number: 12244 Gerrit-PatchSet: 4 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Pooja Nilangekar <pooja.nilange...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 30 Jan 2019 19:38:07 +0000 Gerrit-HasComments: Yes
