Tim Armstrong has posted comments on this change. Change subject: IMPALA-3882: Simplify some query exec state locking ......................................................................
Patch Set 4: (3 comments) I did an initial pass over it. I'm still struggling with reasoning about whether this introduces new dangerous races - the get_metadata() one is obviously fixed, but it's difficult to anticipate what other races may be possible. If you have any suggestions about how to approach this it would be welcome. Have you done any stress testing of this? It would be good to do some local stress testing against the minicluster. E.g. ./tests/stress/concurrent_select.py --cancel-probability=0.5 --max-queries 10000 --tpch-db=tpch_parquet --fail-upon-successive-errors=100 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 625: .get() I don't feel that strongly, but comparisons with NULL for scoped_ptr/unique_ptr/shared_ptr should work without the .get(). Line 641: return; Unneeded return. http://gerrit.cloudera.org:8080/#/c/4935/4/be/src/service/impala-server.h File be/src/service/impala-server.h: Line 82: /// 2. session_state_map_lock_ Curious - do we have a similar problem with the session map lock? There's a somewhat similar pattern in GetSessionState() to the one this patch fixes. -- 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 <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
