Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10060 )
Change subject: IMPALA-5216: Make admission control queuing async ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/10060/5/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/10060/5/be/src/service/client-request-state.h@151 PS5, Line 151: /// that these updates reach the coordinator object after Exec() has been called on it. > Are these comments stale? It seems like these functions are just straightfo not stale, but you are right, they can be worded better. http://gerrit.cloudera.org:8080/#/c/10060/5/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/10060/5/be/src/service/client-request-state.cc@735 PS5, Line 735: || operation_state_ < operation_state) > Need braces for multi-line if. Done http://gerrit.cloudera.org:8080/#/c/10060/5/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10060/5/be/src/service/impala-server.cc@673 PS5, Line 673: "Query id $0 has not started any backends yet.", PrintId(query_id)); > Does this interact with live_summary=1 in the shell at all? nice catch. As discussed, returning an empty ExecSummary object instead http://gerrit.cloudera.org:8080/#/c/10060/5/tests/authorization/test_authorization.py File tests/authorization/test_authorization.py: http://gerrit.cloudera.org:8080/#/c/10060/5/tests/authorization/test_authorization.py@299 PS5, Line 299: self.wait_for_admission_control(execute_statement_resp.operationHandle) based on the change that an empty ExecSummary is returned if in PENDING state then just an OK response from GetExecSummary means that it was successful and user has access. Therefore removing the need to wait for admission control here. http://gerrit.cloudera.org:8080/#/c/10060/5/tests/authorization/test_authorization.py@330 PS5, Line 330: def wait_for_admission_control(self, operation_handle): : """Waits for the admission control processing of the query to complete by polling : GetOperationStatus""" : from tests.hs2.test_hs2 import TestHS2 : while True: : get_operation_status_req = TCLIService.TGetOperationStatusReq() : get_operation_status_req.operationHandle = operation_handle : get_operation_status_resp = \ : self.hs2_client.GetOperationStatus(get_operation_status_req) : TestHS2.check_response(get_operation_status_resp) : if TCLIService.TOperationState.INITIALIZED_STATE < \ : get_operation_status_resp.operationState < \ : TCLIService.TOperationState.PENDING_STATE: : return get_operation_status_resp : sleep(0.05) removed due to reason mentioned above http://gerrit.cloudera.org:8080/#/c/10060/5/tests/hs2/test_hs2.py File tests/hs2/test_hs2.py: http://gerrit.cloudera.org:8080/#/c/10060/5/tests/hs2/test_hs2.py@456 PS5, Line 456: # Wait for query to start running. > Should we try fetching the summary before admission control finishes? To e based on the change that an empty ExecSummary is returned if in PENDING state, there will no longer be an error path. Instead, moving the wait_for_admission_control() call after Line 467 as before that the GetExecSummary() call should be successful in both PENDING and RUNNING state. -- To view, visit http://gerrit.cloudera.org:8080/10060 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I989cf5b259afb8f5bc5c35590c94961c81ce88bf Gerrit-Change-Number: 10060 Gerrit-PatchSet: 5 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 07 May 2018 17:48:46 +0000 Gerrit-HasComments: Yes
