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 <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
