[Impala-ASF-CR] IMPALA-4411: Kudu inserts violate lock ordering and could deadlock
Internal Jenkins has posted comments on this change. Change subject: IMPALA-4411: Kudu inserts violate lock ordering and could deadlock .. Patch Set 3: Verified+1 -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4411: Kudu inserts violate lock ordering and could deadlock
Internal Jenkins has submitted this change and it was merged. Change subject: IMPALA-4411: Kudu inserts violate lock ordering and could deadlock .. IMPALA-4411: Kudu inserts violate lock ordering and could deadlock This fixes a potential hang because the code took QueryExecState::lock_ before SessionState::lock, which is the wrong order. A hang has not yet been observed. The code that was taking the session lock doesn't actually need to be holding the QueryExecState lock_, so this is easy to fix by moving the relevant code. Testing: Running an exhaustive test job, and stress tests manually. Change-Id: I2aa36fce78525a80a6b880e1b668105006bc1425 Reviewed-on: http://gerrit.cloudera.org:8080/4895 Reviewed-by: Matthew Jacobs Tested-by: Internal Jenkins --- M be/src/service/query-exec-state.cc 1 file changed, 16 insertions(+), 9 deletions(-) Approvals: Matthew Jacobs: Looks good to me, approved Internal Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/4895 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I2aa36fce78525a80a6b880e1b668105006bc1425 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4411: Kudu inserts violate lock ordering and could deadlock
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4411: Kudu inserts violate lock ordering and could deadlock .. Patch Set 3: Code-Review+2 rebase -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4411: Kudu inserts violate lock ordering and could deadlock
Internal Jenkins has posted comments on this change. Change subject: IMPALA-4411: Kudu inserts violate lock ordering and could deadlock .. Patch Set 2: Verified-1 Build failed: http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/389/ -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4411: Kudu inserts violate lock ordering and could deadlock
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4411: Kudu inserts violate lock ordering and could deadlock .. Patch Set 2: Code-Review+2 carrying +2 -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4411: Kudu inserts violate lock ordering and could deadlock
Hello Henry Robinson, Sailesh Mukil, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4895 to look at the new patch set (#2). Change subject: IMPALA-4411: Kudu inserts violate lock ordering and could deadlock .. IMPALA-4411: Kudu inserts violate lock ordering and could deadlock This fixes a potential hang because the code took QueryExecState::lock_ before SessionState::lock, which is the wrong order. A hang has not yet been observed. The code that was taking the session lock doesn't actually need to be holding the QueryExecState lock_, so this is easy to fix by moving the relevant code. Testing: Running an exhaustive test job, and stress tests manually. Change-Id: I2aa36fce78525a80a6b880e1b668105006bc1425 --- M be/src/service/query-exec-state.cc 1 file changed, 16 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/95/4895/2 -- To view, visit http://gerrit.cloudera.org:8080/4895 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2aa36fce78525a80a6b880e1b668105006bc1425 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4411: Kudu inserts violate lock ordering and could deadlock
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4411: Kudu inserts violate lock ordering and could deadlock .. Patch Set 1: (3 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 I wasn't able to reproduce it easily. I tried inserting sleeps and running stress and cancellation tests, but no luck. I think it may have required a rare combination of events across a number of threads, e.g. like the one mentioned by Tim in his related patch for fixing another case of mis-ordering the same locks. 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? Done 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 ( Done -- 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 Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4411: Kudu inserts violate lock ordering and could deadlock
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( : 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 Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4411: Kudu inserts violate lock ordering and could deadlock
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4411: Kudu inserts violate lock ordering and could deadlock .. Patch Set 1: Code-Review+1 (1 comment) 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 537: session_->kudu_latest_observed_ts = std::max( : session_->kudu_latest_observed_ts, latest_kudu_ts); > It's a good idea, though I don't think this lock is particularly highly con I just grepped through the codebase and one call in particular seems to be made in many places. "WithSession()". That function gets the session_ lock. But if it seems like an over-optimization for now, I'm fine with leaving it as it is. -- 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 Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4411: Kudu inserts violate lock ordering and could deadlock
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4411: Kudu inserts violate lock ordering and could deadlock .. Patch Set 1: (1 comment) 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 537: session_->kudu_latest_observed_ts = std::max( : session_->kudu_latest_observed_ts, latest_kudu_ts); > Just a suggestion, if we make this a CAS loop, we can avoid taking the sess It's a good idea, though I don't think this lock is particularly highly contended. Are there some uses in particular that you're thinking of? -- 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 Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4411: Kudu inserts violate lock ordering and could deadlock
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4411: Kudu inserts violate lock ordering and could deadlock .. Patch Set 1: (1 comment) 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 537: session_->kudu_latest_observed_ts = std::max( : session_->kudu_latest_observed_ts, latest_kudu_ts); Just a suggestion, if we make this a CAS loop, we can avoid taking the session lock, as that looks like a highly contended lock. Also, since this is on teardown, the performance impact shouldn't matter as much. What do you think? -- 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 Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4411: Kudu inserts violate lock ordering and could deadlock
Tim Armstrong has posted comments on this change. Change subject: IMPALA-4411: Kudu inserts violate lock ordering and could deadlock .. Patch Set 1: Code-Review+1 Code change looks good assuming tests pass -- 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 Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4411: Kudu inserts violate lock ordering and could deadlock
Matthew Jacobs has uploaded a new change for review. http://gerrit.cloudera.org:8080/4895 Change subject: IMPALA-4411: Kudu inserts violate lock ordering and could deadlock .. IMPALA-4411: Kudu inserts violate lock ordering and could deadlock This fixes a potential hang because the code took QueryExecState::lock_ before SessionState::lock, which is the wrong order. A hang has not yet been observed. The code that was taking the session lock doesn't actually need to be holding the QueryExecState lock_, so this is easy to fix by moving the relevant code. Testing: Running an exhaustive test job, and stress tests manually. Change-Id: I2aa36fce78525a80a6b880e1b668105006bc1425 --- M be/src/service/query-exec-state.cc 1 file changed, 12 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/95/4895/1 -- To view, visit http://gerrit.cloudera.org:8080/4895 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I2aa36fce78525a80a6b880e1b668105006bc1425 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs
