Henry Robinson has posted comments on this change. Change subject: IMPALA-4411: Kudu inserts violate lock ordering and could deadlock ......................................................................
Patch Set 1: Code-Review+2 (4 comments) http://gerrit.cloudera.org:8080/#/c/4895/1//COMMIT_MSG Commit Message: PS1, Line 17: Running an exhaustive test job, and stress tests : manually. Did you reproduce the hang? It should be easy to do if you add a sleep call after the acquisition of the session lock, and then issue some other operation that takes the locks in the right order. No worries if not - just not a bad idea to confirm analysis of the lock ordering, even if it's pretty self-evident like in this case. http://gerrit.cloudera.org:8080/#/c/4895/1/be/src/service/query-exec-state.cc File be/src/service/query-exec-state.cc: PS1, Line 532: // This should be safe to access on coord_ after Wait() has been called. This is a bit vague. Is it safe or not? PS1, Line 535: "Updating session latest observed Kudu timestamp: " << latest_kudu_ts Add the session ID here, otherwise this message is not going to be useful (won't be able to tell which session has which timestamp). PS1, Line 537: session_->kudu_latest_observed_ts = std::max<uint64_t>( : session_->kudu_latest_observed_ts, latest_kudu_ts); > I just grepped through the codebase and one call in particular seems to be WithSession() usually gets called at the start of RPCs, which means it probably isn't very contended because there's usually only one RPC in flight at once. Not worth adding more complicated logic for. -- To view, visit http://gerrit.cloudera.org:8080/4895 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2aa36fce78525a80a6b880e1b668105006bc1425 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
