Re: Review Request 46003: Removed the default value for agent work_dir.

[Greg Mann]

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

(Updated May 30, 2016, 11:18 p.m.)


Review request for mesos, Michael Park and Vinod Kone.


Changes
---

Addresed comments.


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


Repository: mesos


Description
---

The default value for the agent's `--work_dir` flag was removed, the
type of the parameter was changed to `Option`, and code was
added to explicitly set the work directory when necessary.


Diffs (updated)
-

  src/local/local.cpp c0c2766c9317b53a27eb50f6df1eb6ffd556a4cc 
  src/slave/flags.cpp d3543a019e57658cf60879f40c5f0834a1b8e836 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

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


Testing
---

`sudo make check`


Thanks,

Greg Mann

Re: Review Request 46003: Removed the default value for agent work_dir.

[Vinod Kone]

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


Fix it, then Ship it!





src/local/local.cpp (line 351)


use quotes instead of braces to wrap the work dir.



src/local/local.cpp (line 360)


ditt. use quotes.



src/slave/flags.cpp (line 184)


s/slave/agent/



src/slave/flags.cpp (line 188)


s/slaves/agents/



src/slave/flags.cpp (line 189)


s/slave/agent/


- Vinod Kone


On May 27, 2016, 11:47 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46003/
> ---
> 
> (Updated May 27, 2016, 11:47 a.m.)
> 
> 
> Review request for mesos, Michael Park and Vinod Kone.
> 
> 
> Bugs: MESOS-5064
> https://issues.apache.org/jira/browse/MESOS-5064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default value for the agent's `--work_dir` flag was removed, the
> type of the parameter was changed to `Option`, and code was
> added to explicitly set the work directory when necessary.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp c0c2766c9317b53a27eb50f6df1eb6ffd556a4cc 
>   src/slave/flags.cpp d3543a019e57658cf60879f40c5f0834a1b8e836 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 09742ff21513dc2570684d384b257868dd57a9ce 
> 
> Diff: https://reviews.apache.org/r/46003/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 46003: Removed the default value for agent work_dir.

[Greg Mann]

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

(Updated May 27, 2016, 11:47 a.m.)


Review request for mesos, Michael Park and Vinod Kone.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

The default value for the agent's `--work_dir` flag was removed, the
type of the parameter was changed to `Option`, and code was
added to explicitly set the work directory when necessary.


Diffs (updated)
-

  src/local/local.cpp c0c2766c9317b53a27eb50f6df1eb6ffd556a4cc 
  src/slave/flags.cpp d3543a019e57658cf60879f40c5f0834a1b8e836 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

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


Testing
---

`sudo make check`


Thanks,

Greg Mann

Re: Review Request 46003: Removed the default value for agent work_dir.

[Vinod Kone]

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




src/local/local.cpp (line 348)


s/workDirTry/mkdir/



src/tests/containerizer/mesos_containerizer_tests.cpp (lines 115 - 122)


it would be nice to create the temp directory inside the sandbox.

Try workDirectory = os::mkdir("./work_dir");

alternatively, we can add a mkdtemp() method to the TemporaryDirectoryTest 
fixture that gives us a temporary directory inside the sandbox.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 410)


ditto. create temp inside sandbox. see above.


- Vinod Kone


On May 7, 2016, 1:28 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46003/
> ---
> 
> (Updated May 7, 2016, 1:28 a.m.)
> 
> 
> Review request for mesos, Michael Park and Vinod Kone.
> 
> 
> Bugs: MESOS-5064
> https://issues.apache.org/jira/browse/MESOS-5064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default value for the agent's `--work_dir` flag was removed, the
> type of the parameter was changed to `Option`, and code was
> added to explicitly set the work directory when necessary.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp 1c679ecb486cb3d6184ec9a941f2ac5dbd2bcc1f 
>   src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 09742ff21513dc2570684d384b257868dd57a9ce 
> 
> Diff: https://reviews.apache.org/r/46003/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 46003: Removed the default value for agent work_dir.

[Greg Mann]

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

(Updated May 7, 2016, 1:28 a.m.)


Review request for mesos, Michael Park and Vinod Kone.


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


Repository: mesos


Description
---

The default value for the agent's `--work_dir` flag was removed, the
type of the parameter was changed to `Option`, and code was
added to explicitly set the work directory when necessary.


Diffs (updated)
-

  src/local/local.cpp 1c679ecb486cb3d6184ec9a941f2ac5dbd2bcc1f 
  src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

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


Testing
---

`sudo make check`


Thanks,

Greg Mann

Re: Review Request 46003: Removed the default value for agent work_dir.

[Greg Mann]

> On April 12, 2016, 12:44 a.m., Jie Yu wrote:
> > src/tests/containerizer/mesos_containerizer_tests.cpp, line 178
> > 
> >
> > Not yours, but can you follow up with a patch to use 
> > 'UUID::random().toString()' for all such cases in this file? Thanks!

Sure, no problem! Review is here: https://reviews.apache.org/r/46104/


> On April 12, 2016, 12:44 a.m., Jie Yu wrote:
> > src/tests/containerizer/mesos_containerizer_tests.cpp, line 338
> > 
> >
> > Why this change?

When 'environment.hpp' is included, the reference to `Environment` is ambiguous 
between `::mesos::Environment` (the protobuf message), and 
`::mesos::internal::tests::Environment` (the class defined in environment.hpp). 
Adding this explicit namespace resolves the ambiguity.


- Greg


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


On April 12, 2016, 5:45 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46003/
> ---
> 
> (Updated April 12, 2016, 5:45 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5064
> https://issues.apache.org/jira/browse/MESOS-5064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default value for the agent's `--work_dir` flag was removed, the
> type of the parameter was changed to `Option`, and code was
> added to explicitly set the work directory when necessary.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/slave/flags.hpp 300db49100d989d6c0409766b1341cb956ea1631 
>   src/slave/flags.cpp dd7bc9a48dfd8481336a2d2ec0beecd19a342644 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 09742ff21513dc2570684d384b257868dd57a9ce 
> 
> Diff: https://reviews.apache.org/r/46003/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 46003: Removed the default value for agent work_dir.

[Greg Mann]

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

(Updated April 12, 2016, 5:45 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

The default value for the agent's `--work_dir` flag was removed, the
type of the parameter was changed to `Option`, and code was
added to explicitly set the work directory when necessary.


Diffs (updated)
-

  src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
  src/slave/flags.hpp 300db49100d989d6c0409766b1341cb956ea1631 
  src/slave/flags.cpp dd7bc9a48dfd8481336a2d2ec0beecd19a342644 
  src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
  src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
  src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

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


Testing
---

`sudo make check`


Thanks,

Greg Mann

Re: Review Request 46003: Removed the default value for agent work_dir.

[Jie Yu]

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




src/local/local.cpp (line 353)


Instead of using CHECK_SOME(), let's check the error and use 
EXIT(EXIT_FAILURE) instead so that it fails gracefully.

Also, I think we should following the same pattern using a single top level 
temp directory with numerical sub-directories (e.g., 0, 1, 2) for slaves.



src/slave/flags.cpp (line 181)


not suitable for whe work directory in production.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 117)


Can you return Error instead?



src/tests/containerizer/mesos_containerizer_tests.cpp (line 178)


Not yours, but can you follow up with a patch to use 
'UUID::random().toString()' for all such cases in this file? Thanks!



src/tests/containerizer/mesos_containerizer_tests.cpp (line 338)


Why this change?


- Jie Yu


On April 11, 2016, 9:06 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46003/
> ---
> 
> (Updated April 11, 2016, 9:06 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5064
> https://issues.apache.org/jira/browse/MESOS-5064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default value for the agent's `--work_dir` flag was removed, the
> type of the parameter was changed to `Option`, and code was
> added to explicitly set the work directory when necessary.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/slave/flags.hpp 300db49100d989d6c0409766b1341cb956ea1631 
>   src/slave/flags.cpp dd7bc9a48dfd8481336a2d2ec0beecd19a342644 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 09742ff21513dc2570684d384b257868dd57a9ce 
> 
> Diff: https://reviews.apache.org/r/46003/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 46003: Removed the default value for agent work_dir.

[Greg Mann]

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

(Updated April 11, 2016, 9:06 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description (updated)
---

The default value for the agent's `--work_dir` flag was removed, the
type of the parameter was changed to `Option`, and code was
added to explicitly set the work directory when necessary.


Diffs
-

  src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
  src/slave/flags.hpp 300db49100d989d6c0409766b1341cb956ea1631 
  src/slave/flags.cpp dd7bc9a48dfd8481336a2d2ec0beecd19a342644 
  src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
  src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
  src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

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


Testing
---

`sudo make check`


Thanks,

Greg Mann

Re: Review Request 46003: Removed the default value for agent work_dir.

[Greg Mann]

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

(Updated April 11, 2016, 4:29 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

The default value for the agent's `--work_dir` flag was removed, the type of 
the parameter was changed to `Option`, and code was added to explicitly 
set the work directory when necessary.


Diffs (updated)
-

  src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
  src/slave/flags.hpp 300db49100d989d6c0409766b1341cb956ea1631 
  src/slave/flags.cpp dd7bc9a48dfd8481336a2d2ec0beecd19a342644 
  src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
  src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
  src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

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


Testing
---

`sudo make check`


Thanks,

Greg Mann