Re: Review Request 43271: Modify subprocess to deal with LIBPROCESS_PORT specially.

[Shuai Lin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43271/#review120714
---




3rdparty/libprocess/src/subprocess.cpp (line 345)


Nitpick: would the warning always be logged when forking mesos-fetcher? 
Because it would use a copy of `os::environment()`, and #43272 removes the 
logic that removes `LIBPROCESS_PORT` from it?


- Shuai Lin


On Feb. 19, 2016, 6:54 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43271/
> ---
> 
> (Updated Feb. 19, 2016, 6:54 p.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/src/subprocess.cpp 
> 44ca6d0869f3dbcfda1ac01d0d6b79dc20c4267c 
> 
> Diff: https://reviews.apache.org/r/43271/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> Tests are run in the next review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43271: Modify subprocess to deal with LIBPROCESS_PORT specially.

[Joseph Wu]

> On Feb. 19, 2016, 2:32 a.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 329
> > 
> >
> > 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 subprocess_environment(
> > const Option>& 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 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 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;
> > }

Note: We can't use the `operator[]` because it's not const.


> On Feb. 19, 2016, 2:32 a.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 450
> > 
> >
> > 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.

Note: I pulled out the temp var since we will never fully inherit the 
environment with this change.  So the surrounding `if` (in the original) is no 
longer necessary.


- Joseph


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43271/#review119851
---


On Feb. 19, 2016, 10:54 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43271/
> ---
> 
> (Updated Feb. 19, 2016, 10:54 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/src/subprocess.cpp 
> 44ca6d0869f3dbcfda1ac01d0d6b79dc20c4267c 
> 
> Diff: https://reviews.apache.org/r/43271/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> Tests are run in the next review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43271: Modify subprocess to deal with LIBPROCESS_PORT specially.

[Joseph Wu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43271/
---

(Updated Feb. 19, 2016, 10:54 a.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Till 
Toenshoff.


Changes
---

Made `subprocess_environment` a private (static) function.  Tweaked some 
comments.


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 (updated)
-

  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

Re: Review Request 43271: Modify subprocess to deal with LIBPROCESS_PORT specially.

[Bernd Mathiske]

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


This can be a static function that does not show up in the header at all.



3rdparty/libprocess/include/process/subprocess.hpp (line 264)


s/method/function/



3rdparty/libprocess/include/process/subprocess.hpp (line 265)


s/passing//



3rdparty/libprocess/src/subprocess.cpp (line 328)


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 subprocess_environment(
const Option>& 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 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 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)


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

Re: Review Request 43271: Modify subprocess to deal with LIBPROCESS_PORT specially.

[Joseph Wu]

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


Changes
---

Moved remaining environment processing from `subprocess` into 
`subprocess_environment`.  Expanded some comments.


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 (updated)
-

  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

Re: Review Request 43271: Modify subprocess to deal with LIBPROCESS_PORT specially.

[Bernd Mathiske]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43271/#review119619
---




3rdparty/libprocess/src/subprocess.cpp (line 303)


I know which vars you mean here, but other readers won't know. I suggest to 
name them explicitly.



3rdparty/libprocess/src/subprocess.cpp (line 382)


Breaking out subprocess_environment but not the code below as a subroutine 
looks unbalanced. Why not factor the whole affair into a common subroutine 
together?

This routine may look like a filter function: env -> env.


- Bernd Mathiske


On Feb. 5, 2016, 3:25 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43271/
> ---
> 
> (Updated Feb. 5, 2016, 3:25 p.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 
> bb50cc3070245a294fa16efe44f14ae893bc5518 
>   3rdparty/libprocess/src/subprocess.cpp 
> ff477e37a9619c780bddd5a8e629fa981b729715 
> 
> Diff: https://reviews.apache.org/r/43271/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> Tests are run in the next review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>