Will Berkeley 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: (7 comments) http://gerrit.cloudera.org:8080/#/c/11313/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11313/1//COMMIT_MSG@16 PS1, Line 16: not assigned I'd prefer a value like <none>. There's a couple of advantages to it: 1. It contains a character that is invalid in a location string, so it's unambiguously not a location. 2. It doesn't have spaces in it, which is sometimes helpful when someone uses an alternate format and processes the output with scripts. 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 mean ServerRegistrationPB is also used for both master and tservers. I think the right place for location information is in ListTabletServersResponsePB::Entry, since that is a PB used to describe a tablet server specifically. message Entry { required NodeInstancePB instance_id = 1; optional ServerRegistrationPB registration = 2; optional int32 millis_since_heartbeat = 3; optional string location = 4; } 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 Node I agree we should leave the field unset if there's no location. For one thing, the PB object will return an empty string for the location when the location isn't set. Also, just a nit, but I prefer boost::optional::get() to retrieve the value out of an optional, to distinguish it from a pointer because it has value semantics. 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) Also, comments end in a period. http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2050 PS1, Line 2050: // Location (assigned) Ditto period. 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? Yeah, we need to either make this a separate test where the flag is set before the cluster is started, or the cluster needs to be restarted with new flags. Calling StartExternalMiniCluster again doesn't work. http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/testdata/first_argument.sh File src/kudu/tools/testdata/first_argument.sh: PS1: You'll need to add this as a data file in the CMakeLists. See my patch to add location assignment for an example. -- 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 <fw...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Fri, 24 Aug 2018 18:20:03 +0000 Gerrit-HasComments: Yes
