impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3882: Simplify some query exec state locking
Date Sat, 05 Nov 2016 00:44:14 GMT
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 <henry@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message