> On June 21, 2013, 1:40 a.m., Vinod Kone wrote: > > src/detector/detector.hpp, lines 95-96 > > <https://reviews.apache.org/r/11975/diff/1/?file=308451#file308451line95> > > > > 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. > >
Updated below. Let me know if it's what you meant. > On June 21, 2013, 1:40 a.m., Vinod Kone wrote: > > src/detector/detector.cpp, lines 294-295 > > <https://reviews.apache.org/r/11975/diff/1/?file=308452#file308452line294> > > > > Hmmm. This doesn't read correct. > > > > You probably want: > > "Host " << hostname << " created ..."; ? I'm not sure what you mean. It looks correct to me. > On June 21, 2013, 1:40 a.m., Vinod Kone wrote: > > src/detector/detector.cpp, lines 297-303 > > <https://reviews.apache.org/r/11975/diff/1/?file=308452#file308452line297> > > > > 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? > 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? Both nodes are ephemeral, and we always check for 'result' before 'result'-hostname, so it shouldn't matter if 'result'-hostname exists when 'result' does not. If ZooKeeper is being consistent, this shouldn't ever occur. Using 2 separate znodes allows us to have compatibility between versions and gracefully handle failure. > On June 21, 2013, 1:40 a.m., Vinod Kone wrote: > > src/detector/detector.cpp, line 450 > > <https://reviews.apache.org/r/11975/diff/1/?file=308452#file308452line450> > > > > Can you use strings::endsWith() ? Good idea! > On June 21, 2013, 1:40 a.m., Vinod Kone wrote: > > src/sched/sched.cpp, line 115 > > <https://reviews.apache.org/r/11975/diff/1/?file=308459#file308459line115> > > > > 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? We could indeed do a DNS lookup on the IP. This depends on the reverse DNS records being correct, however. I don't think most people will have these set correctly. - Brenden ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11975/#review22219 ----------------------------------------------------------- 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. > > > Repository: 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 > >
