Adar Dembo has posted comments on this change.

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

Patch Set 3:

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:

PS3, Line 82: // Sent by the TS when it first heartbeats with a master. This 
sends the
            : // master all of the necessary information about the current 
            : // of the TS. This is also used by master during master bringup.
The new comment describes how ServerRegistrationPB is used instead of what it 
means. The latter is more relevant here.
File src/kudu/integration-tests/cluster_itest_util.h:

Line 53: class ServerRegistrationPB;
Nit: sort alphabetically.

Line 78:   ServerRegistrationPB registration;
Hmm, how does the forward declaration help with this? The layout of 
ServerRegistrationPB must be known in order to satisfy this definition.
File src/kudu/master/master-path-handlers.h:

Line 28: class ServerRegistrationPB;
Why is this forward declaration needed?

PS3, Line 68: MasterRegistrationPB
Does this type even exist? Does this method still need to be templated?
File src/kudu/master/

Line 54: class ServerRegistrationPB;
Again, not seeing how this helps, given that to declare a ServerRegistrationPB 
on the stack you need the real class declaration.

PS3, Line 72:     ServerRegistrationPB reg;
            :     master_->GetMasterRegistration(&reg);
            :     ASSERT_TRUE(reg.has_software_version());
This seems like an odd place to stash a test. Why not in its own TEST_F? Also, 
is this a better location than registration-test?
File src/kudu/master/master.h:

Line 33: #include "kudu/common/wire_protocol.h"
Nit: sort alphabetically.

Line 41: class ServerRegistrationPB;
I don't think this helps given L135.
File src/kudu/master/master.proto:

Line 26: 
Nit: why the added empty line?

Line 234:   optional ServerRegistrationPB registration = 2;
I wonder if this is backwards compatible. The message types are clearly 
different, but the layout of both is compatible. Can you find out?

Line 555: // TODO: Just use ServerRegistration here.
Nit: is this comment still relevant? Looks like we're providing a 
ServerRegistrationPB now.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c
Gerrit-PatchSet: 3
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