impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <>
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:


The new test fails deterministically without the patch when run locally.
File be/src/service/

PS7, Line 296: NULL
> nit: change that one too to at least keep functions consistent
File be/src/service/

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

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

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

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

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie44daa93e3ae4d04d091261f3ec4891caffe8026
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-HasComments: Yes

View raw message