Dinesh Bhat has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC ......................................................................
Patch Set 4: (14 comments) TFTR Adar/Alexey, updated the patch after addressing rev comments. http://gerrit.cloudera.org:8080/#/c/4099/1//COMMIT_MSG Commit Message: PS1, Line 9: This change also consolidates TSRegistrationPB and > nit: I think the link to the Web server's sample output might be specified This was more or less to assist the review, as I doubt anyone would be interested in following that link via commit-log in the git history. http://gerrit.cloudera.org:8080/#/c/4099/3//COMMIT_MSG Commit Message: Line 7: KUDU-1534 : Added software_version to ListMasters RPC > Maybe add a note below about the cleanup you did? Done http://gerrit.cloudera.org:8080/#/c/4099/1/src/kudu/common/wire_protocol.proto File src/kudu/common/wire_protocol.proto: Line 82: // This entails RPC/HTTP addresses and software version on > nit: and its version (optional). That is correct, it looks like you accessed an older diffs Alexey, comments were updated in newer diffs which I think addresses this rev comment already. http://gerrit.cloudera.org:8080/#/c/4099/3/src/kudu/common/wire_protocol.proto File src/kudu/common/wire_protocol.proto: PS3, Line 82: // This entails RPC/HTTP addresses and software version on : // the servers and used in the registation/heartbeat phase. : message ServerRegistrationPB { > The new comment describes how ServerRegistrationPB is used instead of what Tweaked to some extent, pls check again. http://gerrit.cloudera.org:8080/#/c/4099/3/src/kudu/integration-tests/cluster_itest_util.h File src/kudu/integration-tests/cluster_itest_util.h: Line 78: gscoped_ptr<tserver::TabletServerServiceProxy> tserver_proxy; > Hmm, how does the forward declaration help with this? The layout of ServerR Good catch, I overlooked that this was an instantiation. 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 i Hmm, I thought about that too, but here I tried to be consistent with tablet-servers page output. They follow exact same format like this. PS1, Line 333: R > Nit: an extra space. Is it really needed? Fixed. http://gerrit.cloudera.org:8080/#/c/4099/3/src/kudu/master/master-path-handlers.h File src/kudu/master/master-path-handlers.h: PS3, Line 68: tml(const ServerRegi > Does this type even exist? Does this method still need to be templated? Ha ! No, it was a blind text find/replace I did both in this line and above forward decl. Fixed both. http://gerrit.cloudera.org:8080/#/c/4099/3/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: Line 54: class ServerRegistrationPB; > Again, not seeing how this helps, given that to declare a ServerRegistratio Seriously, I guess I was simply adding this to all headers just to see if compiles smoothly, but later forgot to clean them up. Thanks for noticing them. PS3, Line 72: : // Create a client proxy to it. : MessengerBuilder bld("Client"); > This seems like an odd place to stash a test. Why not in its own TEST_F? Al I thought since ServerRegistrationPB was kinda stuffed in 'registration' as a mandatory field(though version is optional), we could let every Setup() go through this check. I see your point, I created a simplest test possible under registration-test to check version now. http://gerrit.cloudera.org:8080/#/c/4099/3/src/kudu/master/master.h File src/kudu/master/master.h: Line 33: #include "kudu/util/status.h" > Nit: sort alphabetically. Done. Line 41: > I don't think this helps given L135. Fixed. http://gerrit.cloudera.org:8080/#/c/4099/3/src/kudu/master/master.proto File src/kudu/master/master.proto: Line 234: > I wonder if this is backwards compatible. The message types are clearly dif Good point, will check them out, so don't +2 on this yet pending this test result. Line 555: // Node instance information is always set. > Nit: is this comment still relevant? Looks like we're providing a ServerReg I removed it, don't actually know what was it conveying there. -- 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: 4 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
