Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11313 )
Change subject: [location_awareness] Add 'location' column in tserver list ...................................................................... Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/common/wire_protocol.proto File src/kudu/common/wire_protocol.proto: http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/common/wire_protocol.proto@84 PS1, Line 84: The tablet server's I think NodeInstancePB is about any kudu server, i.e. masters are also meant to be represented by this structure. How locations are about to be used in case of masters is another question, though. BTW, I remember the original idea was to put location into ServerRegistrationPB. Did something changed in that regard? I.e., why do you think NodeInstancePB is a better place for that? http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/master/ts_descriptor.cc File src/kudu/master/ts_descriptor.cc: http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/master/ts_descriptor.cc@310 PS1, Line 310: instance_pb->set_location(location_ == boost::none ? : "" : string(location_->begin(), location_->end())); Why to set it to an empty string if it's possible to not set it all in NodeInstancPB? Also, boost::optional has operator* () that allows to de-refence the embedded value. http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2038 PS1, Line 2038: // Location (not assigned) nit: drop the parentheses. http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2039 PS1, Line 2039: NO_FATALS(StartExternalMiniCluster()); Why is it necessary to start it again when it's been already started at line 1984? http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2047 PS1, Line 2047: " location\n--------------\n not assigned" Consider using ASSERT_STR_CONTAINS macro. Or the spacing is really important here? http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2050 PS1, Line 2050: // Location (assigned) nit: drop the parentheses. http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2054 PS1, Line 2054: FLAGS_location_mapping_cmd = Substitute("$0 $1", kLocationCmdPath, location); Why is it important to set FLAGS_location_mapping_cmd if it's an external cluster anyway and it's being passed the --location_mapping_cmd flag? http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2058 PS1, Line 2058: NO_FATALS(StartExternalMiniCluster(opts)); ditto: the external minicluster has been started already at line 1984, no? http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2066 PS1, Line 2066: Is it important to have this spacing to be some specified length? If not, consider using ASSERT_STR_CONTAINS macro instead. -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 1 Gerrit-Owner: Fengling Wang <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Fri, 24 Aug 2018 17:25:41 +0000 Gerrit-HasComments: Yes
