Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12615 )
Change subject: KUDU-2711 (part 3). Only send one TSInfoPB per server in GetTableLocations ...................................................................... Patch Set 1: (11 comments) http://gerrit.cloudera.org:8080/#/c/12615/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12615/1//COMMIT_MSG@12 PS1, Line 12: In the : worst case, this may slightly regress performance if the number of : replicas in the response is much fewer than the number of TS in the : cluster, since there is some extra overhead of building the index : mapping. Would it be possible to quantify this? http://gerrit.cloudera.org:8080/#/c/12615/1//COMMIT_MSG@60 PS1, Line 60: 100% (max) = 6336 Nit: indentation http://gerrit.cloudera.org:8080/#/c/12615/1/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: http://gerrit.cloudera.org:8080/#/c/12615/1/src/kudu/client/meta_cache.cc@811 PS1, Line 811: rpc.resp().ts_infos() Nit: could pull this out into a local a la tablet_locations, since it's used in two places. http://gerrit.cloudera.org:8080/#/c/12615/1/src/kudu/integration-tests/registration-test.cc File src/kudu/integration-tests/registration-test.cc: http://gerrit.cloudera.org:8080/#/c/12615/1/src/kudu/integration-tests/registration-test.cc@202 PS1, Line 202: nullptr); Nit: add /* ... */ to clarify the role of the nullptr. http://gerrit.cloudera.org:8080/#/c/12615/1/src/kudu/integration-tests/table_locations-itest.cc File src/kudu/integration-tests/table_locations-itest.cc: http://gerrit.cloudera.org:8080/#/c/12615/1/src/kudu/integration-tests/table_locations-itest.cc@251 PS1, Line 251: for (auto& loc : resp.tablet_locations()) { These loop variables can be const. Below too. http://gerrit.cloudera.org:8080/#/c/12615/1/src/kudu/integration-tests/table_locations-itest.cc@252 PS1, Line 252: ASSERT_GT(loc.replicas_size(), 0); Maybe also ASSERT that interned_replicas_size() is 0? http://gerrit.cloudera.org:8080/#/c/12615/1/src/kudu/integration-tests/table_locations-itest.cc@275 PS1, Line 275: LOG(INFO) << resp.DebugString(); Used only for debugging? http://gerrit.cloudera.org:8080/#/c/12615/1/src/kudu/integration-tests/table_locations-itest.cc@282 PS1, Line 282: ASSERT_GE(idx, 0); Also ASSERT_LT(idx, resp.ts_infos_size()) http://gerrit.cloudera.org:8080/#/c/12615/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/12615/1/src/kudu/master/catalog_manager.cc@4462 PS1, Line 4462: TSInfoPB* tsinfo_pb = new TSInfoPB(); Could you wrap this in a unique_ptr and have the lambda return it? http://gerrit.cloudera.org:8080/#/c/12615/1/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/12615/1/src/kudu/master/master.proto@373 PS1, Line 373: repeated ReplicaPB replicas = 4; Should this be prefixed with DEPRECATED_ as per the PB deprecation style? Or would that look weird from the perspective of GetTabletLocationsRequestPB? http://gerrit.cloudera.org:8080/#/c/12615/1/src/kudu/master/master.proto@535 PS1, Line 535: below above -- To view, visit http://gerrit.cloudera.org:8080/12615 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ief65d0825e919f681b7ade6a856c103201f8305c Gerrit-Change-Number: 12615 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Wed, 27 Feb 2019 00:59:51 +0000 Gerrit-HasComments: Yes
