Re: Review Request 57762: Fixed environment duplication in command executor.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57762]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 24, 2017, 2:24 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57762/
> ---
> 
> (Updated March 24, 2017, 2:24 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Greg Mann, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-7263
> https://issues.apache.org/jira/browse/MESOS-7263
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp db703f054decbca62f7fbe98f5d28f6e4c6c9b0f 
> 
> 
> Diff: https://reviews.apache.org/r/57762/diff/7/
> 
> 
> Testing
> ---
> 
> make check + functional testing..
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 57762: Fixed environment duplication in command executor.

[Alexander Rukletsov]

> On March 24, 2017, 2:29 a.m., Adam B wrote:
> > src/launcher/executor.cpp
> > Lines 494 (patched)
> > 
> >
> > "Overwriting environment variable 'foo' with value from task 
> > environment."

This is not necessarily true. Duplicates might be in task environment as well.


- Alexander


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


On March 24, 2017, 2:24 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57762/
> ---
> 
> (Updated March 24, 2017, 2:24 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Greg Mann, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-7263
> https://issues.apache.org/jira/browse/MESOS-7263
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp db703f054decbca62f7fbe98f5d28f6e4c6c9b0f 
> 
> 
> Diff: https://reviews.apache.org/r/57762/diff/7/
> 
> 
> Testing
> ---
> 
> make check + functional testing..
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 57762: Fixed environment duplication in command executor.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On March 24, 2017, 2:24 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57762/
> ---
> 
> (Updated March 24, 2017, 2:24 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Greg Mann, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-7263
> https://issues.apache.org/jira/browse/MESOS-7263
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp db703f054decbca62f7fbe98f5d28f6e4c6c9b0f 
> 
> 
> Diff: https://reviews.apache.org/r/57762/diff/7/
> 
> 
> Testing
> ---
> 
> make check + functional testing..
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 57762: Fixed environment duplication in command executor.

[Adam B]

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


Fix it, then Ship it!




I've got some nits, but nothing major.


src/launcher/executor.cpp
Line 473 (original), 483 (patched)


Technically the default, so you don't have to set it.



src/launcher/executor.cpp
Lines 491 (patched)


Not my favorite wrapping, but I'd allow it. I'd probably prefer:
```
  foreach (const Environment::Variable& variable,
   taskEnvironment->variables()) {
```



src/launcher/executor.cpp
Lines 494 (patched)


"Overwriting environment variable 'foo' with value from task environment."



src/launcher/executor.cpp
Lines 497 (patched)


No need to overwrite if the value is the same. Not a big deal if you do 
though.



src/launcher/executor.cpp
Lines 507 (patched)


"Overwriting environment variable 'foo' with value from command 
environment."


- Adam B


On March 23, 2017, 7:24 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57762/
> ---
> 
> (Updated March 23, 2017, 7:24 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Greg Mann, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-7263
> https://issues.apache.org/jira/browse/MESOS-7263
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp db703f054decbca62f7fbe98f5d28f6e4c6c9b0f 
> 
> 
> Diff: https://reviews.apache.org/r/57762/diff/7/
> 
> 
> Testing
> ---
> 
> make check + functional testing..
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 57762: Fixed environment duplication in command executor.

[Till Toenshoff]

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

(Updated March 24, 2017, 2:24 a.m.)


Review request for mesos, Adam B, Benjamin Bannier, Greg Mann, Jie Yu, and 
Joseph Wu.


Summary (updated)
-

Fixed environment duplication in command executor.


Bugs: MESOS-7263
https://issues.apache.org/jira/browse/MESOS-7263


Repository: mesos


Description
---

see summary.


Diffs
-

  src/launcher/executor.cpp db703f054decbca62f7fbe98f5d28f6e4c6c9b0f 


Diff: https://reviews.apache.org/r/57762/diff/7/


Testing
---

make check + functional testing..


Thanks,

Till Toenshoff