Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................


Patch Set 3:

(8 comments)

As discussed, eliminated the separate list on the webpage in favor of just 
having the "Queued" event

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:
Done


PS2, Line 900: // TODO-MT: call AdmitQuery()
> this should also have set_is_admitted(true) until this TODO is resolved.
Done


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: _t wait
> now 'waiting' is a bit ambiguous... can you rename to waiting_for_close now
Done


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:   /// Resource pool associated with this query, or an empty 
string if the schedule has not
             :   /// been created and had the pool set yet.
             :   std::string request_pool() const {
             :     return schedule_ == NULL ? "" : schedule_->request_pool();
             :   }
             :   int num_rows_fetched() const { return num_rows_fetched_; }
             :   void set_fetched_rows() { fetched_rows_ = true; }
             :   bool fetched_rows() const { return fetched_rows_; }
             :   b
> I think this is big enough to move it out of the header.
Done


PS2, Line 166:   const TResultSetMetadata* result_metadata() { return 
&result_metadata_; }
             :   const TUniqueId& query_id() const { return 
query_ctx_.query_id; }
             :   c
> comment here too, when does this return an empty string?
Done


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_queries_page
> the metrics page exposes json too (and 'metrics' is a bit overloaded), so w
Done


PS2, Line 603: the number of queries currently in the 'admit
> the number of queries currently in the 'admitted' and 'queued' states
Done


PS2, Line 710:       if thread.error is not None:
             :         raise thread.error
             : 
> here's where it'd be helpful to differentiate between current counts (i.e. 
Done


-- 
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: 3
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

Reply via email to