Adar Dembo has posted comments on this change.

Change subject: KUDU-1534 : Added software_version to ListMasters RPC

Patch Set 5:


Just a couple of nits but this is otherwise good, though we still need to find 
out whether this maintains backwards compatibility.
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).
File src/kudu/master/

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.
File src/kudu/master/ts_descriptor.h:

Line 34: class ServerRegistrationPB;
Nit: should come before Sockaddr alphabetically.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Dinesh Bhat <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to