Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.
--- 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 50002: Allow all flags load methods to specify a prefix.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50002/ --- (Updated Aug. 10, 2016, 11:24 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 --- Allow all flags load methods to specify a prefix. This also refactors the prefix logic into one place, so that's nice. Required for the actual fix for passing work_dir in local. Diffs (updated) - 3rdparty/stout/include/stout/flags/flags.hpp dd93627 3rdparty/stout/tests/flags_tests.cpp 848d707 Diff: https://reviews.apache.org/r/50002/diff/ Testing --- Added additional tests to flags_tests.cpp to ensure that prefix works on the other methods. `make check` passes Thanks, Ammar Askar
Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.
> On Aug. 6, 2016, 12:29 a.m., Vinod Kone wrote:
> > src/local/local.cpp, lines 179-180
> > <https://reviews.apache.org/r/50003/diff/7/?file=1448796#file1448796line179>
> >
> > 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 50002: Allow all flags load methods to specify a prefix.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50002/ --- (Updated Aug. 5, 2016, 11:58 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 --- Allow all flags load methods to specify a prefix. This also refactors the prefix logic into one place, so that's nice. Required for the actual fix for passing work_dir in local. Diffs (updated) - 3rdparty/stout/include/stout/flags/flags.hpp dd93627 3rdparty/stout/tests/flags_tests.cpp 77f3a6a Diff: https://reviews.apache.org/r/50002/diff/ Testing --- Added additional tests to flags_tests.cpp to ensure that prefix works on the other methods. `make check` passes Thanks, Ammar Askar
Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.
> 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.
--- 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.
--- 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.
> On July 18, 2016, 10:30 p.m., Greg Mann wrote: > > src/tests/test_framework_test.sh, line 24 > > <https://reviews.apache.org/r/50003/diff/4/?file=1445257#file1445257line24> > > > > 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.
--- 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.
> On July 15, 2016, 5:41 p.m., Greg Mann wrote: > > src/local/flags.hpp, line 33 > > <https://reviews.apache.org/r/50003/diff/2/?file=1444262#file1444262line33> > > > > 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.
> 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.
--- 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 50002: Allow all flags load methods to specify a prefix.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50002/ --- (Updated July 15, 2016, 4:45 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 --- Allow all flags load methods to specify a prefix. This also refactors the prefix logic into one place, so that's nice. Required for the actual fix for passing work_dir in local. Diffs (updated) - 3rdparty/stout/include/stout/flags/flags.hpp dd93627 3rdparty/stout/tests/flags_tests.cpp 77f3a6a Diff: https://reviews.apache.org/r/50002/diff/ Testing --- Added additional tests to flags_tests.cpp to ensure that prefix works on the other methods. `make check` passes Thanks, Ammar Askar
Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.
--- 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 50002: Allow all flags load methods to specify a prefix.
> On July 14, 2016, 1:27 a.m., Vinod Kone wrote: > > 3rdparty/stout/tests/flags_tests.cpp, line 546 > > <https://reviews.apache.org/r/50002/diff/1/?file=1443425#file1443425line546> > > > > s/test_option_map/values2/ Scoped so they can share the same name of `values` - Ammar --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50002/#review142181 --- On July 14, 2016, 10:10 p.m., Ammar Askar wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50002/ > --- > > (Updated July 14, 2016, 10:10 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 > --- > > Allow all flags load methods to specify a prefix. > > This also refactors the prefix logic into one place, so that's nice. > Required for the actual fix for passing work_dir in local. > > > Diffs > - > > 3rdparty/stout/include/stout/flags/flags.hpp dd93627 > 3rdparty/stout/tests/flags_tests.cpp 77f3a6a > > Diff: https://reviews.apache.org/r/50002/diff/ > > > Testing > --- > > Added additional tests to flags_tests.cpp to ensure that prefix works on the > other methods. > > `make check` passes > > > Thanks, > > Ammar Askar > >
Re: Review Request 50002: Allow all flags load methods to specify a prefix.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50002/ --- (Updated July 14, 2016, 10:10 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 --- Allow all flags load methods to specify a prefix. This also refactors the prefix logic into one place, so that's nice. Required for the actual fix for passing work_dir in local. Diffs (updated) - 3rdparty/stout/include/stout/flags/flags.hpp dd93627 3rdparty/stout/tests/flags_tests.cpp 77f3a6a Diff: https://reviews.apache.org/r/50002/diff/ Testing --- Added additional tests to flags_tests.cpp to ensure that prefix works on the other methods. `make check` passes Thanks, Ammar Askar
Re: Review Request 50002: Allow all flags load methods to specify a prefix.
> On July 14, 2016, 4:44 p.m., Greg Mann wrote:
> > 3rdparty/stout/tests/flags_tests.cpp, line 546
> > <https://reviews.apache.org/r/50002/diff/1/?file=1443425#file1443425line546>
> >
> > Another option would be to scope the flag loading/checking so that you
> > can use the same variable name for each map:
> > ```cpp
> > {
> > flags = TestFlags();
> > std::map<std::string, Option> values;
> > values["name1"] = "billy joel";
> > values["name2"] = "51";
> >
> > // load(map<string, Option, unknowns, prefix)
> > Try load = flags.load(values, false, "FLAGSTEST_");
> >
> > EXPECT_SOME(load);
> > EXPECT_EQ(0, load->warnings.size());
> >
> > EXPECT_EQ(flags.name1, "billy joel");
> > EXPECT_EQ(flags.name2, 51);
> >
> > EXPECT_SOME(load);
> > EXPECT_EQ(0, load->warnings.size());
> >
> > EXPECT_EQ(flags.name1, "billy joel");
> > EXPECT_EQ(flags.name2, 51);
> > }
> > ```
This sounds good, it also helps seperate which method is being tested than
whitespace.
- Ammar
-------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50002/#review142246
---
On July 13, 2016, 8:53 p.m., Ammar Askar wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50002/
> ---
>
> (Updated July 13, 2016, 8:53 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
> ---
>
> Allow all flags load methods to specify a prefix.
>
> This also refactors the prefix logic into one place, so that's nice.
> Required for the actual fix for passing work_dir in local.
>
>
> Diffs
> -
>
> 3rdparty/stout/include/stout/flags/flags.hpp
> dd9362772d1fbd32638fc7e70126fd49d4a03c68
> 3rdparty/stout/tests/flags_tests.cpp
> 77f3a6af110da1ffcdf2b7ab2b66431a6b5c91d3
>
> Diff: https://reviews.apache.org/r/50002/diff/
>
>
> Testing
> ---
>
> Added additional tests to flags_tests.cpp to ensure that prefix works on the
> other methods.
>
> `make check` passes
>
>
> Thanks,
>
> Ammar Askar
>
>
Review Request 50003: Propagate work_dir flag from local runs to agents/masters.
--- 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
Review Request 50002: Allow all flags load methods to specify a prefix.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50002/ --- Review request for mesos, Greg Mann and Vinod Kone. Bugs: MESOS-5613 https://issues.apache.org/jira/browse/MESOS-5613 Repository: mesos Description --- Allow all flags load methods to specify a prefix. This also refactors the prefix logic into one place, so that's nice. Required for the actual fix for passing work_dir in local. Diffs - 3rdparty/stout/include/stout/flags/flags.hpp dd9362772d1fbd32638fc7e70126fd49d4a03c68 3rdparty/stout/tests/flags_tests.cpp 77f3a6af110da1ffcdf2b7ab2b66431a6b5c91d3 Diff: https://reviews.apache.org/r/50002/diff/ Testing --- Added additional tests to flags_tests.cpp to ensure that prefix works on the other methods. `make check` passes Thanks, Ammar Askar
