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 8: (3 comments) Just wanted to get the replay regarding 'coord_' out there. Will address the remaining comments in the next iteration. http://gerrit.cloudera.org:8080/#/c/10060/7/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/10060/7/be/src/service/client-request-state.h@172 PS7, Line 172: : /// Only set for 'QUERY' and 'DML' statement types and is available only after Exec() : / > If you find that there are too many entry points to CRS due to the various TLDR: I am not sure, if we can entirely get rid of most of the confusing usage around coord_ with the cleanup (since I have not done that analysis yet) but currently I dont think it would improve things much if we abstract away access to coord_ Summary: It looks like in the codebase, GetCoordinator() is called for coord_ in 3 differnt states: 1. null 2. initialized but Exec() not called yet 3. Exec() successfully called One way to figure these states out if we get rid of the atomic bool is: 1. null -> hold CRS::lock, check if coord_ is nullptr 2. initialized but Exec() not called yet -> the null check in #1 plus hold the Coordinator::lock_, add a new Coordinator ExecState(INITIALIZED, EXECUTING,...) and check if Coordinator->ExecState is INITIALIZED 3. Exec() successfully called, all the above plus check coord_ ExecState We are currently only concerned with dealing with coord_ if it is in either state #1 or #3. 'coord_exec_started_' is currently serving that purpose, by only making it visible through GetCoordinator() when it reaches state #3. If we remove 'coord_exec_started_', we would have to hold CRS::lock_ and Coordinator::lock_ everytime we access it. If we decide to remove access to coord_ and abstract away/wrap calls to coord_, then the way calls are being made currently, we would need 6 more wrappers. If we do both, this would still look like just refactoring and would take the same amount of effort to reason about any code related to these classes. http://gerrit.cloudera.org:8080/#/c/10060/7/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/10060/7/be/src/service/client-request-state.cc@481 PS7, Line 481: ule(query_id(), > I think it would be okay. The only reason to be hesitant is if there might makes sense. Created a JIRA for this IMPALA-7038 http://gerrit.cloudera.org:8080/#/c/10060/7/be/src/service/client-request-state.cc@1155 PS7, Line 1155: > Out of curiosity, which test expects FINISHED/OK after cancellation? hs2/test_hs2.py::TestHS2::test_get_profile (the last assert) -- 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: 8 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, 15 May 2018 23:04:24 +0000 Gerrit-HasComments: Yes
