Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12244 )
Change subject: IMPALA-8092: Add an admission controller debug page ...................................................................... Patch Set 1: (19 comments) This is cool - I checked it out and played around with it a bit. I think we can improve the ergonomics a bit, but I really liked just being able to see what's going on in the admission controller in real time. http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/scheduling/admission-controller.h@403 PS1, Line 403: void ResetStats(); We might want to rename this - it seems like this only resets informational statistics, not statistics that are used by the admission control algorithms. Not sure if there's a better name than "Informational" http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/scheduling/admission-controller.h@449 PS1, Line 449: std::vector<int64_t> peak_mem_histogram_; > just looked at HdrHistogram, it seems like a full blown implementation of a Yeah I took a look too and it seemed non-trivial to use. I'm fine with leaving as-is for now. http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/scheduling/admission-controller.h@455 PS1, Line 455: EMA I think lower-case ema is more consistent with elsewhere. http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/scheduling/admission-controller.h@593 PS1, Line 593: Query lower case? http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/scheduling/admission-controller.cc@1098 PS1, Line 1098: bind<bool>(QueueNodeToJsonHelperCallback, &queued_queries, document, _1)); Use a lambda? http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/service/impala-http-handler.cc@145 PS1, Line 145: admission_controller maybe just /admission to be more concise? http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/service/impala-http-handler.cc@148 PS1, Line 148: resource_pool_reset This shows up at the top of the debug page now. I think you need to pass in false like some of the above cases. This might be a good time to make the is_on_nav_bar argument to RegisterUrlCallback non-default. http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/service/impala-http-handler.cc@873 PS1, Line 873: QueryInfo(const TUniqueId& q_id, int64_t memory_limit, int64_t memory_limit_for_ac, We might be able to use a struct initializer {} and avoid this constructor. http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/service/impala-http-handler.cc@886 PS1, Line 886: & Consider just passing in the variables that need to be accessed, i.e. running_queries http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/service/impala-http-handler.cc@889 PS1, Line 889: if (request_state->operation_state() != TOperationState::INITIALIZED_STATE I think we probably want to read operation_state() once and put it in a local rather than on two branches of the condition. It's hard to otherwise reason about what might happen if the state changes in-between the conditions being evaluated. http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/service/impala-http-handler.cc@923 PS1, Line 923: // In order to embed a plain json inside the webpage generated by mustache, we need This is a little unfortunate but I think we can live with it. http://gerrit.cloudera.org:8080/#/c/12244/1/www/admission_controller.tmpl File www/admission_controller.tmpl: PS1: It would be nice if we had a way to show only a single resource pool on a page (maybe just with an argument to filter them). Then if the pool headings linked to those pages. I'm thinking it would be convenient to be able to look at a single page and F5 it. http://gerrit.cloudera.org:8080/#/c/12244/1/www/admission_controller.tmpl@194 PS1, Line 194: <p class="lead">This page lists all resource pools are their corresponding stats.</p> It actually only lists resource pools that queries have been submitted to (since they are lazily created). http://gerrit.cloudera.org:8080/#/c/12244/1/www/admission_controller.tmpl@210 PS1, Line 210: Requests I think "Max Concurrent Queries" would be more intuitive (we use "Queries" elsewhere in the page. http://gerrit.cloudera.org:8080/#/c/12244/1/www/admission_controller.tmpl@223 PS1, Line 223: min_query_mem_limit should be max http://gerrit.cloudera.org:8080/#/c/12244/1/www/admission_controller.tmpl@231 PS1, Line 231: local maybe "submitted to this coordinator" instead of "local to this coordinator" - might match user-facing concepts better. http://gerrit.cloudera.org:8080/#/c/12244/1/www/admission_controller.tmpl@283 PS1, Line 283: <td>Queries Currently Running</td> Is is reasonably easy to show the total number of queries that were submitted to the pool historically? http://gerrit.cloudera.org:8080/#/c/12244/1/www/admission_controller.tmpl@293 PS1, Line 293: <td>Total Memory Reserved</td> Might be worth being more verbose here - "Total Memory Reserved Across Cluster" http://gerrit.cloudera.org:8080/#/c/12244/1/www/admission_controller.tmpl@298 PS1, Line 298: <td>Local Memory Admitted</td> Maybe "Memory Admitted on this Coordinator" -- 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: 1 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, 23 Jan 2019 20:35:30 +0000 Gerrit-HasComments: Yes
