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 <tmarsh...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-HasComments: Yes
