impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1972/IMPALA-3882: Fix client request state map lock contention
Date Fri, 19 May 2017 23:49:51 GMT
Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-1972/IMPALA-3882: Fix client_request_state_map_lock_ contention
......................................................................


Patch Set 7:

(8 comments)

The new test fails deterministically without the patch when run locally.

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

PS7, Line 296: NULL
> nit: change that one too to at least keep functions consistent
Done


http://gerrit.cloudera.org:8080/#/c/6707/7/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:

PS7, Line 721: just return
> that's not what the code does (it also sets plan_metadata_unavailable), ple
Done


PS7, Line 730: adopt_lock_t
> shouldn't that be deleted?
Oops, missed that during rebase.


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

Line 836: #endif
> actually, how about moving this to before UpdateQueryStatus() since that mo
Done


http://gerrit.cloudera.org:8080/#/c/6707/7/tests/custom_cluster/test_query_concurrency.py
File tests/custom_cluster/test_query_concurrency.py:

PS7, Line 32: The intention here is to check contention on the query_exec_state_map_lock_
> This is talking about how the old code worked, which won't make sense to pe
Rephrased.


PS7, Line 54: This creates lock contention on the coordinator by
            :     calling QuerySummaryHandler() method
> This is no longer true with your fix. How about saying:
Yea it doesn't make sense to someone reading it with the fix. . IMO, the test class comment
conveys the intention already and this specific comment looks redundant. Removed it, please
let me know if you disagree.


PS7, Line 74: time.sleep(2)
> I'm worried that this will be flaky, especially with ASAN.  Instead of this
Yep good idea to reduce flakiness. But I don't think passing a large value to get_inflight_queries()
would achieve that since the /inflight_queries end point never times out. Instead we should
repeatedly poll that page in loop with timeout. Redid the logic accordingly.


PS7, Line 83: time.sleep(2)
> this delay is a bit harder to eliminate.  How about we increase --stress_me
Same comment as above. I redid the logic, please let me know if that makes sense.


-- 
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message