Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-1972/IMPALA-3882: Fix QueryExecState::lock_ contention ......................................................................
Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/6707/2//COMMIT_MSG Commit Message: PS2, Line 7: yExecState::lock_ contention > let's not actually solve that problem right now. it doesn't seem necessary Done Line 11: The most common occurrence of this is while loading the webpage of a query > Yes, but please double check this is safe. From code inspection, I believe Ran a stress test and its green (Thanks Mike B.). Also I checked the callers manually and it looked ok to me too. Line 18: side effects. Instead, > As stated above, let's not do that. Done Line 22: - As an exception, we don't grab QES::lock_ while the query planning is > As we discussed, please see if you can add a test that: Not much success on this front. I'm not able to repro the scenario via tests. I added code to inject sleep simulating table loading in the exec request and loops that keep polling the web UI, but I'm not able to get the test failing on the asf-master (without this patch). May be I'm not generating enough load for lock contention to happen. Thoughts? (I've included the test in PS2). http://gerrit.cloudera.org:8080/#/c/6707/2/be/src/service/impala-beeswax-server.cc File be/src/service/impala-beeswax-server.cc: Line 197: { > Let's take the QES::lock_ here and not do the promise thing. Done. http://gerrit.cloudera.org:8080/#/c/6707/2/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: Line 741: lock_guard<mutex> l(*exec_state->lock()); > same Done http://gerrit.cloudera.org:8080/#/c/6707/2/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: Line 715: if (exec_state.get() != nullptr) { > please fill in those details. Done http://gerrit.cloudera.org:8080/#/c/6707/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: PS2, Line 814: : (*exec_state)->query_events()->MarkEvent("Query submitted"); : Changes undone as discussed. http://gerrit.cloudera.org:8080/#/c/6707/3/be/src/service/impala-server.cc File be/src/service/impala-server.cc: PS3, Line 603: // For queries in CREATED state, the profile information isn't populated yet. : if (exec_state->query_state() == beeswax::QueryState::CREATED) return Status::OK(); Wanted to double check if this change is required. Reason being, with this change, the profile doesn't show up on the web UI till the metadata is loaded. That might affect some users/clients relying on the web UI profiles to track the query state? http://gerrit.cloudera.org:8080/#/c/6707/2/be/src/service/impala-server.h File be/src/service/impala-server.h: PS2, Line 296: : /// goto http://msdn.microsoft.com/en-us/library/ms714687.aspx > I think this comment just adds confusion. It's up to the QueryExecState cl Done http://gerrit.cloudera.org:8080/#/c/6707/2/be/src/service/query-exec-state.h File be/src/service/query-exec-state.h: PS2, Line 244: > we won't need that. Done -- To view, visit http://gerrit.cloudera.org:8080/6707 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie44daa93e3ae4d04d091261f3ec4891caffe8026 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-HasComments: Yes
