impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Date Thu, 20 Oct 2016 20:40:33 GMT
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 <tmarshall@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message