Adar Dembo has posted comments on this change.

Change subject: KUDU-501 Redirect to leader master web UI
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8068/2//COMMIT_MSG
Commit Message:

PS2, Line 12: a
Nit: an


http://gerrit.cloudera.org:8080/#/c/8068/2/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

Line 214:     (*output)["error"] = Substitute("Master is not ready: ", 
l.catalog_status().ToString());
Missing an $0 here.

A unit test would have caught this. Perhaps you could add one? We have the 
PeriodicWebUIChecker and several multi-master tests you could reuse.


PS2, Line 654:  ServerEntryPB
Nit: auto is fine here too, since the scope is short.


Line 671:   return Status::NotFound("no leader master known to this master");
Do you think it would it be useful to add some representation of 'masters' to 
the message? Perhaps the errors, if they exist?


http://gerrit.cloudera.org:8080/#/c/8068/2/www/table.mustache
File www/table.mustache:

Line 26:   <div>You can find this page on the <a href="{{{.}}}">leader master's 
web UI</a>.</div>
Hmm, we can't do a fancy HTTP automatic redirect? Via a 3xx status code 
(https://en.wikipedia.org/wiki/URL_redirection#HTTP_status_codes_3xx)?


-- 
To view, visit http://gerrit.cloudera.org:8080/8068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If31543b7899976ec02704111e9e789035c44dfe1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <[email protected]>
Gerrit-HasComments: Yes

Reply via email to