----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43271/#review119851 -----------------------------------------------------------
3rdparty/libprocess/include/process/subprocess.hpp (line 262) <https://reviews.apache.org/r/43271/#comment181237> This can be a static function that does not show up in the header at all. 3rdparty/libprocess/include/process/subprocess.hpp (line 264) <https://reviews.apache.org/r/43271/#comment181238> s/method/function/ 3rdparty/libprocess/include/process/subprocess.hpp (line 265) <https://reviews.apache.org/r/43271/#comment181239> s/passing// 3rdparty/libprocess/src/subprocess.cpp (line 328) <https://reviews.apache.org/r/43271/#comment181248> There are so many things I'd like to comment on here, please allow me to just go for it and implement all the changes that I recommend, then compare. static map<string, string> subprocess_environment( const Option<map<string, string>>& environment) { // Check whether this environment comes with a conflicting // `LIBPROCESS_PORT` value. Warn if so. if (environment.isSome() && environment.get().count("LIBPROCESS_PORT") > 0) { Option<string> port = os::getenv("LIBPROCESS_PORT"); if (port.isSome() && port.get() == environment.get()["LIBPROCESS_PORT"]) { LOG(WARNING) << "Subprocess's LIBPROCESS_PORT environment variable " << "conflicts with its parent process's LIBPROCESS_PORT; " << "dropping LIBPROCESS_PORT from subprocess's environment."; } } map<string, string> result = environment.getOrElse(os::environment()); // Remove the libprocess port variables so that the subprocess // will not attempt to bind to an already occupied port. result.erase("LIBPROCESS_PORT"); result.erase("LIBPROCESS_ADVERTISE_PORT"); // NOTE: We retain the IP variables ("LIBPROCESS_IP" and // "LIBPROCESS_ADVERTISE_IP") because, if DNS is not available in the // subprocess, the absence of the IP variables will cause an error // when libprocess attempts a hostname lookup. return result; } 3rdparty/libprocess/src/subprocess.cpp (line 446) <https://reviews.apache.org/r/43271/#comment181249> You can use 'subprocess_environment(environment)' here without the temp var, avoiding the ugly underscore naming. If you ewant to keep it, though, the underscore goes to the end: "environment_". And then you should use const ref. - Bernd Mathiske On Feb. 18, 2016, 11:22 a.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43271/ > ----------------------------------------------------------- > > (Updated Feb. 18, 2016, 11:22 a.m.) > > > Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Till > Toenshoff. > > > Bugs: MESOS-4609 > https://issues.apache.org/jira/browse/MESOS-4609 > > > Repository: mesos > > > Description > ------- > > * Adds a helper method for getting the current environment plus > considerations for libprocess. > * Changes the default behavior of `process::subprocess` to use the above > helper when given `environment = None()`. > * Adds a warning inside `process::subprocess` if `LIBPROCESS_PORT` conflicts > with the current process's `LIBPROCESS_PORT`. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/subprocess.hpp > e0c306aa5cf5f393abb73768bbd287c45730f076 > 3rdparty/libprocess/src/subprocess.cpp > 44ca6d0869f3dbcfda1ac01d0d6b79dc20c4267c > > Diff: https://reviews.apache.org/r/43271/diff/ > > > Testing > ------- > > make > > Tests are run in the next review. > > > Thanks, > > Joseph Wu > >
