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 Thu, 18 May 2017 22:09:02 GMT
Bharath Vissapragada has posted comments on this change.

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

Patch Set 6:

Commit Message:

PS6, Line 16: While the actual fix for this is to not hold the QES::lock_ during
            : metadata load,
> I disagree. what you are doing is the actual fix for the lock acquisition o

PS6, Line 20: We make sure that QES::lock_ doesn't block query_exec_state_map_lock_
            :   in hot paths.
> After this change, isn't it the case that we'll never try to acquire anothe
This was stale. Updated.
File be/src/service/

Line 192:   DEFINE_int64(metadata_loading_pause_injection_ms, 0, "Simulates metadata loading
for a "
> how about prefixing the flag with "stress_", like we do for the one in free

PS6, Line 822:     // Note: this acquires the exec_state lock *before* the
             :     // query_exec_state_map_ lock. This is the opposite of
             :     // GetQueryExecState(..., true), and therefore looks like a
             :     // candidate for deadlock. The reason this works here is that
             :     // GetQueryExecState cannot find exec_state (under the exec state
             :     // map lock) and take it's lock until RegisterQuery has
             :     // finished. By that point, the exec state map lock will have been
             :     // given up, so the classic deadlock interleaving is not possible.
> this comment is outdated now.
File be/src/service/impala-server.h:

PS6, Line 85: /// 4. query_exec_state_map_lock_
> i think this should be updated. with this change, do we ever hold this lock
No. Checked the usage of in the callers. Updated the comment.
File tests/custom_cluster/

PS6, Line 55: GetRuntimeProfileStr
> This doesn't make sense. GetRuntimeProfileStr() doesn't hold the map lock w
Ah this comment is wrong. My intention was exactly to call QuerySummaryHandler() which is
place that calls with the interleaving locking that causes the hang. Corrected it.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie44daa93e3ae4d04d091261f3ec4891caffe8026
Gerrit-PatchSet: 6
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