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 debug webpage
Date Wed, 19 Oct 2016 19:51:51 GMT
Matthew Jacobs has posted comments on this change.

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


Patch Set 1:

(7 comments)

Nice! This will be useful to a lot of people.

http://gerrit.cloudera.org:8080/#/c/4756/1//COMMIT_MSG
Commit Message:

PS1, Line 7: the
the queries debug page


http://gerrit.cloudera.org:8080/#/c/4756/1/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

Line 458:   schedule->summary_profile()->AddInfoString(PROFILE_INFO_KEY_ADMISSION_RESULT,
here would be a good place to add an event to indicate Queued


http://gerrit.cloudera.org:8080/#/c/4756/1/be/src/scheduling/query-schedule.h
File be/src/scheduling/query-schedule.h:

PS1, Line 236:   bool is_queued() const {
             :     const std::string* result =
             :         summary_profile_->GetInfoString(PROFILE_INFO_KEY_ADMISSION_RESULT);
             :     return result == NULL ? false : *result == PROFILE_INFO_VAL_QUEUED;
             :   }
Inspecting the info string is hacky -- but we can avoid having to provide this interface if
the query events include an event right before queueing. Then please move the const strings
back to AC as well.


http://gerrit.cloudera.org:8080/#/c/4756/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS1, Line 783: Start execution
This ends up being misleading when planning takes a while. I think it'll be better to change
this to "Registered", and probably to move it closer to line 802 (it shouldn't matter if it's
here or right before RegisterQuery()).


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
we should get input from a few others about naming. 'scheduling' is somewhat overloaded so
we may want to change it here and elsewhere.


PS1, Line 41:     <th>Is Queued?</th>
Instead of this printed true/false in a separate column, can you add an event to indicate
the query is getting queued? Then it'll show up in Last Event. I think that makes sense there,
especially since queuing isn't independent of the events.


PS1, Line 151: <h3>Last {{completed_log_size}} Completed Queries</h3>
We should show the resource pool in this table 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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message