impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4456: Address scalability issues of qs map lock and client request state map lock
Date Mon, 23 Oct 2017 22:34:56 GMT
Philip Zeyliger has posted comments on this change. (

Change subject: IMPALA-4456: Address scalability issues of qs_map_lock_ and client_request_state_map_lock_

Patch Set 3:

Commit Message:
PS3, Line 30: shareded to a default of 4 buckets initally.
nit: sharded
File be/src/runtime/
PS3, Line 84:     uint8_t qs_map_bucket = QueryIdToBucket(query_id);
            :     lock_guard<mutex> l(qs_map_lock_[qs_map_bucket]);
Would it make sense to write a helper so that this code could look like:

// I don't like my naming here...
gs_map_bucket_guard qs_map_bucket_(get_guard(query_id));
qs_map_bucket_.find(query_id); ...

i.e., every time you use this thing, you have to use QueryIdToBucket(), and then use the result
of that for both the lock guard andthe map. Could that be shortened with a helper class that
contains both the lock guard and the map you're allowed to use?
File be/src/service/
PS3, Line 244:   for (int i = 0 ; i < 4 ; ++i) {
4 should be the constant here, no?
File be/src/service/impala-server.h:
PS3, Line 562:   /// on disk. Must be called with the corressponding client_request_state_map_lock_[]
nit: corresponding

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61089090e1095da45a8a64ed3ccc78bd310807f1
Gerrit-Change-Number: 8363
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Comment-Date: Mon, 23 Oct 2017 22:34:56 +0000
Gerrit-HasComments: Yes

  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message