impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <ger...@cloudera.org>
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. ( http://gerrit.cloudera.org:8080/8363
)

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


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8363/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8363/3//COMMIT_MSG@30
PS3, Line 30: shareded to a default of 4 buckets initally.
nit: sharded


http://gerrit.cloudera.org:8080/#/c/8363/3/be/src/runtime/query-exec-mgr.cc
File be/src/runtime/query-exec-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/8363/3/be/src/runtime/query-exec-mgr.cc@84
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?


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

http://gerrit.cloudera.org:8080/#/c/8363/3/be/src/service/impala-http-handler.cc@244
PS3, Line 244:   for (int i = 0 ; i < 4 ; ++i) {
4 should be the constant here, no?


http://gerrit.cloudera.org:8080/#/c/8363/3/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8363/3/be/src/service/impala-server.h@562
PS3, Line 562:   /// on disk. Must be called with the corressponding client_request_state_map_lock_[]
nit: corresponding



-- 
To view, visit http://gerrit.cloudera.org:8080/8363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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 <sailesh@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Oct 2017 22:34:56 +0000
Gerrit-HasComments: Yes

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