----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11975/#review22219 -----------------------------------------------------------
src/detector/detector.hpp <https://reviews.apache.org/r/11975/#comment45641> please align the arguments as: BasicMasterDetector(const ...., const); Actually the new style is to put each argument on a new line. But if you decide to do that, please change all the functions in this file for consistency. src/detector/detector.hpp <https://reviews.apache.org/r/11975/#comment45642> ditto. src/detector/detector.hpp <https://reviews.apache.org/r/11975/#comment45643> ditto. src/detector/detector.cpp <https://reviews.apache.org/r/11975/#comment45645> alignment. src/detector/detector.cpp <https://reviews.apache.org/r/11975/#comment45648> thank you src/detector/detector.cpp <https://reviews.apache.org/r/11975/#comment45649> Hmmm. This doesn't read correct. You probably want: "Host " << hostname << " created ..."; ? src/detector/detector.cpp <https://reviews.apache.org/r/11975/#comment45651> Why not add the hostname in the data when we create the znode above? I think having 2 ephemeral nodes makes it complex to reason about master changes. For example what does it mean if 'result' znode went away but 'result'-hostname did not? Of course, if we decide to add hostname to the existing znode, we need to carefully plan the deployment strategy to roll this kind of change out to production mesos clusters. For example, slaves need to be rolled first before masters. Also, new slaves (with this code) need to be backwards compatible with old masters (without this change). Thoughts? src/detector/detector.cpp <https://reviews.apache.org/r/11975/#comment45655> Can you use strings::endsWith() ? src/detector/detector.cpp <https://reviews.apache.org/r/11975/#comment45657> formatting. LOG(INFO) << .... << .... src/detector/detector.cpp <https://reviews.apache.org/r/11975/#comment45658> ditto. src/master/master.cpp <https://reviews.apache.org/r/11975/#comment45663> hmm. not sure if this helper is that useful. its not doing that much. can you inline this where needed? src/sched/sched.cpp <https://reviews.apache.org/r/11975/#comment45667> we typically use an 'Option' type here to signal that a parameter is optional. actually, this makes me think. do we need to pass the hostname to the detector. can't we just deduce the hostname from the pid? - Vinod Kone On June 19, 2013, 5:38 p.m., Brenden Matthews wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11975/ > ----------------------------------------------------------- > > (Updated June 19, 2013, 5:38 p.m.) > > > Review request for mesos. > > > Description > ------- > > Use hostname instead of IP for redirection. > > In the Web UI, the redirection does not work for hosts where the local > interface address is not publicly accessible. For example, with EC2 the > redirection will not work. > > This change writes the Master's hostname into ZooKeeper, thereby > allowing the redirection to work with private IP addresses. > > > Diffs > ----- > > src/detector/detector.hpp b0e66888050c1987b7200cdbf21ebe5e2e55e6c0 > src/detector/detector.cpp 12deefa0b9df3f4946d80f500caaa5199b8ea28e > src/local/local.cpp 3364c15f6b2374d752e83eb8a1c3c49de6bcc259 > src/master/http.cpp 47471d6881fa1b101cdeb3fb48a59712490267e9 > src/master/main.cpp 19fcb9f09d8fc03a0719aada7d216b575eb3069b > src/master/master.hpp 86c5232d57e9e86e13acf48b68f1e3cf23442495 > src/master/master.cpp 60c6d4f88f8024796c5e495d7cc6ddde2b754887 > src/messages/messages.proto 2c196eee0f3adba90799878b7e126bc85f635b65 > src/sched/sched.cpp 248e12ef4a608256454ee9f90c9c066d442a6d9f > src/slave/main.cpp 750a12766bde64059bfd4635ea077cbd43cb4301 > src/tests/cluster.hpp f743bb3251af81fb9d8afd51de4df6efcf289bb9 > src/tests/master_detector_tests.cpp > 57f4e3ef953537b7d102978bcfb30da331482e86 > src/webui/master/static/controllers.js > 31e7c91e3f07d252e83697aa42c5df3c2657eac2 > > Diff: https://reviews.apache.org/r/11975/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Brenden Matthews > >
