Re: Review Request 47205: Added optional environment variable argument to mesos-docker-executor.

2016-05-27 Thread Kapil Arya

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


Ship it!




Ship It!

- Kapil Arya


On May 25, 2016, 10:02 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47205/
> ---
> 
> (Updated May 25, 2016, 10:02 p.m.)
> 
> 
> Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.
> 
> 
> Bugs: MESOS-5350
> https://issues.apache.org/jira/browse/MESOS-5350
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag opens up a way for hooks to specify environment variables for
> docker tasks.  Existing hooks can only affect the environment variables
> of docker executors.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.hpp 798ca3d4e261854a3b911d59929f2ca2afeb9ac8 
>   src/docker/executor.cpp 04f44044a80e1b355580ee3d1d8d301ac3baf565 
> 
> Diff: https://reviews.apache.org/r/47205/diff/
> 
> 
> Testing
> ---
> 
> See later reviews in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>



Re: Review Request 47205: Added optional environment variable argument to mesos-docker-executor.

2016-05-26 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On May 26, 2016, 2:02 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47205/
> ---
> 
> (Updated May 26, 2016, 2:02 a.m.)
> 
> 
> Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.
> 
> 
> Bugs: MESOS-5350
> https://issues.apache.org/jira/browse/MESOS-5350
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag opens up a way for hooks to specify environment variables for
> docker tasks.  Existing hooks can only affect the environment variables
> of docker executors.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.hpp 798ca3d4e261854a3b911d59929f2ca2afeb9ac8 
>   src/docker/executor.cpp 04f44044a80e1b355580ee3d1d8d301ac3baf565 
> 
> Diff: https://reviews.apache.org/r/47205/diff/
> 
> 
> Testing
> ---
> 
> See later reviews in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>



Re: Review Request 47205: Added optional environment variable argument to mesos-docker-executor.

2016-05-25 Thread Joseph Wu


> On May 25, 2016, 6:18 p.m., Jie Yu wrote:
> > src/docker/executor.hpp, line 90
> > 
> >
> > Can this be `Option`?

If I make this a `JSON::Object`, the invocation site 
(https://reviews.apache.org/r/47216/diff/2#1) will do one extra copy (see the 
next section):
It would go:
-> `map`
-> `map` 
-> `string`

Instead of:
-> `map`
-> `string`

The parsing path remains unchanged, except there's one less `Try` 
in the executor's main.
I prefer the current flag type.

---

The conversion would look like:
```
map task_environment = ;

JSON::Object json;
foreachpair (const string& key, const string& value, task_environment) {
  json.values[key] = value;
}
```


- Joseph


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


On May 25, 2016, 7:02 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47205/
> ---
> 
> (Updated May 25, 2016, 7:02 p.m.)
> 
> 
> Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.
> 
> 
> Bugs: MESOS-5350
> https://issues.apache.org/jira/browse/MESOS-5350
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag opens up a way for hooks to specify environment variables for
> docker tasks.  Existing hooks can only affect the environment variables
> of docker executors.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.hpp 798ca3d4e261854a3b911d59929f2ca2afeb9ac8 
>   src/docker/executor.cpp 04f44044a80e1b355580ee3d1d8d301ac3baf565 
> 
> Diff: https://reviews.apache.org/r/47205/diff/
> 
> 
> Testing
> ---
> 
> See later reviews in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>



Re: Review Request 47205: Added optional environment variable argument to mesos-docker-executor.

2016-05-25 Thread Joseph Wu

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

(Updated May 25, 2016, 7:02 p.m.)


Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.


Changes
---

Reorder some flags.


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


Repository: mesos


Description
---

This flag opens up a way for hooks to specify environment variables for
docker tasks.  Existing hooks can only affect the environment variables
of docker executors.


Diffs (updated)
-

  src/docker/executor.hpp 798ca3d4e261854a3b911d59929f2ca2afeb9ac8 
  src/docker/executor.cpp 04f44044a80e1b355580ee3d1d8d301ac3baf565 

Diff: https://reviews.apache.org/r/47205/diff/


Testing
---

See later reviews in chain.


Thanks,

Joseph Wu



Re: Review Request 47205: Added optional environment variable argument to mesos-docker-executor.

2016-05-25 Thread Jie Yu

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




src/docker/executor.hpp (line 89)


kill this line. In fact, I would reorder and move the one that's being 
deprecated to the end.



src/docker/executor.hpp (line 90)


Can this be `Option`?



src/docker/executor.cpp (line 680)


json->values



src/docker/executor.cpp (line 685)


I would align `<<` with the `<<` above.


- Jie Yu


On May 24, 2016, 9:02 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47205/
> ---
> 
> (Updated May 24, 2016, 9:02 p.m.)
> 
> 
> Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.
> 
> 
> Bugs: MESOS-5350
> https://issues.apache.org/jira/browse/MESOS-5350
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag opens up a way for hooks to specify environment variables for
> docker tasks.  Existing hooks can only affect the environment variables
> of docker executors.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.hpp 798ca3d4e261854a3b911d59929f2ca2afeb9ac8 
>   src/docker/executor.cpp 04f44044a80e1b355580ee3d1d8d301ac3baf565 
> 
> Diff: https://reviews.apache.org/r/47205/diff/
> 
> 
> Testing
> ---
> 
> See later reviews in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>



Re: Review Request 47205: Added optional environment variable argument to mesos-docker-executor.

2016-05-24 Thread Joseph Wu

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

(Updated May 24, 2016, 2:02 p.m.)


Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.


Changes
---

Move map validation logic into main.


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


Repository: mesos


Description
---

This flag opens up a way for hooks to specify environment variables for
docker tasks.  Existing hooks can only affect the environment variables
of docker executors.


Diffs (updated)
-

  src/docker/executor.hpp 798ca3d4e261854a3b911d59929f2ca2afeb9ac8 
  src/docker/executor.cpp 04f44044a80e1b355580ee3d1d8d301ac3baf565 

Diff: https://reviews.apache.org/r/47205/diff/


Testing
---

See later reviews in chain.


Thanks,

Joseph Wu



Re: Review Request 47205: Added optional environment variable argument to mesos-docker-executor.

2016-05-23 Thread Kapil Arya

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




src/docker/executor.hpp (lines 80 - 107)


Can we keep just a simple `add` here and move this logic to the cpp file?


- Kapil Arya


On May 11, 2016, 4:03 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47205/
> ---
> 
> (Updated May 11, 2016, 4:03 p.m.)
> 
> 
> Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.
> 
> 
> Bugs: MESOS-5350
> https://issues.apache.org/jira/browse/MESOS-5350
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag opens up a way for hooks to specify environment variables for
> docker tasks.  Existing hooks can only affect the environment variables
> of docker executors.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.hpp 798ca3d4e261854a3b911d59929f2ca2afeb9ac8 
>   src/docker/executor.cpp d60addcbe4a1869945ce42f4bb4b1e80e3f29f19 
> 
> Diff: https://reviews.apache.org/r/47205/diff/
> 
> 
> Testing
> ---
> 
> See later reviews in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>



Re: Review Request 47205: Added optional environment variable argument to mesos-docker-executor.

2016-05-11 Thread Joseph Wu


> On May 11, 2016, 12:26 p.m., Benjamin Bannier wrote:
> > src/docker/executor.hpp, line 120
> > 
> >
> > Any reason we wouldn't use an unordered map like e.g., a stout 
> > `hashmap` here and as function argument elsewhere in this patch?

This is meant to match the argument type of `docker->run`, which expects a 
`std::map`.


> On May 11, 2016, 12:26 p.m., Benjamin Bannier wrote:
> > src/docker/executor.cpp, line 502
> > 
> >
> > stout `hashmap`?

Same reason as above.


- Joseph


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


On May 11, 2016, 11:56 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47205/
> ---
> 
> (Updated May 11, 2016, 11:56 a.m.)
> 
> 
> Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.
> 
> 
> Bugs: MESOS-5350
> https://issues.apache.org/jira/browse/MESOS-5350
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag opens up a way for hooks to specify environment variables for
> docker tasks.  Existing hooks can only affect the environment variables
> of docker executors.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.hpp 798ca3d4e261854a3b911d59929f2ca2afeb9ac8 
>   src/docker/executor.cpp d60addcbe4a1869945ce42f4bb4b1e80e3f29f19 
> 
> Diff: https://reviews.apache.org/r/47205/diff/
> 
> 
> Testing
> ---
> 
> See later reviews in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>



Re: Review Request 47205: Added optional environment variable argument to mesos-docker-executor.

2016-05-11 Thread Joseph Wu

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

(Updated May 11, 2016, 1:03 p.m.)


Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.


Changes
---

Add some missing includes.


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


Repository: mesos


Description
---

This flag opens up a way for hooks to specify environment variables for
docker tasks.  Existing hooks can only affect the environment variables
of docker executors.


Diffs (updated)
-

  src/docker/executor.hpp 798ca3d4e261854a3b911d59929f2ca2afeb9ac8 
  src/docker/executor.cpp d60addcbe4a1869945ce42f4bb4b1e80e3f29f19 

Diff: https://reviews.apache.org/r/47205/diff/


Testing
---

See later reviews in chain.


Thanks,

Joseph Wu



Re: Review Request 47205: Added optional environment variable argument to mesos-docker-executor.

2016-05-11 Thread Benjamin Bannier

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




src/docker/executor.hpp (line 27)


`#include `



src/docker/executor.hpp (line 74)


Just as a note, since you call `add` with `_environment` instead of 
`::task_environment` you explicitly (and consistently with the other 
invocations here) select an overload of `FlagsBase::add` which will make this 
class non-copyable, see MESOS-3335. This will need to be cleaned up as part of 
MESOS-3335.



src/docker/executor.hpp (line 120)


Any reason we wouldn't use an unordered map like e.g., a stout `hashmap` 
here and as function argument elsewhere in this patch?



src/docker/executor.cpp (line 502)


stout `hashmap`?


- Benjamin Bannier


On May 11, 2016, 8:56 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47205/
> ---
> 
> (Updated May 11, 2016, 8:56 p.m.)
> 
> 
> Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.
> 
> 
> Bugs: MESOS-5350
> https://issues.apache.org/jira/browse/MESOS-5350
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag opens up a way for hooks to specify environment variables for
> docker tasks.  Existing hooks can only affect the environment variables
> of docker executors.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.hpp 798ca3d4e261854a3b911d59929f2ca2afeb9ac8 
>   src/docker/executor.cpp d60addcbe4a1869945ce42f4bb4b1e80e3f29f19 
> 
> Diff: https://reviews.apache.org/r/47205/diff/
> 
> 
> Testing
> ---
> 
> See later reviews in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>