impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4411: Kudu inserts violate lock ordering and could deadlock
Date Tue, 01 Nov 2016 04:43:48 GMT
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 <mj@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message