Dinesh Bhat has posted comments on this change.

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

Patch Set 4:


TFTR Adar/Alexey, updated the patch after addressing rev comments.

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.

Commit Message:

Line 7: KUDU-1534 : Added software_version to ListMasters RPC
> Maybe add a note below about the cleanup you did?

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.

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.

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.

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?

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.

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.

File src/kudu/master/master.h:

Line 33: #include "kudu/util/status.h"
> Nit: sort alphabetically.

Line 41: 
> I don't think this helps given L135.

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 

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

Reply via email to