Will Berkeley has posted comments on this change. Change subject: KUDU-501 Redirect to leader master web UI ......................................................................
Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/8068/2//COMMIT_MSG Commit Message: PS2, Line 12: a > Nit: an Done 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. I enhanced webserver-stress-itest to run against multiple masters. It catches crash-causing substitute errors in this codepath pretty reliably, say 25-50% of the time. PS2, Line 650: if (!s.ok()) { : s = s.CloneAndPrepend("unable to list masters"); : return s; : } > RETURN_NOT_OK_PREPEND? Done PS2, Line 654: ServerEntryPB > Nit: auto is fine here too, since the scope is short. Done 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' /masters will have this info. Line 683: (*output)["leader_redirect"] = Substitute("$0/$1", leader_http_addr, path); > I'm a little nervous of endless redirect loops. Done http://gerrit.cloudera.org:8080/#/c/8068/2/src/kudu/master/master-path-handlers.h File src/kudu/master/master-path-handlers.h: PS2, Line 79: output > nit: can you surround this in 's? At first glance it didn't read as a varia Done 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 (ht I don't think the webserver lets us set a custom response code or headers very easily. It would at least require re-plumbing some web stuff. Also, I wouldn't want to immediately redirect to another master's web ui because I fear users would get confused about which master web ui they were looking at (I could see myself getting confused by this :p). We could probably add some timed redirect or something. Anyway, I think what's here is a good improvement for now, but I'd like to do a more automatic redirect later. -- 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: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
