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 12: (3 comments) http://gerrit.cloudera.org:8080/#/c/10060/12/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/10060/12/be/src/service/client-request-state.h@319 PS12, Line 319: async_ > the async part seems redundant with "thread" to me, and inconsistent with o The "async" part i added was only to differentiate it from the thread that is executing the client Exec RPC and to make it sound similar to the method "ExecAsyncQueryOrDmlRequest()" which spawns it. http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/service/client-request-state.h@176 PS9, Line 176: or* GetCoordinator( > Maybe: coord_exec_called_, then it's very literal so less chance for other Done http://gerrit.cloudera.org:8080/#/c/10060/12/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/10060/12/be/src/service/impala-http-handler.cc@776 PS12, Line 776: request_state->operation_state() != TOperationState::ERROR_STATE > I guess that's for the case that the cancellation happens before the coordi we actually want to check for queries that go through the admission controller path and the problem is that a CTAS query is also one of them, but it is a DDL. Initially, the I was using a method "uses_admission_control()" that checked for those conditions, but I realized that a CTAS query only goes through that path if the table does not exist, which can only be determined while in CRS::Exec(). That eventually led me to define uses_admission_control() as checking for the existence of async_exec_thread_. Now that we got rid of that method, this was the next best alternative as I dont see other query types to be in this operation state without being unregistered and archived. Now that I dug more into it, I see there is a possibility where "compute states" queries can end up with ERROR_STATE, while the client does not close the query handle. In that case the highlighted conditional will let it pass, but would give the same output as that taken from an archived query, so seems harmless -- 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: 12 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: Tue, 22 May 2018 22:43:16 +0000 Gerrit-HasComments: Yes
