You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2018/02/21 13:17:05 UTC
[kudu-CR] KUDU-2039: /masters can show the wrong list of masters
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9378
Change subject: KUDU-2039: /masters can show the wrong list of masters
......................................................................
KUDU-2039: /masters can show the wrong list of masters
The ListMasters RPC returned masters based on the -master_addresses
flag. It should return masters based on the consensus configuration.
Since we verify the two sets of addresses are the same at startup, this
usually makes no difference. However, if a quorum master is
replaced by a server with a new UUID, previously we would report it
in ListMasters and therefore on /masters. This patch adds an
additional check that the UUIDs match, and returns an error
along with the registration information if there is a mismatch.
Screenshot for down master: https://github.com/wdberkeley/kudu/blob/kudu2309screenshots/www/mastersonedown.png
Screenshot for UUID mismatch: https://github.com/wdberkeley/kudu/blob/kudu2309screenshots/www/mastersmismatch.png
Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
---
M src/kudu/master/master.cc
M src/kudu/master/master_path_handlers.cc
M src/kudu/master/sys_catalog.h
M www/masters.mustache
4 files changed, 44 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/9378/1
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2039: /masters can show the wrong list of masters
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9378 )
Change subject: KUDU-2039: /masters can show the wrong list of masters
......................................................................
Patch Set 2: Code-Review+1
(3 comments)
Looks good! Just nits.
http://gerrit.cloudera.org:8080/#/c/9378/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9378/2//COMMIT_MSG@7
PS2, Line 7: 2039
2309
http://gerrit.cloudera.org:8080/#/c/9378/2//COMMIT_MSG@12
PS2, Line 12: quorum master
nit: just "master"?
http://gerrit.cloudera.org:8080/#/c/9378/2//COMMIT_MSG@13
PS2, Line 13: it
nit: "the new UUID, even though it may not be a part of the previously established quorum"
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 21 Feb 2018 22:12:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2309: /masters can show the wrong list of masters
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9378 )
Change subject: KUDU-2309: /masters can show the wrong list of masters
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache
File www/masters.mustache:
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache@40
PS4, Line 40: {{^has_no_errors}}
> The '?' operator isn't working when I test it, unfortunately. Also, it appe
Interesting. That's fine by me then!
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 06 Mar 2018 18:51:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2309: /masters can show the wrong list of masters
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9378
to look at the new patch set (#4).
Change subject: KUDU-2309: /masters can show the wrong list of masters
......................................................................
KUDU-2309: /masters can show the wrong list of masters
The ListMasters RPC (on which the /masters page is based) returned
masters based on the -master_addresses flag. It should return masters
based on the consensus configuration. Since we verify the two sets of
addresses are the same at startup, this usually makes no difference.
However, if a master is replaced by a server with a new UUID,
previously we would report the new UUID, even though it may not be
part of the previously established quorum. This patch adds an
additional check that the UUIDs match, and returns an error along with
the registration information if there is a mismatch.
Screenshot for down master: https://github.com/wdberkeley/kudu/blob/kudu2309screenshots/www/mastersonedown.png
Screenshot for UUID mismatch: https://github.com/wdberkeley/kudu/blob/kudu2309screenshots/www/mastersmismatch.png
Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
---
M src/kudu/master/master.cc
M src/kudu/master/master_path_handlers.cc
M src/kudu/master/sys_catalog.h
M www/masters.mustache
4 files changed, 53 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/9378/4
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2309: /masters can show the wrong list of masters
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9378 )
Change subject: KUDU-2309: /masters can show the wrong list of masters
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9378/3/src/kudu/master/master.cc
File src/kudu/master/master.cc:
http://gerrit.cloudera.org:8080/#/c/9378/3/src/kudu/master/master.cc@331
PS3, Line 331: StatusToPB(s, peer_entry.mutable_error());
Just another thought: maybe, it's worth at least add the UUID of the peer from the Raft consensus info in that case? At least, that information is always available, so it's not hurt to have it even if it's not possible to verify that the UUID in the consensus and server registration match.
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 22 Feb 2018 23:26:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2309: /masters can show the wrong list of masters
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9378 )
Change subject: KUDU-2309: /masters can show the wrong list of masters
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9378/3/www/masters.mustache
File www/masters.mustache:
http://gerrit.cloudera.org:8080/#/c/9378/3/www/masters.mustache@40
PS3, Line 40: <h2>Errors</h2>
Does it make sense to show this if there are no errors?
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 23 Feb 2018 20:34:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2309: /masters can show the wrong list of masters
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9378 )
Change subject: KUDU-2309: /masters can show the wrong list of masters
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache
File www/masters.mustache:
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache@40
PS4, Line 40: {{^has_no_errors}}
> In that case, the {{#errors}} section would need to surround the whole <tab
I have 2 suggestions below:
This is somehow not documented well, but I found this usage in Impala code. You can use the '?' symbol to check if a list is non-empty. So you could do something like this:
{{?errors}}
<h2>Errors</h2>
<table class='table table-striped'>
<thead><tr>
<th>UUID</th>
<th>Error</th>
</tr></thead>
<tbody>
{{#errors}}
<tr>
<td>{{uuid}}</td>
<td><pre><code>{{error}}</code></pre></td>
</tr>
{{/errors}}
</tbody>
</table>
{{/errors}}
I'm not sure if Impala and Kudu use the same version of mustache, but it's worth a try.
A final suggestion is to try this, but it may or may not be what you want. We could display the static table headers unconditionally, and only fill in the body through the mustache list:
<h2>Errors</h2>
<table class='table table-striped'>
<thead><tr>
<th>UUID</th>
<th>Error</th>
</tr></thead>
<tbody>
{{#errors}}
<tr>
<td>{{uuid}}</td>
<td><pre><code>{{error}}</code></pre></td>
</tr>
{{/errors}}
{{^errors}}
<tr>
<td>N/A</td>
<td>N/A</td>
</tr>
{{/errors}}
</tbody>
</table>
If we don't want to display even the table header when there are no errors, then this doesn't apply.
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 06 Mar 2018 18:05:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2309: /masters can show the wrong list of masters
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9378 )
Change subject: KUDU-2309: /masters can show the wrong list of masters
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache
File www/masters.mustache:
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache@40
PS4, Line 40: {{^has_no_errors}}
mustache doesn't have a way to condition based on the length of the 'errors' array? That's pretty crappy :(
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 05 Mar 2018 19:18:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2039: /masters can show the wrong list of masters
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9378 )
Change subject: KUDU-2039: /masters can show the wrong list of masters
......................................................................
Patch Set 2: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9378/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:
http://gerrit.cloudera.org:8080/#/c/9378/2/src/kudu/master/master.cc@308
PS2, Line 308: push_back
nit: maybe, update this to be emplace_back(std::move(local_entry)) as well?
http://gerrit.cloudera.org:8080/#/c/9378/2/src/kudu/master/master.cc@317
PS2, Line 317: auto config
nit: for better readability, could this be 'const auto config' ?
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 22 Feb 2018 03:53:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2309: /masters can show the wrong list of masters
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9378 )
Change subject: KUDU-2309: /masters can show the wrong list of masters
......................................................................
KUDU-2309: /masters can show the wrong list of masters
The ListMasters RPC (on which the /masters page is based) returned
masters based on the -master_addresses flag. It should return masters
based on the consensus configuration. Since we verify the two sets of
addresses are the same at startup, this usually makes no difference.
However, if a master is replaced by a server with a new UUID,
previously we would report the new UUID, even though it may not be
part of the previously established quorum. This patch adds an
additional check that the UUIDs match, and returns an error along with
the registration information if there is a mismatch.
Screenshot for down master: https://github.com/wdberkeley/kudu/blob/kudu2309screenshots/www/mastersonedown.png
Screenshot for UUID mismatch: https://github.com/wdberkeley/kudu/blob/kudu2309screenshots/www/mastersmismatch.png
Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Reviewed-on: http://gerrit.cloudera.org:8080/9378
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/master/master.cc
M src/kudu/master/master_path_handlers.cc
M src/kudu/master/sys_catalog.h
M www/masters.mustache
4 files changed, 53 insertions(+), 27 deletions(-)
Approvals:
Kudu Jenkins: Verified
Todd Lipcon: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2039: /masters can show the wrong list of masters
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9378 )
Change subject: KUDU-2039: /masters can show the wrong list of masters
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9378/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9378/2//COMMIT_MSG@7
PS2, Line 7: 2039
> 2309
Done
http://gerrit.cloudera.org:8080/#/c/9378/2//COMMIT_MSG@12
PS2, Line 12: quorum master
> nit: just "master"?
Done
http://gerrit.cloudera.org:8080/#/c/9378/2//COMMIT_MSG@13
PS2, Line 13: it
> nit: "the new UUID, even though it may not be a part of the previously esta
Done
http://gerrit.cloudera.org:8080/#/c/9378/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:
http://gerrit.cloudera.org:8080/#/c/9378/2/src/kudu/master/master.cc@308
PS2, Line 308: push_back
> nit: maybe, update this to be emplace_back(std::move(local_entry)) as well?
Done
http://gerrit.cloudera.org:8080/#/c/9378/2/src/kudu/master/master.cc@317
PS2, Line 317: auto config
> nit: for better readability, could this be 'const auto config' ?
Done
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 22 Feb 2018 19:10:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2309: /masters can show the wrong list of masters
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9378 )
Change subject: KUDU-2309: /masters can show the wrong list of masters
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache
File www/masters.mustache:
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache@40
PS4, Line 40: {{^has_no_errors}}
> mustache doesn't have a way to condition based on the length of the 'errors
"An inverted section begins with a caret (hat) and ends with a slash. That is {{^person}} begins a "person" inverted section while {{/person}} ends it.
While sections can be used to render text one or more times based on the value of the key, inverted sections may render text once based on the inverse value of the key. That is, they will be rendered if the key doesn't exist, is false, or is an empty list"
https://mustache.github.io/mustache.5.html
I don't think the 'has_no_errors' field is necessary. The manual says if an array is empty, it will execute the condition under {{^emptyarray}}.
So I imagine you could just have a regular section after:
{{#errors}}
...
...
{{/errors}}
{{^errors}}
No errors
{{/errors}}
Or something similar.
Alternatively, you could use Javascript to read the JSON and suppress HTML fields accordingly.
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 05 Mar 2018 21:23:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2309: /masters can show the wrong list of masters
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9378 )
Change subject: KUDU-2309: /masters can show the wrong list of masters
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache
File www/masters.mustache:
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache@40
PS4, Line 40: {{^has_no_errors}}
> I have 2 suggestions below:
The '?' operator isn't working when I test it, unfortunately. Also, it appears the Impala mustache library is older at 80cbe5 than ours at 87a592e.
This SO question has some alternative methods:
https://stackoverflow.com/questions/11653764
but I like the current workaround. It's a little goofy because of the extra variable and double-negation, but it does exactly what is desired without using implementation-specific or unreliable features.
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 06 Mar 2018 18:45:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2309: /masters can show the wrong list of masters
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9378 )
Change subject: KUDU-2309: /masters can show the wrong list of masters
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 06 Mar 2018 22:32:14 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2039: /masters can show the wrong list of masters
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9378
to look at the new patch set (#2).
Change subject: KUDU-2039: /masters can show the wrong list of masters
......................................................................
KUDU-2039: /masters can show the wrong list of masters
The ListMasters RPC returned masters based on the -master_addresses
flag. It should return masters based on the consensus configuration.
Since we verify the two sets of addresses are the same at startup, this
usually makes no difference. However, if a quorum master is
replaced by a server with a new UUID, previously we would report it
in ListMasters and therefore on /masters. This patch adds an
additional check that the UUIDs match, and returns an error
along with the registration information if there is a mismatch.
Screenshot for down master: https://github.com/wdberkeley/kudu/blob/kudu2309screenshots/www/mastersonedown.png
Screenshot for UUID mismatch: https://github.com/wdberkeley/kudu/blob/kudu2309screenshots/www/mastersmismatch.png
Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
---
M src/kudu/master/master.cc
M src/kudu/master/master_path_handlers.cc
M src/kudu/master/sys_catalog.h
M www/masters.mustache
4 files changed, 46 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/9378/2
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2309: /masters can show the wrong list of masters
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9378 )
Change subject: KUDU-2309: /masters can show the wrong list of masters
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9378/3/src/kudu/master/master.cc
File src/kudu/master/master.cc:
http://gerrit.cloudera.org:8080/#/c/9378/3/src/kudu/master/master.cc@331
PS3, Line 331: LOG(WARNING) << s.ToString();
> Just another thought: maybe, it's worth at least add the UUID of the peer f
Done
http://gerrit.cloudera.org:8080/#/c/9378/3/www/masters.mustache
File www/masters.mustache:
http://gerrit.cloudera.org:8080/#/c/9378/3/www/masters.mustache@40
PS3, Line 40: {{^has_no_errors}}
> Does it make sense to show this if there are no errors?
Done
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache
File www/masters.mustache:
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache@40
PS4, Line 40: {{^has_no_errors}}
> mustache doesn't have a way to condition based on the length of the 'errors
Yep. You can say "if the array is empty, do this" and "if the array is nonempty, do this for each element", but you can't say "if the array is nonempty, do this *once*", AFAIK.
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache@40
PS4, Line 40: {{^has_no_errors}}
> "An inverted section begins with a caret (hat) and ends with a slash. That
In that case, the {{#errors}} section would need to surround the whole <table> element in order to have it be suppressed when there are no errors-- but then there'd be one table per error.
I spent a bit of time trying things to make this exact situation work while doing previous mustache patches for Kudu, and I think this is the simplest way to do it. I'd be happy if someone had a better way, though.
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 06 Mar 2018 17:06:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2309: /masters can show the wrong list of masters
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9378
to look at the new patch set (#3).
Change subject: KUDU-2309: /masters can show the wrong list of masters
......................................................................
KUDU-2309: /masters can show the wrong list of masters
The ListMasters RPC (on which the /masters page is based) returned
masters based on the -master_addresses flag. It should return masters
based on the consensus configuration. Since we verify the two sets of
addresses are the same at startup, this usually makes no difference.
However, if a master is replaced by a server with a new UUID,
previously we would report the new UUID, even though it may not be
part of the previously established quorum. This patch adds an
additional check that the UUIDs match, and returns an error along with
the registration information if there is a mismatch.
Screenshot for down master: https://github.com/wdberkeley/kudu/blob/kudu2309screenshots/www/mastersonedown.png
Screenshot for UUID mismatch: https://github.com/wdberkeley/kudu/blob/kudu2309screenshots/www/mastersmismatch.png
Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
---
M src/kudu/master/master.cc
M src/kudu/master/master_path_handlers.cc
M src/kudu/master/sys_catalog.h
M www/masters.mustache
4 files changed, 47 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/9378/3
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>