Dan Hecht has posted comments on this change. Change subject: IMPALA-1972/IMPALA-3882: [DRAFT] Don't hold QueryExecState::lock_ during planning ......................................................................
Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/6707/2//COMMIT_MSG Commit Message: PS2, Line 7: Don't hold QueryExecState::lock_ during planning let's not actually solve that problem right now. it doesn't seem necessary for IMPALA-1972 which is the large pain. Instead, the other two changes should be sufficient: - Never block on QES::lock_ while holding the map lock. This breaks the cycle that leads to one query blocking another query. - Don't try to take QES::lock_ from the webserver if the query is still planning. This deals with the webserver freezing when metadata loading takes a long time. Line 11: the locking is the responsibility of the callers. Yes, but please double check this is safe. From code inspection, I believe it is. Line 18: set after the ExecState is built. As stated above, let's not do that. But please add a short snippet about the webserver change. Line 22: and run parallel queries. As we discussed, please see if you can add a test that: 1) Executes a query and simulates getting stuck in planning. 2) Hits the debug webserver. Before this fix, it will hang. 3) Executes a second query. Before this fix, this will also get stuck. 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: // Convert TResultSetMetadata to Beeswax.ResultsMetadata Let's take the QES::lock_ here and not do the promise thing. 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: // Convert TResultSetMetadata to TGetResultSetMetadataResp same 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->query_state() == beeswax::QueryState::CREATED) return; please fill in those details. http://gerrit.cloudera.org:8080/#/c/6707/2/be/src/service/impala-server.h File be/src/service/impala-server.h: PS2, Line 296: Callers are responsible : /// for acquiring QueryExecState::lock_ before performing any operations on it. I think this comment just adds confusion. It's up to the QueryExecState class to explain its synchronization. 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: planning_finished_promise_ we won't need that. -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-HasComments: Yes
