impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3882: Simplify some query exec state locking
Date Fri, 04 Nov 2016 06:18:10 GMT
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3882: Simplify some query exec state locking
......................................................................


Patch Set 4:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4935/4/be/src/service/impala-beeswax-server.cc
File be/src/service/impala-beeswax-server.cc:

Line 279:   lock_guard<mutex> l(*exec_state->lock());
There still is a race between GetQES() and this lock(). What if it the query gets unregistered
and QES->Done() is called?

We still hold a ref to the shared_ptr, but wouldn't a lot of the state be torn down?

Same for every other place this happens.


http://gerrit.cloudera.org:8080/#/c/4935/4/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

PS4, Line 708: shared_ptr<QueryExecState> exec_state = GetQueryExecState(query_id);
If this line is moved above L701, we wouldn't need to call GetQueryExecState() twice (GetSessionIdForQuery()
calls it too).

We can get the QES at L701 and just grab the session ID from that object.

Also, it doesn't seem like GetSessionIdForQuery() is used anywhere else. So, we might as well
get rid of it after this refactor.


http://gerrit.cloudera.org:8080/#/c/4935/4/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

Line 789:   RETURN_IF_ERROR((*exec_state)->ExecQuery());
What happens if Cancel() was called before we reach here?

Previously holding the exec_state->lock() would have saved us.


http://gerrit.cloudera.org:8080/#/c/4935/4/be/src/service/query-exec-state.cc
File be/src/service/query-exec-state.cc:

PS4, Line 586: Like the other Exec(), fill out as much profile information as we're able to.
Update comment.


PS4, Line 594: SetPlanningFinished();
Why call SetPlanningFinished() here?


Line 620:       block_on_wait_cv_.wait(l);
Should we switch to promises instead of cond vars here?


-- 
To view, visit http://gerrit.cloudera.org:8080/4935
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I516357d2b5e9eb83e8209872cbe4c078c778a629
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message