Sailesh Mukil has posted comments on this change. Change subject: IMPALA-3882: Simplify some query exec state locking ......................................................................
Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/4935/4/be/src/service/impala-beeswax-server.cc File be/src/service/impala-beeswax-server.cc: Line 279: lock_guard<mutex> l(*exec_state->lock()); There still is a race between GetQES() and this lock(). What if it the query gets unregistered and QES->Done() is called? We still hold a ref to the shared_ptr, but wouldn't a lot of the state be torn down? Same for every other place this happens. http://gerrit.cloudera.org:8080/#/c/4935/4/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: PS4, Line 708: shared_ptr<QueryExecState> exec_state = GetQueryExecState(query_id); If this line is moved above L701, we wouldn't need to call GetQueryExecState() twice (GetSessionIdForQuery() calls it too). We can get the QES at L701 and just grab the session ID from that object. Also, it doesn't seem like GetSessionIdForQuery() is used anywhere else. So, we might as well get rid of it after this refactor. http://gerrit.cloudera.org:8080/#/c/4935/4/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 789: RETURN_IF_ERROR((*exec_state)->ExecQuery()); What happens if Cancel() was called before we reach here? Previously holding the exec_state->lock() would have saved us. http://gerrit.cloudera.org:8080/#/c/4935/4/be/src/service/query-exec-state.cc File be/src/service/query-exec-state.cc: PS4, Line 586: Like the other Exec(), fill out as much profile information as we're able to. Update comment. PS4, Line 594: SetPlanningFinished(); Why call SetPlanningFinished() here? Line 620: block_on_wait_cv_.wait(l); Should we switch to promises instead of cond vars here? -- To view, visit http://gerrit.cloudera.org:8080/4935 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I516357d2b5e9eb83e8209872cbe4c078c778a629 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
