Adar Dembo has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC ......................................................................
Patch Set 5: (3 comments) Just a couple of nits but this is otherwise good, though we still need to find out whether this maintains backwards compatibility. http://gerrit.cloudera.org:8080/#/c/4099/5/src/kudu/common/wire_protocol.proto File src/kudu/common/wire_protocol.proto: PS5, Line 82: This entails RPC/HTTP addresses and software version on : // the servers and used in the registation/heartbeat phase. Nit: how about "Basic properties common to both masters and tservers. Guaranteed not to change unless the server is restarted." - "entails" is an odd word choice. - There's no point to listing all of the fields; that list is self-explanatory, plus it'll become obsolete when a new field is added. - I don't think this is the right place to explain how/when the message is used. Better to find the usage sites and comment on them (though I think they're already commented). http://gerrit.cloudera.org:8080/#/c/4099/5/src/kudu/master/master-path-handlers.cc File src/kudu/master/master-path-handlers.cc: PS5, Line 511: std::string MasterPathHandlers::RegistrationToHtml( : const ServerRegistrationPB& reg, : const std::string& link_text) const { Nit: don't need to qualify string with std. http://gerrit.cloudera.org:8080/#/c/4099/5/src/kudu/master/ts_descriptor.h File src/kudu/master/ts_descriptor.h: Line 34: class ServerRegistrationPB; Nit: should come before Sockaddr alphabetically. -- To view, visit http://gerrit.cloudera.org:8080/4099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
