-----------------------------------------------------------
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
> 
>

Reply via email to