Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11727 )
Change subject: [location_awareness] Add ts location to TSInfoPB ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/11727/2/src/kudu/integration-tests/table_locations-itest.cc File src/kudu/integration-tests/table_locations-itest.cc: http://gerrit.cloudera.org:8080/#/c/11727/2/src/kudu/integration-tests/table_locations-itest.cc@127 PS2, Line 127: properply Nit: properly http://gerrit.cloudera.org:8080/#/c/11727/2/src/kudu/integration-tests/table_locations-itest.cc@128 PS2, Line 128: class TableLocationsWithTSLocationTest : public KuduTest { You should be able to extend the existing test fixture; just add a virtual method that provides the set of options to configure in the cluster. The base implementation will do nothing, and this fixture will return the option needed to set up the location mapping command. http://gerrit.cloudera.org:8080/#/c/11727/2/src/kudu/integration-tests/table_locations-itest.cc@138 PS2, Line 138: "scripts/first_argument.sh"); Nit: indentation http://gerrit.cloudera.org:8080/#/c/11727/2/src/kudu/integration-tests/table_locations-itest.cc@140 PS2, Line 140: FLAGS_location_mapping_cmd = strings::Substitute("$0 $1", location_cmd_path, location); Could you actually configure this using 'opts'? The problem is that if this switched to an ExternalMiniCluster, FLAGS_... configuration won't work anymore. http://gerrit.cloudera.org:8080/#/c/11727/2/src/kudu/integration-tests/table_locations-itest.cc@315 PS2, Line 315: : { // Check that the master doesn't give back partial results while the table is being created. : GetTableLocationsRequestPB req; : GetTableLocationsResponsePB resp; : RpcController controller; : req.mutable_table()->set_table_name(table_name); : : for (int i = 1; ; i++) { : if (i > 10) { : FAIL() << "Create table timed out"; : } : : controller.Reset(); : ASSERT_OK(proxy_->GetTableLocations(req, &resp, &controller)); : SCOPED_TRACE(SecureDebugString(resp)); : : if (resp.has_error()) { : ASSERT_EQ(MasterErrorPB::TABLET_NOT_RUNNING, resp.error().code()); : SleepFor(MonoDelta::FromMilliseconds(i * i * 100)); : } else { : ASSERT_EQ(1, resp.tablet_locations().size()); : break; : } : } : } Isn't this tested by TestGetTableLocations above already? Is this code duplicated or is it subtly different? http://gerrit.cloudera.org:8080/#/c/11727/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11727/2/src/kudu/master/catalog_manager.cc@4436 PS2, Line 4436: if (ts_desc->location()) tsinfo_pb->set_location(*(ts_desc->location())); Do we need the if statement? What does location() return otherwise? If it's an empty string, we could just as well call it unconditionally. -- To view, visit http://gerrit.cloudera.org:8080/11727 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic5865cb47698817097a7f62c968c5cfd80e07259 Gerrit-Change-Number: 11727 Gerrit-PatchSet: 2 Gerrit-Owner: Fengling Wang <fw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Mon, 22 Oct 2018 20:53:21 +0000 Gerrit-HasComments: Yes
