You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org> on 2016/10/19 17:05:37 UTC
[Impala-ASF-CR] IMPALA-1169: Admission control info on the debug webpage
Thomas Tauber-Marshall has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4756
Change subject: IMPALA-1169: Admission control info on the debug webpage
......................................................................
IMPALA-1169: Admission control info on the debug webpage
Previously, the /queries webpage showed three sets of queries -
currently in flight, waiting to be closed, and finished. This
patch adds a third category, not yet scheduled, which contains
queries that would have previously been shown as in flight, but
that haven't actually started executing yet because they are
still in planning or have been queued by the admission
controller.
It also adds a column to show the resource pool associated with
any running queries, and includes a column for the not scheduled
queries indicating if they are currently queued by the admission
controller.
These changes make it easier to monitor the admission control
decisions from the debug webpage.
Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/simple-scheduler.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-exec-state.h
M tests/common/impala_service.py
M tests/custom_cluster/test_admission_controller.py
M www/queries.tmpl
10 files changed, 145 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/4756/1
--
To view, visit http://gerrit.cloudera.org:8080/4756
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
IMPALA-1169: Admission control info on the queries debug webpage
This patch adds a new event, 'Queued', to the query event log to
indicate when a query is queued by the admission controller. This
means that queries on the '/queries' page that are currently
queued will display this as their 'Last Event', making it possible
to see which queries are current queued.
It also adds a column to show the resource pool associated with
the queries, and it updates the wording of the first event that
gets marked for each query from 'Start execution' to 'Query
submitted', since this is before planning and admission control
and therefore execution hasn't actually startd yet.
Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
Reviewed-on: http://gerrit.cloudera.org:8080/4756
Reviewed-by: Matthew Jacobs <mj...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/scheduling/admission-controller.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-exec-state.h
M tests/common/impala_service.py
M tests/custom_cluster/test_admission_controller.py
M www/queries.tmpl
8 files changed, 68 insertions(+), 1 deletion(-)
Approvals:
Matthew Jacobs: Looks good to me, approved
Internal Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4756
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4756/5/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
PS5, Line 801: Registered
> I don't think 'Registered' is very meaningful to users (and also it's being
I had suggested moving it to be closer to the register call if this is called Registered, but 'Query submitted' is a good and then it makes sense to put this back where it was. We mostly just wanted to avoid the confusion of using 'Start execution' before this goes through planning and AC.
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs, Internal Jenkins, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4756
to look at the new patch set (#9).
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
IMPALA-1169: Admission control info on the queries debug webpage
This patch adds a new event, 'Queued', to the query event log to
indicate when a query is queued by the admission controller. This
means that queries on the '/queries' page that are currently
queued will display this as their 'Last Event', making it possible
to see which queries are current queued.
It also adds a column to show the resource pool associated with
the queries, and it updates the wording of the first event that
gets marked for each query from 'Start execution' to 'Query
submitted', since this is before planning and admission control
and therefore execution hasn't actually startd yet.
Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
---
M be/src/scheduling/admission-controller.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-exec-state.h
M tests/common/impala_service.py
M tests/custom_cluster/test_admission_controller.py
M www/queries.tmpl
8 files changed, 68 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/4756/9
--
To view, visit http://gerrit.cloudera.org:8080/4756
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/4756/1//COMMIT_MSG
Commit Message:
PS1, Line 7: the
> the queries debug page
Done
http://gerrit.cloudera.org:8080/#/c/4756/1/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
Line 458: return Status::OK();
> here would be a good place to add an event to indicate Queued
Done
http://gerrit.cloudera.org:8080/#/c/4756/1/be/src/scheduling/query-schedule.h
File be/src/scheduling/query-schedule.h:
PS1, Line 236:
: /// The query options from the TClientRequest
: const TQueryOptions& query_options_;
: RuntimeProfile* summary_profile_;
: R
> Inspecting the info string is hacky -- but we can avoid having to provide t
Done
http://gerrit.cloudera.org:8080/#/c/4756/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
PS1, Line 783:
> This ends up being misleading when planning takes a while. I think it'll be
Done
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 somewha
Sure, maybe "not yet executing", "pending execution", or "setting up"?
PS1, Line 41: <th>Details</th>
> Instead of this printed true/false in a separate column, can you add an eve
Done
PS1, Line 151: <table class='table table-hover table-border'>
> We should show the resource pool in this table as well
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 5:
Thanks! Do the exhaustive AC tests pass?
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 4:
(3 comments)
Thanks, almost there. You should be able to test the AC tests well w/ exhaustive. I think it should run w/ something like:
cd tests
impala-py.test --exploration_strategy=exhuastive custom_cluster/test_admission_controller.py
That may take a long time.
http://gerrit.cloudera.org:8080/#/c/4756/4/tests/custom_cluster/test_admission_controller.py
File tests/custom_cluster/test_admission_controller.py:
PS4, Line 609: print(queries_json)
remove
PS4, Line 613: if query['last_event'] != 'Registered' and \
: query['last_event'] != 'Planning finished':
: assert query['resource_pool'] == self.pool_name
: else:
: assert query['resource_pool'] == '' or \
: query['resource_pool'] == self.pool_name
This else clause seems a bit weird. The logic would probably be easier if flipped, and I think we'd want to split these cases to be more precise about when is it empty vs eq to pool_name. Don't we know it will be empty if it's registered? I guess we don't know if it's in Planning Finished-though based on the points at which this is called, maybe the states are actually stable?
e.g.
if Registered:
# should be empty
elif Planning Finished:
# TODO - are we sure queries will be in this state when this fn is called in the code below? the states may be stable. if we can get here, then this would be the more specific place we don't know if its empty or set yet
else:
# now we know it'll be set
PS4, Line 620: assert query['stmt_type'] == 'DDL' or query['stmt_type'] == 'SET'
In my comment before I meant to give an example rather than something we can assert. There are other types too, i think you can leave out this assertion. The test has SET statements but checking the other cases may be misleading for readers.
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 9: Verified+1
--
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 8: Verified-1
Build failed: http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/419/
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4756/5/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
PS5, Line 801: Registered
I don't think 'Registered' is very meaningful to users (and also it's being set *before* registration, so the time is wrong.
"Query submitted" or something might be clearer. The idea is an event that shows that Impala has started processing a query. I preferred it in its earlier place - what was the rationale for moving it?
http://gerrit.cloudera.org:8080/#/c/4756/5/be/src/service/query-exec-state.h
File be/src/service/query-exec-state.h:
Line 157: /// Resource pool associated with this query, or an empty string if the schedule has not
Blank line before this.
Line 159: // control.
missing /
PS5, Line 161: NULL
nullptr
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 5:
(3 comments)
For COMPUTE STATS, the "compute stats" query and both its child queries will be displayed on the /queries page.
The child queries operate just like regular queries, eg. they are subject to admission control and can be shown as "Queued".
The "compute stats" query is not subject to admission control, and its "Last Event" will be shown as "Planning Finished" while the child queries are running, before moving to "Child queries finished"
http://gerrit.cloudera.org:8080/#/c/4756/4/tests/custom_cluster/test_admission_controller.py
File tests/custom_cluster/test_admission_controller.py:
PS4, Line 609: queries_json = impa
> remove
Done
PS4, Line 613: assert query['last_event'] != 'Registered' and \
: query['last_event'] != 'Planning finished'
: assert query['resource_pool'] == self.pool_name
: else:
: assert query['resource_pool'] == ''
:
> This else clause seems a bit weird. The logic would probably be easier if f
Done
PS4, Line 620: turns the number of queries currently in the 'queued' state from t
> In my comment before I meant to give an example rather than something we ca
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs, Internal Jenkins, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4756
to look at the new patch set (#8).
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
IMPALA-1169: Admission control info on the queries debug webpage
This patch adds a new event, 'Queued', to the query event log to
indicate when a query is queued by the admission controller. This
means that queries on the '/queries' page that are currently
queued will display this as their 'Last Event', making it possible
to see which queries are current queued.
It also adds a column to show the resource pool associated with
the queries, and it updates the wording of the first event that
gets marked for each query from 'Start execution' to 'Query
submitted', since this is before planning and admission control
and therefore execution hasn't actually startd yet.
Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/simple-scheduler.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-exec-state.h
M tests/common/impala_service.py
M tests/custom_cluster/test_admission_controller.py
M www/queries.tmpl
9 files changed, 71 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/4756/8
--
To view, visit http://gerrit.cloudera.org:8080/4756
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 6: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4756/6/be/src/scheduling/simple-scheduler.cc
File be/src/scheduling/simple-scheduler.cc:
PS6, Line 906: !FLAGS_disable_admission_control
slightly easier to read if you reverse the if-branches and remove the ! to get rid of the double negative.
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 8: Code-Review+2
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 7: Verified-1
Build failed: http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/369/
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4756/6/be/src/scheduling/simple-scheduler.cc
File be/src/scheduling/simple-scheduler.cc:
PS6, Line 906: TURN_IF_ERROR(admission_controll
> slightly easier to read if you reverse the if-branches and remove the ! to
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 6: Code-Review+1
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has uploaded a new patch set (#4).
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
IMPALA-1169: Admission control info on the queries debug webpage
This patch adds a new event, 'Queued', to the query event log to
indicate when a query is queued by the admission controller. This
means that queries on the '/queries' page that are currently
queued will display this as their 'Last Event', making it possible
to see which queries are current queued.
It also adds a column to show the resource pool associated with
the queries.
Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/simple-scheduler.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-exec-state.h
M tests/common/impala_service.py
M tests/custom_cluster/test_admission_controller.py
M www/queries.tmpl
9 files changed, 75 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/4756/4
--
To view, visit http://gerrit.cloudera.org:8080/4756
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has uploaded a new patch set (#6).
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
IMPALA-1169: Admission control info on the queries debug webpage
This patch adds a new event, 'Queued', to the query event log to
indicate when a query is queued by the admission controller. This
means that queries on the '/queries' page that are currently
queued will display this as their 'Last Event', making it possible
to see which queries are current queued.
It also adds a column to show the resource pool associated with
the queries, and it updates the wording of the first event that
gets marked for each query from 'Start execution' to 'Query
submitted', since this is before planning and admission control
and therefore execution hasn't actually startd yet.
Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/simple-scheduler.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-exec-state.h
M tests/common/impala_service.py
M tests/custom_cluster/test_admission_controller.py
M www/queries.tmpl
9 files changed, 72 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/4756/6
--
To view, visit http://gerrit.cloudera.org:8080/4756
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 3:
(9 comments)
even w/o the new section, how does compute stats behave?
http://gerrit.cloudera.org:8080/#/c/4756/3/be/src/scheduling/query-schedule.cc
File be/src/scheduling/query-schedule.cc:
Line 56: request_pool_(""),
was this intentional? i don't see why this is necessary
http://gerrit.cloudera.org:8080/#/c/4756/3/be/src/scheduling/simple-scheduler.cc
File be/src/scheduling/simple-scheduler.cc:
Line 900: // TODO-MT: call AdmitQuery()
call AdmitQuery() rather than always setting is_admitted
http://gerrit.cloudera.org:8080/#/c/4756/3/be/src/scheduling/simple-scheduler.h
File be/src/scheduling/simple-scheduler.h:
Line 92: /// If Schedule() returns OK, schedule::is_admitted will be true.
Sorry, on second thought this doesn't add much. There's a comment in the base class where this information might be better suited, but I can't think of a way to phrase this right now that I think makes it worthwhile.
http://gerrit.cloudera.org:8080/#/c/4756/3/be/src/service/query-exec-state.h
File be/src/service/query-exec-state.h:
PS3, Line 158: and had the pool set yet.
, not had the pool set yet, or this is StmtType doesn't go through admission control.
http://gerrit.cloudera.org:8080/#/c/4756/3/tests/common/impala_service.py
File tests/common/impala_service.py:
PS3, Line 83: def get_webpage_json(self, page_name):
: """Returns the json for the given Impala debug webpage, eg. '/queries'"""
: return json.loads(self.read_debug_webpage(page_name + "?json"))
can you move this up to be under read_debug_webpage and call it get_debug_webpage_json?
http://gerrit.cloudera.org:8080/#/c/4756/3/tests/custom_cluster/test_admission_controller.py
File tests/custom_cluster/test_admission_controller.py:
PS3, Line 603: the 'admitted' and 'queued' states
outdated
PS3, Line 658: metrics from the webpage
counts from the queries page
Line 663:
would be nice to add a fn that gets the queries page json and checks that all queries (both in flight and completed) either:
a) have query type QUERY or DML and the pool is self.pool_name
OR
b) has an empty pool name (I think it'd be DDL or SET)
I think the results should be stable to check here (they're not making state transitions)
Line 708:
the function to check the pool names could be called here too
--
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 <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 8:
Why did the gvo fail? Were there any code changes in the latest patch set or just a rebase?
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/4756/3/be/src/scheduling/query-schedule.cc
File be/src/scheduling/query-schedule.cc:
Line 56: is_admitted_(false) {
> was this intentional? i don't see why this is necessary
Done
http://gerrit.cloudera.org:8080/#/c/4756/3/be/src/scheduling/simple-scheduler.cc
File be/src/scheduling/simple-scheduler.cc:
Line 900: // TODO-MT: call AdmitQuery() rather than always setting is_admitted
> call AdmitQuery() rather than always setting is_admitted
Done
http://gerrit.cloudera.org:8080/#/c/4756/3/be/src/scheduling/simple-scheduler.h
File be/src/scheduling/simple-scheduler.h:
Line 92: virtual Status Schedule(QuerySchedule* schedule);
> Sorry, on second thought this doesn't add much. There's a comment in the ba
Done
http://gerrit.cloudera.org:8080/#/c/4756/3/be/src/service/query-exec-state.h
File be/src/service/query-exec-state.h:
PS3, Line 158: and had the pool set yet,
> , not had the pool set yet, or this is StmtType doesn't go through admissio
Done
http://gerrit.cloudera.org:8080/#/c/4756/3/tests/common/impala_service.py
File tests/common/impala_service.py:
PS3, Line 83: sleep(interval)
: assert 0, 'Metric value %s did not reach value %s in %ss' %\
: (metric_name, expected_value, timeout)
> can you move this up to be under read_debug_webpage and call it get_debug_w
Done
http://gerrit.cloudera.org:8080/#/c/4756/3/tests/custom_cluster/test_admission_controller.py
File tests/custom_cluster/test_admission_controller.py:
PS3, Line 603:
> outdated
Done
PS3, Line 658:
> counts from the queries page
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has uploaded a new patch set (#5).
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
IMPALA-1169: Admission control info on the queries debug webpage
This patch adds a new event, 'Queued', to the query event log to
indicate when a query is queued by the admission controller. This
means that queries on the '/queries' page that are currently
queued will display this as their 'Last Event', making it possible
to see which queries are current queued.
It also adds a column to show the resource pool associated with
the queries.
Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/simple-scheduler.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-exec-state.h
M tests/common/impala_service.py
M tests/custom_cluster/test_admission_controller.py
M www/queries.tmpl
9 files changed, 71 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/4756/5
--
To view, visit http://gerrit.cloudera.org:8080/4756
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 9:
The reason the GVO failed was the change in this patch that always calls QuerySchedule::set_is_admitted(true) for query schedules that don't go through AdmissionController::AdmitQuery. This was causing AdmissionController::ReleaseQuery to try to release queries that had never been subject to admission control, resulting in a crash.
Originally, this was fine because the only condition where AdmitQuery wouldn't be called was when FLAG_disable_admission_control was false, which would also prevent ReleaseQuery from being called. However, a recent change added 'is_mt_execution' as a condition for skipping AdmitQuery but not as a condition for skipping ReleaseQuery, leading to the crash.
Also originally, setting is_admitted to true when FLAG_disable_admission_control to true was necessary as we were relying on QuerySchedule::is_admitted to differentiate queries that were not scheduled/queued/etc, even when admission control is off. With the change to just using the query event log for differentiating not scheduled/queued/etc. states, this is no longer necessary, so to fix the crash I just removed it.
--
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has uploaded a new patch set (#3).
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
IMPALA-1169: Admission control info on the queries debug webpage
This patch adds a new event, 'Queued', to the query event log to
indicate when a query is queued by the admission controller. This
means that queries on the '/queries' page that are currently
queued will display this as their 'Last Event', making it possible
to see which queries are current queued.
It also adds a column to show the resource pool associated with
the queries.
Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/query-schedule.cc
M be/src/scheduling/simple-scheduler.cc
M be/src/scheduling/simple-scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-exec-state.h
M tests/common/impala_service.py
M tests/custom_cluster/test_admission_controller.py
M www/queries.tmpl
11 files changed, 52 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/4756/3
--
To view, visit http://gerrit.cloudera.org:8080/4756
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 9: Code-Review+2
--
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 4:
Also I had a previous question about how COMPUTE STATS behaves.
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4756/5/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
PS5, Line 801: o handle i
> I had suggested moving it to be closer to the register call if this is call
Done
http://gerrit.cloudera.org:8080/#/c/4756/5/be/src/service/query-exec-state.h
File be/src/service/query-exec-state.h:
Line 157:
> Blank line before this.
Done
Line 159: /// been created and had the pool set yet, or this StmtType doesn't go through admission
> missing /
Done
PS5, Line 161: ol()
> nullptr
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
Patch Set 8:
The GVO that failed here did so due to some of the flaky test issues we were having last week.
The latest patch set was just a rebase.
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1169: Admission control info on the debug webpage
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1169: Admission control info on the queries debug webpage
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has uploaded a new patch set (#2).
Change subject: IMPALA-1169: Admission control info on the queries debug webpage
......................................................................
IMPALA-1169: Admission control info on the queries debug webpage
Previously, the /queries webpage showed three sets of queries -
currently in flight, waiting to be closed, and finished. This
patch adds a third category, not yet scheduled, which contains
queries that would have previously been shown as in flight, but
that haven't actually started executing yet because they are
still in planning or have been queued by the admission
controller.
It also adds a column to show the resource pool associated with
any running queries, and includes a column for the not scheduled
queries indicating if they are currently queued by the admission
controller.
These changes make it easier to monitor the admission control
decisions from the debug webpage.
Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/simple-scheduler.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-exec-state.h
M tests/common/impala_service.py
M tests/custom_cluster/test_admission_controller.py
M www/queries.tmpl
9 files changed, 125 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/4756/2
--
To view, visit http://gerrit.cloudera.org:8080/4756
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I504e3c829a14318721e3a42de6281bcc578f7283
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>