Matthew Jacobs has posted comments on this change. Change subject: IMPALA-1169: Admission control info on the queries debug webpage ......................................................................
Patch Set 2: (9 comments) As we discussed in person, we should call out how this should behave for other "query types" E.g. DDL always is considered to be admitted and bypasses the new preparing(?) list. (Is that true?) What about COMPUTE STATS? Can you check for the COMPUTE STATS statement itself and see what happens with the child queries it generates (there should be 2). I'd like to capture this information clearly so we can work with the CM folks to expose things the same way in their UI. http://gerrit.cloudera.org:8080/#/c/4756/2/be/src/scheduling/simple-scheduler.cc File be/src/scheduling/simple-scheduler.cc: PS2, Line 879: Status SimpleScheduler::Schedule Can you comment in the header the post-conditions: returns with schedule is_admitted==true OR an error PS2, Line 900: // TODO-MT: call AdmitQuery() this should also have set_is_admitted(true) until this TODO is resolved. http://gerrit.cloudera.org:8080/#/c/4756/2/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: PS2, Line 348: waiting now 'waiting' is a bit ambiguous... can you rename to waiting_for_close now? http://gerrit.cloudera.org:8080/#/c/4756/2/be/src/service/query-exec-state.h File be/src/service/query-exec-state.h: PS2, Line 157: bool is_admitted() const { : if (exec_request_.stmt_type == TStmtType::QUERY || : exec_request_.stmt_type == TStmtType::DML || : (catalog_op_type() == TCatalogOpType::DDL && : ddl_type() == TDdlType::CREATE_TABLE_AS_SELECT)) { : return schedule_ == NULL ? false : schedule_->is_admitted(); : } : return true; : } I think this is big enough to move it out of the header. Also, please add a comment and mention that this is always true for X query types. PS2, Line 166: std::string request_pool() const { : return schedule_ == NULL ? "" : schedule_->request_pool(); : } comment here too, when does this return an empty string? http://gerrit.cloudera.org:8080/#/c/4756/2/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: PS2, Line 602: _get_json_metrics the metrics page exposes json too (and 'metrics' is a bit overloaded), so we should indicate this is the queries page. e.g. _get_queries_page_counts() . not the best name ever but differentiates it from metrics and its json output. PS2, Line 603: the number of 'admitted' and 'queued' queries the number of queries currently in the 'admitted' and 'queued' states (Indicates these are the current values, the metrics are monotonically increasing counters. In more metrics-y terminology that's a gauge vs a counter.) PS2, Line 710: json_metrics = self._get_json_metrics() : assert json_metrics['admitted'] == 0 : assert json_metrics['queued'] == 0 here's where it'd be helpful to differentiate between current counts (i.e. should be 0) and the counter deltas computed above (which are > 0); it's not clear why this makes it look like admitted is 0 but it is nonzero in the checks just a few lines above. Can you rename the variables and brief comment? http://gerrit.cloudera.org:8080/#/c/4756/1/www/queries.tmpl File www/queries.tmpl: PS1, Line 26: {{num_not_scheduled_queries}} queries not scheduled yet > Sure, maybe "not yet executing", "pending execution", or "setting up"? I like "Preparing for Execution", but let's wait to hear from others as well since the naming of some of the plumbing should reflect the same names as well. -- To view, visit http://gerrit.cloudera.org:8080/4756 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-HasComments: Yes
