Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[haosdent huang]

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




src/local/local.cpp (line 183)


The `flags` is shadown with the outside variable `flags`. Need to rename to 
`masterFlags`.


- haosdent huang


On Aug. 10, 2016, 11:28 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated Aug. 10, 2016, 11:28 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp d6fe4d0 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

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

(Updated Aug. 10, 2016, 11:28 p.m.)


Review request for mesos, Greg Mann and Vinod Kone.


Changes
---

rebased


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


Repository: mesos


Description
---

Propagate work_dir flag from local runs to agents/masters.


Diffs (updated)
-

  src/local/flags.hpp f0af0d2 
  src/local/local.cpp d6fe4d0 

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


Testing
---

Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
regression test for this would be good, please advise.


Thanks,

Ammar Askar

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Vinod Kone]

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



can you rebase?

- Vinod Kone


On July 21, 2016, 1:38 a.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 21, 2016, 1:38 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Vinod Kone]

> On Aug. 6, 2016, 12:29 a.m., Vinod Kone wrote:
> > src/local/local.cpp, lines 179-180
> > 
> >
> > not sure if 'propagated_flags' is the right name.
> > 
> > also, wondering if it would be intuitive to just do
> > 
> > os::setenv("MESOS_WORK_DIR") = flags.work_dir;
> 
> Ammar Askar wrote:
> >not sure if 'propagated_flags' is the right name.
> 
> The reasoning was they are being propogated from the local main()'s flags 
> to the agent's and master's flags.
> 
> 
> >also, wondering if it would be intuitive to just do
> 
> That would certainly work, the reason I didn't do it that way and opted 
> to take the approach with the map is to make this more extensible in the 
> future. So in case more flags need to be added to the local run. And if they 
> only needed to be provided to a master or just the agents, it's much easier 
> and clearer than fiddling with environment variables, in my opinion.

sgtm.


- Vinod


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


On July 21, 2016, 1:38 a.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 21, 2016, 1:38 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

> On Aug. 6, 2016, 12:29 a.m., Vinod Kone wrote:
> > src/local/local.cpp, lines 179-180
> > 
> >
> > not sure if 'propagated_flags' is the right name.
> > 
> > also, wondering if it would be intuitive to just do
> > 
> > os::setenv("MESOS_WORK_DIR") = flags.work_dir;

>not sure if 'propagated_flags' is the right name.

The reasoning was they are being propogated from the local main()'s flags to 
the agent's and master's flags.


>also, wondering if it would be intuitive to just do

That would certainly work, the reason I didn't do it that way and opted to take 
the approach with the map is to make this more extensible in the future. So in 
case more flags need to be added to the local run. And if they only needed to 
be provided to a master or just the agents, it's much easier and clearer than 
fiddling with environment variables, in my opinion.


- Ammar


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


On July 21, 2016, 1:38 a.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 21, 2016, 1:38 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Vinod Kone]

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


Fix it, then Ship it!





src/local/local.cpp (lines 179 - 180)


not sure if 'propagated_flags' is the right name.

also, wondering if it would be intuitive to just do

os::setenv("MESOS_WORK_DIR") = flags.work_dir;


- Vinod Kone


On July 21, 2016, 1:38 a.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 21, 2016, 1:38 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

> On July 25, 2016, 6:04 p.m., Greg Mann wrote:
> > Thanks Ammar! Patches look good to me; Vinod is going to take a look when 
> > he has some cycles.

bump


- Ammar


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


On July 21, 2016, 1:38 a.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 21, 2016, 1:38 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Greg Mann]

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



Thanks Ammar! Patches look good to me; Vinod is going to take a look when he 
has some cycles.

- Greg Mann


On July 21, 2016, 1:38 a.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 21, 2016, 1:38 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50002, 50003]

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 July 21, 2016, 1:38 a.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 21, 2016, 1:38 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

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

(Updated July 21, 2016, 1:38 a.m.)


Review request for mesos, Greg Mann and Vinod Kone.


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


Repository: mesos


Description
---

Propagate work_dir flag from local runs to agents/masters.


Diffs (updated)
-

  src/local/flags.hpp f0af0d2 
  src/local/local.cpp a543aef 

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


Testing
---

Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
regression test for this would be good, please advise.


Thanks,

Ammar Askar

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Greg Mann]

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




src/local/flags.hpp (line 50)


Hi Ammar: sorry for the extra comment here, but I actually just discovered 
the helper function `os::temp()`. We use it 
[here](https://github.com/apache/mesos/blob/master/src/slave/flags.cpp#L129). 
Since it looks like this is our accepted way to set default temporary 
locations, we should follow that pattern here as well.


- Greg Mann


On July 20, 2016, 5:10 a.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 20, 2016, 5:10 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50002, 50003]

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 July 20, 2016, 5:10 a.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 20, 2016, 5:10 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

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

(Updated July 20, 2016, 5:10 a.m.)


Review request for mesos, Greg Mann and Vinod Kone.


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


Repository: mesos


Description
---

Propagate work_dir flag from local runs to agents/masters.


Diffs (updated)
-

  src/local/flags.hpp f0af0d2 
  src/local/local.cpp a543aef 

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


Testing
---

Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
regression test for this would be good, please advise.


Thanks,

Ammar Askar

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Greg Mann]

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


Fix it, then Ship it!





src/local/flags.hpp (lines 33 - 34)


s/work_dir/`work_dir`/


- Greg Mann


On July 18, 2016, 10:51 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 18, 2016, 10:51 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50002, 50003]

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 July 18, 2016, 10:51 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 18, 2016, 10:51 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

> On July 18, 2016, 10:30 p.m., Greg Mann wrote:
> > src/tests/test_framework_test.sh, line 24
> > 
> >
> > I see what you mean about the failure of these `atexit` statements - I 
> > also see lots of litter in my /tmp directory, and in fact, this 
> > TestFramework test was failing for me due to leftover agent metadata in 
> > `/tmp/mesos/local`.
> > 
> > Before modifying this test in this way, I'd like to get to the bottom 
> > of the `atexit` behavior so that this will pass reliably. If you like, I 
> > think you'd be safe getting rid of this part of the diff and leaving the 
> > TestFramework test as it is. I can cover the case where we don't set the 
> > `work_dir` as part of https://issues.apache.org/jira/browse/MESOS-5850

Removed the test change for now.


- Ammar


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


On July 18, 2016, 10:51 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 18, 2016, 10:51 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50002, 50003]

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 July 16, 2016, 2:14 a.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 16, 2016, 2:14 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
>   src/tests/test_framework_test.sh 73b4ddc 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

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

(Updated July 16, 2016, 2:14 a.m.)


Review request for mesos, Greg Mann and Vinod Kone.


Changes
---

rebased on latest master to hopefully fix the patch


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


Repository: mesos


Description
---

Propagate work_dir flag from local runs to agents/masters.


Diffs (updated)
-

  src/local/flags.hpp f0af0d2 
  src/local/local.cpp a543aef 
  src/tests/test_framework_test.sh 73b4ddc 

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


Testing
---

Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
regression test for this would be good, please advise.


Thanks,

Ammar Askar

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [50003, 50002]

Failed command: ./support/apply-review.sh -n -r 50003

Error:
2016-07-16 00:25:35 URL:https://reviews.apache.org/r/50003/diff/raw/ 
[4380/4380] -> "50003.patch" [1]
error: patch failed: src/tests/test_framework_test.sh:20
error: src/tests/test_framework_test.sh: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/14335/console

- Mesos ReviewBot


On July 15, 2016, 9:01 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 15, 2016, 9:01 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
>   src/tests/test_framework_test.sh 617ca52 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

> On July 15, 2016, 5:41 p.m., Greg Mann wrote:
> > src/local/flags.hpp, line 33
> > 
> >
> > I was originally put off by the duplication of the `work_dir` flag 
> > here, since it makes `work_dir` the only normal Mesos master/agent flag 
> > that can be specified at the command line. Another option I see is to 
> > manually check the value of `MESOS_WORK_DIR` and then set it via 
> > `flags.work_dir = DEFAULT` if necessary. However, I think I prefer the 
> > method of using our flag objects to express the default value rather than 
> > manually checking an environment variable.
> > 
> > Anyway! What do you think about adding a comment here explaining why 
> > we're doing this for `work_dir` but not any other flag? i.e., we need to 
> > set `work_dir` early because it's a required flag and `load` will barf if 
> > it's not already set. Local mode is the one case in which we deem it 
> > acceptable to use a default location within `/tmp` for the `work_dir`.

>since it makes work_dir the only normal Mesos master/agent flag that can be 
>specified at the command line.

Sure but that's just for now, in the future this could be expanded to pass down 
any flags to the agents/master from `local`

>Anyway! What do you think about adding a comment here explaining why we're 
>doing this for work_dir but not any other flag? i.e., we need to set work_dir 
>early because it's a required flag and load will barf if it's not already set. 
>Local mode is the one case in which we deem it acceptable to use a default 
>location within /tmp for the work_dir.

Done


- Ammar


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


On July 15, 2016, 9:01 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 15, 2016, 9:01 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
>   src/tests/test_framework_test.sh 617ca52 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

> On July 15, 2016, 5:41 p.m., Greg Mann wrote:
> > Regarding the regression test: several of our tests use this "local mode" 
> > to test example frameworks (see 'src/tests/examples_tests.cpp'), but they 
> > all set the MESOS_WORK_DIR env var before running. I also noticed that we 
> > don't have any tests which run the 'mesos-local' binary; I created a 
> > separate ticket for that: https://issues.apache.org/jira/browse/MESOS-5850.
> > 
> > One easy thing you could do to cover the issue which led to these patches 
> > is to modify one of the existing example framework test scripts to _not_ 
> > set MESOS_WORK_DIR before running. This would just involve removing lines 
> > like 
> > [these](https://github.com/apache/mesos/blob/4764768fc781bb99294039cebf072748f190bfba/src/tests/test_framework_test.sh#L25-L26).

Done, as a side note, I don't think the `atexit` stuff is working because if I 
do `ls /tmp/mesos*` after running `make check`, I still see a bunch of folders.


- Ammar


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


On July 15, 2016, 9:01 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 15, 2016, 9:01 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
>   src/tests/test_framework_test.sh 617ca52 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

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

(Updated July 15, 2016, 9:01 p.m.)


Review request for mesos, Greg Mann and Vinod Kone.


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


Repository: mesos


Description
---

Propagate work_dir flag from local runs to agents/masters.


Diffs (updated)
-

  src/local/flags.hpp f0af0d2 
  src/local/local.cpp a543aef 
  src/tests/test_framework_test.sh 617ca52 

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


Testing
---

Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
regression test for this would be good, please advise.


Thanks,

Ammar Askar

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Greg Mann]

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



Regarding the regression test: several of our tests use this "local mode" to 
test example frameworks (see 'src/tests/examples_tests.cpp'), but they all set 
the MESOS_WORK_DIR env var before running. I also noticed that we don't have 
any tests which run the 'mesos-local' binary; I created a separate ticket for 
that: https://issues.apache.org/jira/browse/MESOS-5850.

One easy thing you could do to cover the issue which led to these patches is to 
modify one of the existing example framework test scripts to _not_ set 
MESOS_WORK_DIR before running. This would just involve removing lines like 
[these](https://github.com/apache/mesos/blob/4764768fc781bb99294039cebf072748f190bfba/src/tests/test_framework_test.sh#L25-L26).


src/local/flags.hpp (line 33)


I was originally put off by the duplication of the `work_dir` flag here, 
since it makes `work_dir` the only normal Mesos master/agent flag that can be 
specified at the command line. Another option I see is to manually check the 
value of `MESOS_WORK_DIR` and then set it via `flags.work_dir = DEFAULT` if 
necessary. However, I think I prefer the method of using our flag objects to 
express the default value rather than manually checking an environment variable.

Anyway! What do you think about adding a comment here explaining why we're 
doing this for `work_dir` but not any other flag? i.e., we need to set 
`work_dir` early because it's a required flag and `load` will barf if it's not 
already set. Local mode is the one case in which we deem it acceptable to use a 
default location within `/tmp` for the `work_dir`.


- Greg Mann


On July 14, 2016, 10:11 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 14, 2016, 10:11 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50002, 50003]

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 July 14, 2016, 10:11 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 14, 2016, 10:11 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

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

(Updated July 14, 2016, 10:11 p.m.)


Review request for mesos, Greg Mann and Vinod Kone.


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


Repository: mesos


Description
---

Propagate work_dir flag from local runs to agents/masters.


Diffs (updated)
-

  src/local/flags.hpp f0af0d2 
  src/local/local.cpp a543aef 

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


Testing
---

Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
regression test for this would be good, please advise.


Thanks,

Ammar Askar

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Greg Mann]

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




src/local/flags.hpp (line 36)


This keeps the newlines here consistent with other help strings, but it 
does look a bit messy. Perhaps we should move the newlines to accommodate the 
line wrapping here?



src/local/flags.hpp (line 39)


s/masters/masters and agents/



src/local/flags.hpp (line 41)


`/var/lib/mesos` might be more appropriate here, since this isn't master or 
agent specific?


- Greg Mann


On July 13, 2016, 8:54 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 13, 2016, 8:54 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d251e004f305726e7e4fe7941c2d4081183 
>   src/local/local.cpp a543aef117fea62660d55435be4d66d30f8ee860 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50002, 50003]

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 July 13, 2016, 8:54 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 13, 2016, 8:54 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d251e004f305726e7e4fe7941c2d4081183 
>   src/local/local.cpp a543aef117fea62660d55435be4d66d30f8ee860 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

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

Review request for mesos, Greg Mann and Vinod Kone.


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


Repository: mesos


Description
---

Propagate work_dir flag from local runs to agents/masters.


Diffs
-

  src/local/flags.hpp f0af0d251e004f305726e7e4fe7941c2d4081183 
  src/local/local.cpp a543aef117fea62660d55435be4d66d30f8ee860 

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


Testing
---

Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
regression test for this would be good, please advise.


Thanks,

Ammar Askar