> On 2012-03-26 23:09:55, Matei Zaharia wrote: > > README, line 121 > > <https://reviews.apache.org/r/4491/diff/1/?file=95961#file95961line121> > > > > You might want to provide an example of installing to a non-local > > prefix, because some users will probably not be familiar with the concept. > > Then refer back to this at the start of the next section.
Done. > On 2012-03-26 23:09:55, Matei Zaharia wrote: > > README, line 129 > > <https://reviews.apache.org/r/4491/diff/1/?file=95961#file95961line129> > > > > Put prefix in [brackets] Done, here and everywhere else. > On 2012-03-26 23:09:55, Matei Zaharia wrote: > > README, line 151 > > <https://reviews.apache.org/r/4491/diff/1/?file=95961#file95961line151> > > > > Why is this not called start-mesos anymore? If it's to keep it in line > > with the rest of the binaries we put on the PATH, I would go for something > > like mesos-start-cluster.sh, which seems more grammatically correct than > > just "start". > > Andy Konwinski wrote: > I like this idea. Fixed. > On 2012-03-26 23:09:55, Matei Zaharia wrote: > > src/Makefile.am, line 40 > > <https://reviews.apache.org/r/4491/diff/1/?file=95963#file95963line40> > > > > Just to double-check, if someone does a make install, then modifies > > some config files, and then downloads a new version of Mesos and does make > > install again, will their directories be overwritten? Or does install > > protect the existing files? > > Andy Konwinski wrote: > This is a great point, and we should be very careful about this. I'm not > sure what the exact process for re-installing mesos is actually, but I think > it is simply to run make install again after building a newer version of > Mesos. In that case, perhaps we should only install *-env.sh.template files > instead of *.-env.sh files. Changed to install *.template. The deploy scripts don't barf if none exist either. > On 2012-03-26 23:09:55, Matei Zaharia wrote: > > src/deploy/mesos-deploy-env.sh.in, line 5 > > <https://reviews.apache.org/r/4491/diff/1/?file=95970#file95970line5> > > > > Would be nice to have a list of the variables that can be set here. There are only two for now, those listed. > On 2012-03-26 23:09:55, Matei Zaharia wrote: > > src/deploy/mesos-masters-env.sh.in, line 6 > > <https://reviews.apache.org/r/4491/diff/1/?file=95972#file95972line6> > > > > Say that you can run mesos-master --help for a list of parameters. Done. > On 2012-03-26 23:09:55, Matei Zaharia wrote: > > src/tests/master_detector_tests.cpp, line 90 > > <https://reviews.apache.org/r/4491/diff/1/?file=95987#file95987line90> > > > > Was this included in the patch by mistake? Yes. Removed for another patch. > On 2012-03-26 23:09:55, Matei Zaharia wrote: > > README, line 141 > > <https://reviews.apache.org/r/4491/diff/1/?file=95961#file95961line141> > > > > But the slaves-env.sh file needs to be on all the nodes, right? If the > > intent is to ship the options automatically, that would make it hard for > > the user to configure, say, different numbers of CPUs and memory per host, > > or a different LIBPROCESS_IP, or stuff like that. > > Andy Konwinski wrote: > I chatted with Matei offline about this. The current flow does not ship > the slaves-env.sh file over. If people wanted to set up unique per-machine > slave configurations, they can create a config file somewhere on each such > machine containing the special settings for that machine, and then set their > slaves-env.sh to point to that via the > --conf=/location/of/mesos/conf/file/on/slave/machines, and the values set in > those files would override anything passed over via the deploy scripts as env > variables. > > This raises the important issue of what the order of precedence is for > options that are set in multipole of {env variable, configuration file, > command line} (note that this is not a new issue). > https://github.com/mesos/mesos/wiki/Configuration currently says > "Configuration values are searched for first on the command line, then in the > environment, and then in the config file." but we should expand that comment > to say explicitly what happens if a variable is set in more than one place > (maybe even give an example). > > Matei Zaharia wrote: > Just to make it clear, the goal of the deploy scripts was never to ship > the configuration automatically to all the machines. The old deploy-to-slaves > script happened to do that, but its main purpose is to let people developing > Mesos modify and recompile the code locally, then sync it to all the > machines. I think it's fine to tell the user to configure each machine > separately -- in fact they won't have to do a single thing by default because > most users don't set any config settings on slaves. Otherwise, the mechanisms > for "global" and "local" configuration should be consistent (both environment > variables or both files). > > Matei Zaharia wrote: > By the way, my suggestion with this kind of stuff is that if you don't > want to spend a long time thinking about how to do it, find another project > that uses a similar configuration process, and copy what it does. Otherwise, > you will repeat mistakes that others have found and fixed in the past. This > is why the old scripts were based on per-machine config files in a key-value > pair format -- because a lot of other cluster software does that. Same thing > with the localstatedir / .template file issue. If in doubt go with .template > and tell the user to copy it. Woah, hold on a second. I was trying to get the _same_ semantics as the _old_ scripts. In the old scripts, if you had a local configuration file which had the 'url' field set the deploy scripts would actually use that when launching masters and slaves. That is to say, local configuration was used to create command line options for remote execution! I've stayed consistent with this methodology, just using mesos-*env.sh and environment variables rather than a configuration file in order to deal with naming conflicts of options between slaves and masters. But to be clear, as Andy mentioned, by setting MESOS_CONF any local configuration will get overridden by remote hosts configuration files. I would have been happy not implementing this, but I was _trying_ to keep the old workflow. > On 2012-03-26 23:09:55, Matei Zaharia wrote: > > src/deploy/mesos-start-masters.sh.in, line 36 > > <https://reviews.apache.org/r/4491/diff/1/?file=95974#file95974line36> > > > > What does this do? If you're trying to ship the env variables from the > > local config files to the machine, shouldn't you do export ${var} = value? > > Also, gotta be careful with quoting here. > > Andy Konwinski wrote: > I believe the `var` command prints one "KEY=value" pair per line so the > way Ben has it should work. Perhaps renaming the bash variable here from var > to var_and_value or var_value_pair would make it more clear. > > Also, I think you're right Matei that the way we're passing the > environment variables right now might break if any of those environment > variables contain double quotes (e.g. <code>export MESOS_CONFIG_VAR="say > \"hi\""</code>). One thing we could do is just state in the documentation (on > the wiki, in the README, and in the *-env.sh files themselves) that users > should not include any strings with any quotes (i.e. \") in them. > > Matei Zaharia wrote: > This is pretty hacky IMO -- would prefer to avoid this. Just having it in > the documentation doesn't mean it will be easy to find (a user might still > spend a while debugging). The problem is not only quotes but also spaces in > the variables, because env doesn't quite its outputs. For example: > > matei@mbk:~$ export MYVAR="hello world" > matei@mbk:~$ env | grep MYVAR > MYVAR=hello world I've updated the scripts to handle the quoting better. Again, this is an attempt to match the old semantics. However, in the old semantics, not all the configuration options specified in the local configuration would be used, only those that had been added to the deploy scripts and determined via 'mesos-getconf'. This sounds horrible to me, because it means as we add configuration options we also need to remember to add them into the deploy scripts (just satisfying all of the current options would have required a lot more work). - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4491/#review6383 ----------------------------------------------------------- On 2012-03-26 21:38:18, Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4491/ > ----------------------------------------------------------- > > (Updated 2012-03-26 21:38:18) > > > Review request for mesos, Andy Konwinski and Matei Zaharia. > > > Summary > ------- > > Updated deploy scripts to work with latest version of Mesos. Eliminated > 'deploy-to-slaves' functionality, instead we assume the binaries and > libraries will get installed via other means. Updated the semantics of where > configuration options are gathered from since a single configuration file was > insufficient given conflicting configuration options between binaries (e.g., > mesos-master and mesos-slave). Added a section to the README describing these > scripts. > > > Diffs > ----- > > README 8166b04 > configure.ac 7a0d89f > src/Makefile.am 090c07a > src/conf/deploy-env.sh.template a4565e2 > src/configurator/configurator.hpp ecd1365 > src/deploy/deploy-to-slaves 0e10486 > src/deploy/mesos-common-env.sh.in PRE-CREATION > src/deploy/mesos-daemon b3144c0 > src/deploy/mesos-daemon.sh.in PRE-CREATION > src/deploy/mesos-deploy-env.sh.in PRE-CREATION > src/deploy/mesos-env.sh c3dfda2 > src/deploy/mesos-masters-env.sh.in PRE-CREATION > src/deploy/mesos-slaves-env.sh.in PRE-CREATION > src/deploy/mesos-start-masters.sh.in PRE-CREATION > src/deploy/mesos-start-slaves.sh.in PRE-CREATION > src/deploy/mesos-start.sh.in PRE-CREATION > src/deploy/mesos-stop-masters.sh.in PRE-CREATION > src/deploy/mesos-stop-slaves.sh.in PRE-CREATION > src/deploy/mesos-stop.sh.in PRE-CREATION > src/deploy/start-masters a450ee0 > src/deploy/start-mesos 9e5b922 > src/deploy/start-slaves b3b931f > src/deploy/stop-masters f2aa9e2 > src/deploy/stop-mesos 679a09e > src/deploy/stop-slaves 141acd4 > src/master/main.cpp f31212b > src/tests/master_detector_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/4491/diff > > > Testing > ------- > > N/A > > > Thanks, > > Benjamin > >
