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 Mon, 24 Oct 2016 23:09:51 GMT
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 <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