Alexey Serbin has posted comments on this change.

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


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4099/1//COMMIT_MSG
Commit Message:

Line 8: Sample output is at:
> Done, thanks for catching this.
nit: Web server's sample output


PS1, Line 9: 
https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf
nit: I think the link to the Web server's sample output might be specified in 
the comment for this file or somewhere else, but if you think it's better to 
have it in the commit message as is -- that's all right.


http://gerrit.cloudera.org:8080/#/c/4099/1/src/kudu/common/wire_protocol.proto
File src/kudu/common/wire_protocol.proto:

Line 82: // RPC and HTTP addresses for each server.
nit: and its version (optional).

BTW, as for the criteria when this field is present and when it's not: does 
every newer server fills this field or there are other criteria when this field 
is present?  It would be nice to document that in the comment as well.

Also, do we have any other use for this field besides pure informational one?  
It would be nice to indicate this.  I expect that this is only for 
informational use.  I.e., a master server is not expected to parse this field 
and make some decisions based on the result, right?  Otherwise I would expect 
to see more structured field here instead of a string.


http://gerrit.cloudera.org:8080/#/c/4099/1/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

PS1, Line 316: Registration
I think the output does not look nice for rendering in a single table row if 
adding the RPC, HTTP and version info along with UUID and Role.  Is it a 
requirement to have it in a single table row?  If not, consider leaving only 
version string and moving the registration details (such as RPC and HTTP 
end-points) into the detailed page which opens when you click on the UUID of 
the master.

I.e., consider leaving only UUID, Role, Version columns in the table and moving 
the RPC and HTTP end-points into the detailed server page which opens when 
clicking on the UUID link.


PS1, Line 333:  
Nit: an extra space.  Is it really needed?


-- 
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: 1
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

Reply via email to