Will Berkeley 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: (13 comments) http://gerrit.cloudera.org:8080/#/c/12615/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12615/1//COMMIT_MSG@60 PS1, Line 60: 100% (max) = 6336 > Nit: indentation Done http://gerrit.cloudera.org:8080/#/c/12615/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: http://gerrit.cloudera.org:8080/#/c/12615/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2123 PS1, Line 2123: // TODO(todd): handle "interned" response here > Will, since you said you were interested in taking this on, I think it'd be Affirmative. 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@217 PS1, Line 217: replicas_.emplace_back(std::move(rep)); > warning: std::move of the variable 'rep' of the trivially-copyable type 'ku Done 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 use Done 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. Done 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. Done 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? Done 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? Done 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()) Done 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? Done http://gerrit.cloudera.org:8080/#/c/12615/1/src/kudu/master/catalog_manager.cc@4474 PS1, Line 4474: // TODO: We should track these RPC addresses in the master table itself. > warning: missing username/bug in TODO [google-readability-todo] Done 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? O I'm not sure I understand. What do you mean it would "look weird"? http://gerrit.cloudera.org:8080/#/c/12615/1/src/kudu/master/master.proto@535 PS1, Line 535: below > above Done -- 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: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Mon, 11 Mar 2019 22:55:23 +0000 Gerrit-HasComments: Yes
