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 <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Mon, 23 Oct 2017 22:34:56 +0000 Gerrit-HasComments: Yes
