Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12138 )
Change subject: Support location awareness in READ_CLOSEST for the C++ client ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/12138/3/src/kudu/integration-tests/location_assignment-itest.cc File src/kudu/integration-tests/location_assignment-itest.cc: http://gerrit.cloudera.org:8080/#/c/12138/3/src/kudu/integration-tests/location_assignment-itest.cc@78 PS3, Line 78: auto location = Substitute("/L$0", i); : if (i == client_loc_idx) { : EmplaceOrDie(&info, std::move(location), 2); : } else { : EmplaceOrDie(&info, std::move(location), 1); : } Nit: combine: EmplaceOrDie(&info, Substitute("/L$0", i), i == client_loc_idx ? 2 : 1); http://gerrit.cloudera.org:8080/#/c/12138/3/src/kudu/integration-tests/location_assignment-itest.cc@90 PS3, Line 90: NO_FATALS(cluster_->AssertNoCrashes()); Is this really necessary here? Won't BuildAndStart() fail in some way? http://gerrit.cloudera.org:8080/#/c/12138/3/src/kudu/integration-tests/location_assignment-itest.cc@109 PS3, Line 109: client::sp::shared_ptr<KuduClient> client; : NO_FATALS(CreateClient(&client)); : client::sp::shared_ptr<KuduTable> table; : ASSERT_OK(client->OpenTable(kTableId, &table)); Why do we need a new KuduClient and KuduTable? Can't we reuse what's in TabletServerIntegrationTestBase? -- To view, visit http://gerrit.cloudera.org:8080/12138 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713 Gerrit-Change-Number: 12138 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Fri, 04 Jan 2019 22:15:18 +0000 Gerrit-HasComments: Yes
