[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Adar Dembo has submitted this change and it was merged. Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. KUDU-1534 : Added software_version to ListMasters RPC This change also consolidates TSRegistrationPB and ServerRegistrationPB and merges them into latter as they seem to bear the same signature. As a precaution, testing is performed to make sure downgrade/upgrade are compatible with the message formats being consolidated. Testing details are attached under JIRA. Also added registration string to 'Masters' webui. Sample output is at: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c Reviewed-on: http://gerrit.cloudera.org:8080/4099 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M src/kudu/common/wire_protocol.proto M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/registration-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master-path-handlers.h M src/kudu/master/master-test.cc M src/kudu/master/master.cc M src/kudu/master/master.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h M src/kudu/tserver/heartbeater.cc 17 files changed, 63 insertions(+), 58 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/4099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c Gerrit-PatchSet: 8 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
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Adar Dembo has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. Patch Set 7: Code-Review+2 -- 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: 7 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: No
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. Patch Set 7: Build Started http://104.196.14.100/job/kudu-gerrit/3079/ -- 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: 7 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: No
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Dinesh Bhat has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. Patch Set 6: (2 comments) TFTR Adar, will follow the JIRA suggestions for more testing tonight. http://gerrit.cloudera.org:8080/#/c/4099/6/src/kudu/integration-tests/create-table-itest.cc File src/kudu/integration-tests/create-table-itest.cc: PS6, Line 124: const int kNumServers = 3; : const int kNumTablets = 3; > Why did you change these values? Oops, reverted. that shouldn't have gone in this branch. thanks. http://gerrit.cloudera.org:8080/#/c/4099/6/src/kudu/master/master-path-handlers.cc File src/kudu/master/master-path-handlers.cc: PS6, Line 513: std::string > Missed one. Hmmm... if you see this source(as well as header file), all the routines use std:;string. I would rather be consistent here although I know it's a moot namespace prefix here. I only removed return value to be string instead of std::string(again something followed by all other routines). We can prolly fix this in a different change may be? -- 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: 6 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
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Hello Mike Percy, Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4099 to look at the new patch set (#7). Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. KUDU-1534 : Added software_version to ListMasters RPC This change also consolidates TSRegistrationPB and ServerRegistrationPB and merges them into latter as they seem to bear the same signature. As a precaution, testing is performed to make sure downgrade/upgrade are compatible with the message formats being consolidated. Testing details are attached under JIRA. Also added registration string to 'Masters' webui. Sample output is at: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c --- M src/kudu/common/wire_protocol.proto M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/registration-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master-path-handlers.h M src/kudu/master/master-test.cc M src/kudu/master/master.cc M src/kudu/master/master.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h M src/kudu/tserver/heartbeater.cc 17 files changed, 63 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/4099/7 -- To view, visit http://gerrit.cloudera.org:8080/4099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c Gerrit-PatchSet: 7 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
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Adar Dembo has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. Patch Set 6: (2 comments) I left you some more comments in the JIRA regarding the testing. http://gerrit.cloudera.org:8080/#/c/4099/6/src/kudu/integration-tests/create-table-itest.cc File src/kudu/integration-tests/create-table-itest.cc: PS6, Line 124: const int kNumServers = 3; : const int kNumTablets = 3; Why did you change these values? http://gerrit.cloudera.org:8080/#/c/4099/6/src/kudu/master/master-path-handlers.cc File src/kudu/master/master-path-handlers.cc: PS6, Line 513: std::string Missed one. -- 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: 6 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
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Dinesh Bhat has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. Patch Set 4: Addressed all the rev comments below, thanks Adar. Also updated commit message to reflect upgrade/downgrade testing. -- 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 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: No
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. Patch Set 6: Build Started http://104.196.14.100/job/kudu-gerrit/3078/ -- 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: 6 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: No
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Hello Mike Percy, Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4099 to look at the new patch set (#6). Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. KUDU-1534 : Added software_version to ListMasters RPC This change also consolidates TSRegistrationPB and ServerRegistrationPB and merges them into latter as they seem to bear the same signature. As a precaution, testing is performed to make sure downgrade/upgrade are compatible with the message formats being consolidated. Testing details are attached under JIRA. Also added registration string to 'Masters' webui. Sample output is at: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c --- M src/kudu/common/wire_protocol.proto M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/create-table-itest.cc M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/registration-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master-path-handlers.h M src/kudu/master/master-test.cc M src/kudu/master/master.cc M src/kudu/master/master.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h M src/kudu/tserver/heartbeater.cc 18 files changed, 65 insertions(+), 60 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/4099/6 -- To view, visit http://gerrit.cloudera.org:8080/4099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c Gerrit-PatchSet: 6 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
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Adar Dembo has posted comments on this change.
Change subject: KUDU-1534 : Added software_version to ListMasters RPC
..
Patch Set 5:
(3 comments)
Just a couple of nits but this is otherwise good, though we still need to find
out whether this maintains backwards compatibility.
http://gerrit.cloudera.org:8080/#/c/4099/5/src/kudu/common/wire_protocol.proto
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).
http://gerrit.cloudera.org:8080/#/c/4099/5/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:
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.
http://gerrit.cloudera.org:8080/#/c/4099/5/src/kudu/master/ts_descriptor.h
File src/kudu/master/ts_descriptor.h:
Line 34: class ServerRegistrationPB;
Nit: should come before Sockaddr alphabetically.
--
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: 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
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
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_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
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
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Hello Mike Percy, Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4099 to look at the new patch set (#5). Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. KUDU-1534 : Added software_version to ListMasters RPC This change also consolidates TSRegistrationPB and ServerRegistrationPB and merges them into latter as they seem to bear the same signature. Sample output is at: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c --- M src/kudu/common/wire_protocol.proto M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/registration-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master-path-handlers.h M src/kudu/master/master-test.cc M src/kudu/master/master.cc M src/kudu/master/master.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h M src/kudu/tserver/heartbeater.cc 17 files changed, 63 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/4099/5 -- To view, visit http://gerrit.cloudera.org:8080/4099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset 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
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. Patch Set 5: Build Started http://104.196.14.100/job/kudu-gerrit/3071/ -- 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: 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: No
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. Patch Set 4: Build Started http://104.196.14.100/job/kudu-gerrit/3070/ -- 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 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: No
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Hello Mike Percy, Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4099 to look at the new patch set (#4). Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. KUDU-1534 : Added software_version to ListMasters RPC This change also consolidates TSRegistrationPB and ServerRegistrationPB and merges them into latter as they seem to bear the same signature. Sample output is at: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c --- M src/kudu/common/wire_protocol.proto M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/registration-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master-path-handlers.h M src/kudu/master/master-test.cc M src/kudu/master/master.cc M src/kudu/master/master.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h M src/kudu/tserver/heartbeater.cc 17 files changed, 65 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/4099/4 -- To view, visit http://gerrit.cloudera.org:8080/4099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c Gerrit-PatchSet: 4 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
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Adar Dembo has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. Patch Set 3: (13 comments) 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? http://gerrit.cloudera.org:8080/#/c/4099/3/src/kudu/common/wire_protocol.proto 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 instance : // 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. 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 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. http://gerrit.cloudera.org:8080/#/c/4099/3/src/kudu/master/master-path-handlers.h 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? 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 ServerRegistrationPB on the stack you need the real class declaration. PS3, Line 72: ServerRegistrationPB reg; : master_->GetMasterRegistration(®); : 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? http://gerrit.cloudera.org:8080/#/c/4099/3/src/kudu/master/master.h 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. http://gerrit.cloudera.org:8080/#/c/4099/3/src/kudu/master/master.proto 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 http://gerrit.cloudera.org:8080/4099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings 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
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
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 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
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Dinesh Bhat has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/4099/1//COMMIT_MSG Commit Message: Line 8: > nit: please add a blank line between the top line and the "body" of the mes Done, thanks for catching this. -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Hello Mike Percy, Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4099 to look at the new patch set (#3). Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. KUDU-1534 : Added software_version to ListMasters RPC Sample output is at: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c --- M src/kudu/common/wire_protocol.proto M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/registration-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master-path-handlers.h M src/kudu/master/master-test.cc M src/kudu/master/master.cc M src/kudu/master/master.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h M src/kudu/tserver/heartbeater.cc 17 files changed, 56 insertions(+), 51 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/4099/3 -- To view, visit http://gerrit.cloudera.org:8080/4099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. Patch Set 3: Build Started http://104.196.14.100/job/kudu-gerrit/3042/ -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Dinesh Bhat has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. Patch Set 2: (1 comment) Yeah, I think it is possible to keep just one although they seem to be coming from two different proto files. I really haven't looked at how .pb.* files are generated from proto, it would help if you could give me some pointers to source/makefile. I am hoping that we won't run into into any dependency issues here. http://gerrit.cloudera.org:8080/#/c/4099/2//COMMIT_MSG Commit Message: PS2, Line 10: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf > Not that this really matters, but why a PDF and not a PNG? Isn't that what I can't recall why actually, I guess I exported it to pdf for no specific reason. -- 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: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Adar Dembo has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. Patch Set 2: (1 comment) I was really confused because I thought this patch had already been written. Then I went and looked at https://gerrit.cloudera.org/#/c/4060, and understood. But, why do we have both ServerRegistrationPB and TSRegistrationPB when they're exactly the same? Can you look into whether it'd be realistic to remove the latter in favor of the former? http://gerrit.cloudera.org:8080/#/c/4099/2//COMMIT_MSG Commit Message: PS2, Line 10: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf Not that this really matters, but why a PDF and not a PNG? Isn't that what screenshot tools generate? -- 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: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. Patch Set 2: Build Started http://104.196.14.100/job/kudu-gerrit/3026/ -- 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: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
Hello Mike Percy, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4099 to look at the new patch set (#2). Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. KUDU-1534 : Added software_version to ListMasters RPC Sample output is at: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c --- M src/kudu/common/wire_protocol.proto M src/kudu/master/master-path-handlers.cc M src/kudu/master/master-test.cc M src/kudu/master/master.cc 4 files changed, 17 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/4099/2 -- To view, visit http://gerrit.cloudera.org:8080/4099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC Sample output is at: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf
Todd Lipcon has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC Sample output is at: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf .. Patch Set 1: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/4099/1//COMMIT_MSG Commit Message: Line 8: Sample output is at: nit: please add a blank line between the top line and the "body" of the message -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC Sample output is at: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC Sample output is at: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf .. Patch Set 1: Build Started http://104.196.14.100/job/kudu-gerrit/3025/ -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC Sample output is at: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf
Hello Mike Percy, Adar Dembo, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/4099 to review the following change. Change subject: KUDU-1534 : Added software_version to ListMasters RPC Sample output is at: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf .. KUDU-1534 : Added software_version to ListMasters RPC Sample output is at: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c --- M src/kudu/common/wire_protocol.proto M src/kudu/master/master-path-handlers.cc M src/kudu/master/master-test.cc M src/kudu/master/master.cc 4 files changed, 17 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/4099/1 -- To view, visit http://gerrit.cloudera.org:8080/4099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
