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 Fri, 21 Oct 2016 22:01:21 GMT
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 <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