[kudu-CR] KUDU-2309: /masters can show the wrong list of masters

[Will Berkeley (Code Review)]

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 
---
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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2309: /masters can show the wrong list of masters

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Tue, 06 Mar 2018 22:32:14 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2309: /masters can show the wrong list of masters

[Sailesh Mukil (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Tue, 06 Mar 2018 18:51:02 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2309: /masters can show the wrong list of masters

[Will Berkeley (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Tue, 06 Mar 2018 18:45:43 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2309: /masters can show the wrong list of masters

[Sailesh Mukil (Code Review)]

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 Errors
  

  UUID
  Error


{{#errors}}
  
{{uuid}}
{{error}}
  
{{/errors}}

  
  {{/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:


  Errors
  

  UUID
  Error


{{#errors}}
  
{{uuid}}
{{error}}
  
{{/errors}}
{{^errors}}
  
N/A
N/A
  
{{/errors}}

  

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Tue, 06 Mar 2018 18:05:46 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2309: /masters can show the wrong list of masters

[Will Berkeley (Code Review)]

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  
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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Tue, 06 Mar 2018 17:06:26 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2309: /masters can show the wrong list of masters

[Sailesh Mukil (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Mon, 05 Mar 2018 21:23:41 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2309: /masters can show the wrong list of masters

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Mon, 05 Mar 2018 19:18:05 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2309: /masters can show the wrong list of masters

[Will Berkeley (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2309: /masters can show the wrong list of masters

[Todd Lipcon (Code Review)]

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:   Errors
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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 23 Feb 2018 20:34:11 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2309: /masters can show the wrong list of masters

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Thu, 22 Feb 2018 23:26:11 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2309: /masters can show the wrong list of masters

[Will Berkeley (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley