You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Guangya Liu <gy...@gmail.com> on 2016/04/02 17:54:59 UTC

Review Request 45326: Added agent flag dvdcli_dir for specify dvdcli path.

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

Review request for mesos, Gilbert Song and Jie Yu.


Repository: mesos


Description
-------

Added agent flag dvdcli_dir for specify dvdcli path.


Diffs
-----

  docs/configuration.md da42eaf7069a016fa7eaf929fc285e1fa1f144e9 
  src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
  src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp PRE-CREATION 
  src/slave/flags.hpp d0c606eea74e1a2e69067c43a267047e65a22a04 
  src/slave/flags.cpp 0551ec334c6747507bf7bb068d27d67f3fdd6c83 

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


Testing
-------

make
make check


Thanks,

Guangya Liu

Re: Review Request 45326: Added agent flag dvdcli_dir for specify dvdcli path.

Posted by Guangya Liu <gy...@gmail.com>.

> On 四月 13, 2016, 9:01 p.m., James DeFelice wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp, line 64
> > <https://reviews.apache.org/r/45326/diff/1/?file=1323269#file1323269line64>
> >
> >     should we verify that the dvdcli binary actually exists here (and if not generate an error)?

Yes, will fix this in https://reviews.apache.org/r/46180


- Guangya


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


On 四月 14, 2016, 4:57 a.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45326/
> -----------------------------------------------------------
> 
> (Updated 四月 14, 2016, 4:57 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-5084
>     https://issues.apache.org/jira/browse/MESOS-5084
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added agent flag dvdcli_dir for specify dvdcli path.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
> 
> Diff: https://reviews.apache.org/r/45326/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45326: Added agent flag dvdcli_dir for specify dvdcli path.

Posted by James DeFelice <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45326/#review128743
-----------------------------------------------------------




src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp (line 64)
<https://reviews.apache.org/r/45326/#comment192208>

    should we verify that the dvdcli binary actually exists here (and if not generate an error)?


- James DeFelice


On April 2, 2016, 3:54 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45326/
> -----------------------------------------------------------
> 
> (Updated April 2, 2016, 3:54 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added agent flag dvdcli_dir for specify dvdcli path.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md da42eaf7069a016fa7eaf929fc285e1fa1f144e9 
>   src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp PRE-CREATION 
>   src/slave/flags.hpp d0c606eea74e1a2e69067c43a267047e65a22a04 
>   src/slave/flags.cpp 0551ec334c6747507bf7bb068d27d67f3fdd6c83 
> 
> Diff: https://reviews.apache.org/r/45326/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45326: Added agent flag dvdcli_dir for specify dvdcli path.

Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45326/#review128941
-----------------------------------------------------------




src/slave/flags.cpp (line 803)
<https://reviews.apache.org/r/45326/#comment192393>

    Just leave a concern here:
    
    Do we always have dvdcli installed in `/usr/bin` for all os (e.g., centos)? any special case?


- Gilbert Song


On April 13, 2016, 11:06 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45326/
> -----------------------------------------------------------
> 
> (Updated April 13, 2016, 11:06 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-5084
>     https://issues.apache.org/jira/browse/MESOS-5084
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added agent flag dvdcli_dir for specify dvdcli path.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
> 
> Diff: https://reviews.apache.org/r/45326/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45326: Added agent flag dvdcli_dir for specify dvdcli path.

Posted by Guangya Liu <gy...@gmail.com>.

> On 四月 14, 2016, 10:55 p.m., Jie Yu wrote:
> > docs/configuration.md, line 1411
> > <https://reviews.apache.org/r/45326/diff/3/?file=1343835#file1343835line1411>
> >
> >     It's a directory or a file? Instead of doing that, can we just assume dvdcli is in 'PATH' and add a check in DockerVolumeDriverClient::create?

OK, will try to use `os::which` to handle this. There is a JIRA tracing it: https://issues.apache.org/jira/browse/MESOS-4576 , will try to upload a patch for it.


- Guangya


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


On 四月 14, 2016, 6:06 a.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45326/
> -----------------------------------------------------------
> 
> (Updated 四月 14, 2016, 6:06 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-5084
>     https://issues.apache.org/jira/browse/MESOS-5084
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added agent flag dvdcli_dir for specify dvdcli path.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
> 
> Diff: https://reviews.apache.org/r/45326/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45326: Added agent flag dvdcli_dir for specify dvdcli path.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45326/#review129022
-----------------------------------------------------------




docs/configuration.md (line 1411)
<https://reviews.apache.org/r/45326/#comment192487>

    It's a directory or a file? Instead of doing that, can we just assume dvdcli is in 'PATH' and add a check in DockerVolumeDriverClient::create?


- Jie Yu


On April 14, 2016, 6:06 a.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45326/
> -----------------------------------------------------------
> 
> (Updated April 14, 2016, 6:06 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-5084
>     https://issues.apache.org/jira/browse/MESOS-5084
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added agent flag dvdcli_dir for specify dvdcli path.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
> 
> Diff: https://reviews.apache.org/r/45326/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45326: Implemented os::which().

Posted by Jie Yu <yu...@gmail.com>.

> On April 19, 2016, 3:03 p.m., haosdent huang wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 975
> > <https://reviews.apache.org/r/45326/diff/6/?file=1350574#file1350574line975>
> >
> >     Do we need test the priority here? When a binary exists in different folders at the same time.
> 
> Guangya Liu wrote:
>     I think that as long as the binary can be found should be good enough, why need care the priority here?
> 
> haosdent huang wrote:
>     Let me describe it in another way. Suppose `PATH` is `/usr/bin:/bin`, and `bar` exists in both `/usr/bin/bar` and `/bin/bar`. Are we we always guarantee that take the first one in `os::which()`? Or add comment about the priority?
> 
> Guangya Liu wrote:
>     Yes, I was following linux system, always using the first. Does adding some comments here make sense?
>     
>     root@mesos002:~/src/mesos/m2/mesos# which ls
>     /usr/local/bin/ls
>     root@mesos002:~/src/mesos/m2/mesos# env | grep PATH
>     PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/usr/local/go/bin:/root/go-tools/bin
>     GOPATH=/root/go-tools
>     root@mesos002:~/src/mesos/m2/mesos# ls /usr/local/bin/ls
>     /usr/local/bin/ls
>     root@mesos002:~/src/mesos/m2/mesos# ls /bin/ls
>     /bin/ls
> 
> haosdent huang wrote:
>     Seems add a comment would be better.

I'll add a TODO. We need to test file without 'x' permission as well.


- Jie


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


On April 19, 2016, 2:56 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45326/
> -----------------------------------------------------------
> 
> (Updated April 19, 2016, 2:56 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-4576
>     https://issues.apache.org/jira/browse/MESOS-4576
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented os::which().
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp dfe4eab2d812ef807c7416ac205573d55383c4ee 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9bd34c7508cd813c5de18028956f6a740997c266 
> 
> Diff: https://reviews.apache.org/r/45326/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> [ RUN      ] OsTest.Which
> [       OK ] OsTest.Which (0 ms)
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45326: Implemented os::which().

Posted by Guangya Liu <gy...@gmail.com>.

> On 四月 19, 2016, 3:03 p.m., haosdent huang wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 975
> > <https://reviews.apache.org/r/45326/diff/6/?file=1350574#file1350574line975>
> >
> >     Do we need test the priority here? When a binary exists in different folders at the same time.
> 
> Guangya Liu wrote:
>     I think that as long as the binary can be found should be good enough, why need care the priority here?
> 
> haosdent huang wrote:
>     Let me describe it in another way. Suppose `PATH` is `/usr/bin:/bin`, and `bar` exists in both `/usr/bin/bar` and `/bin/bar`. Are we we always guarantee that take the first one in `os::which()`? Or add comment about the priority?

Yes, I was following linux system, always using the first. Does adding some comments here make sense?

root@mesos002:~/src/mesos/m2/mesos# which ls
/usr/local/bin/ls
root@mesos002:~/src/mesos/m2/mesos# env | grep PATH
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/usr/local/go/bin:/root/go-tools/bin
GOPATH=/root/go-tools
root@mesos002:~/src/mesos/m2/mesos# ls /usr/local/bin/ls
/usr/local/bin/ls
root@mesos002:~/src/mesos/m2/mesos# ls /bin/ls
/bin/ls


> On 四月 19, 2016, 3:03 p.m., haosdent huang wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 976
> > <https://reviews.apache.org/r/45326/diff/6/?file=1350574#file1350574line976>
> >
> >     Should `EXPECT_SOME` here?
> 
> Guangya Liu wrote:
>     Can you please show more detail for why use `EXPECT_SOME` here?
> 
> haosdent huang wrote:
>     `ASSERT` would abort the test while `EXPECT` would continue to run following code. I think we usually use `ASSERT` when something block the test. e.g. `ASSERT_SOME(master);`. And usually use `EXPECT` at those thing we want to test in the test cases. e.g. `EXPECT_CALL(...`.

Yes, but I was following this in test case, https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp#L964


- Guangya


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


On 四月 19, 2016, 2:56 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45326/
> -----------------------------------------------------------
> 
> (Updated 四月 19, 2016, 2:56 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-4576
>     https://issues.apache.org/jira/browse/MESOS-4576
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented os::which().
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp dfe4eab2d812ef807c7416ac205573d55383c4ee 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9bd34c7508cd813c5de18028956f6a740997c266 
> 
> Diff: https://reviews.apache.org/r/45326/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> [ RUN      ] OsTest.Which
> [       OK ] OsTest.Which (0 ms)
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45326: Implemented os::which().

Posted by Guangya Liu <gy...@gmail.com>.

> On 四月 19, 2016, 3:03 p.m., haosdent huang wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 976
> > <https://reviews.apache.org/r/45326/diff/6/?file=1350574#file1350574line976>
> >
> >     Should `EXPECT_SOME` here?

Can you please show more detail for why use `EXPECT_SOME` here?


> On 四月 19, 2016, 3:03 p.m., haosdent huang wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 975
> > <https://reviews.apache.org/r/45326/diff/6/?file=1350574#file1350574line975>
> >
> >     Do we need test the priority here? When a binary exists in different folders at the same time.

I think that as long as the binary can be found should be good enough, why need care the priority here?


- Guangya


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


On 四月 19, 2016, 2:56 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45326/
> -----------------------------------------------------------
> 
> (Updated 四月 19, 2016, 2:56 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-4576
>     https://issues.apache.org/jira/browse/MESOS-4576
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented os::which().
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp dfe4eab2d812ef807c7416ac205573d55383c4ee 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9bd34c7508cd813c5de18028956f6a740997c266 
> 
> Diff: https://reviews.apache.org/r/45326/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> [ RUN      ] OsTest.Which
> [       OK ] OsTest.Which (0 ms)
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45326: Implemented os::which().

Posted by haosdent huang <ha...@gmail.com>.

> On April 19, 2016, 3:03 p.m., haosdent huang wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 976
> > <https://reviews.apache.org/r/45326/diff/6/?file=1350574#file1350574line976>
> >
> >     Should `EXPECT_SOME` here?
> 
> Guangya Liu wrote:
>     Can you please show more detail for why use `EXPECT_SOME` here?

`ASSERT` would abort the test while `EXPECT` would continue to run following code. I think we usually use `ASSERT` when something block the test. e.g. `ASSERT_SOME(master);`. And usually use `EXPECT` at those thing we want to test in the test cases. e.g. `EXPECT_CALL(...`.


> On April 19, 2016, 3:03 p.m., haosdent huang wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 975
> > <https://reviews.apache.org/r/45326/diff/6/?file=1350574#file1350574line975>
> >
> >     Do we need test the priority here? When a binary exists in different folders at the same time.
> 
> Guangya Liu wrote:
>     I think that as long as the binary can be found should be good enough, why need care the priority here?

Let me describe it in another way. Suppose `PATH` is `/usr/bin:/bin`, and `bar` exists in both `/usr/bin/bar` and `/bin/bar`. Are we we always guarantee that take the first one in `os::which()`? Or add comment about the priority?


- haosdent


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


On April 19, 2016, 2:56 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45326/
> -----------------------------------------------------------
> 
> (Updated April 19, 2016, 2:56 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-4576
>     https://issues.apache.org/jira/browse/MESOS-4576
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented os::which().
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp dfe4eab2d812ef807c7416ac205573d55383c4ee 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9bd34c7508cd813c5de18028956f6a740997c266 
> 
> Diff: https://reviews.apache.org/r/45326/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> [ RUN      ] OsTest.Which
> [       OK ] OsTest.Which (0 ms)
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45326: Implemented os::which().

Posted by haosdent huang <ha...@gmail.com>.

> On April 19, 2016, 3:03 p.m., haosdent huang wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 976
> > <https://reviews.apache.org/r/45326/diff/6/?file=1350574#file1350574line976>
> >
> >     Should `EXPECT_SOME` here?
> 
> Guangya Liu wrote:
>     Can you please show more detail for why use `EXPECT_SOME` here?
> 
> haosdent huang wrote:
>     `ASSERT` would abort the test while `EXPECT` would continue to run following code. I think we usually use `ASSERT` when something block the test. e.g. `ASSERT_SOME(master);`. And usually use `EXPECT` at those thing we want to test in the test cases. e.g. `EXPECT_CALL(...`.
> 
> Guangya Liu wrote:
>     Yes, but I was following this in test case, https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp#L964

Got it, let me drop this.


> On April 19, 2016, 3:03 p.m., haosdent huang wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 975
> > <https://reviews.apache.org/r/45326/diff/6/?file=1350574#file1350574line975>
> >
> >     Do we need test the priority here? When a binary exists in different folders at the same time.
> 
> Guangya Liu wrote:
>     I think that as long as the binary can be found should be good enough, why need care the priority here?
> 
> haosdent huang wrote:
>     Let me describe it in another way. Suppose `PATH` is `/usr/bin:/bin`, and `bar` exists in both `/usr/bin/bar` and `/bin/bar`. Are we we always guarantee that take the first one in `os::which()`? Or add comment about the priority?
> 
> Guangya Liu wrote:
>     Yes, I was following linux system, always using the first. Does adding some comments here make sense?
>     
>     root@mesos002:~/src/mesos/m2/mesos# which ls
>     /usr/local/bin/ls
>     root@mesos002:~/src/mesos/m2/mesos# env | grep PATH
>     PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/usr/local/go/bin:/root/go-tools/bin
>     GOPATH=/root/go-tools
>     root@mesos002:~/src/mesos/m2/mesos# ls /usr/local/bin/ls
>     /usr/local/bin/ls
>     root@mesos002:~/src/mesos/m2/mesos# ls /bin/ls
>     /bin/ls

Seems add a comment would be better.


- haosdent


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


On April 19, 2016, 2:56 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45326/
> -----------------------------------------------------------
> 
> (Updated April 19, 2016, 2:56 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-4576
>     https://issues.apache.org/jira/browse/MESOS-4576
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented os::which().
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp dfe4eab2d812ef807c7416ac205573d55383c4ee 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9bd34c7508cd813c5de18028956f6a740997c266 
> 
> Diff: https://reviews.apache.org/r/45326/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> [ RUN      ] OsTest.Which
> [       OK ] OsTest.Which (0 ms)
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45326: Implemented os::which().

Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45326/#review129540
-----------------------------------------------------------


Fix it, then Ship it!




Ship It!


3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp (line 467)
<https://reviews.apache.org/r/45326/#comment192959>

    Should we return error here? I mean use Result<std::string>.



3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp (line 975)
<https://reviews.apache.org/r/45326/#comment192962>

    Do we need test the priority here? When a binary exists in different folders at the same time.



3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp (line 976)
<https://reviews.apache.org/r/45326/#comment192960>

    Should `EXPECT_SOME` here?


- haosdent huang


On April 19, 2016, 2:56 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45326/
> -----------------------------------------------------------
> 
> (Updated April 19, 2016, 2:56 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-4576
>     https://issues.apache.org/jira/browse/MESOS-4576
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented os::which().
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp dfe4eab2d812ef807c7416ac205573d55383c4ee 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9bd34c7508cd813c5de18028956f6a740997c266 
> 
> Diff: https://reviews.apache.org/r/45326/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> [ RUN      ] OsTest.Which
> [       OK ] OsTest.Which (0 ms)
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45326: Implemented os::which().

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45326/#review129567
-----------------------------------------------------------


Ship it!




Ship It!

- Jie Yu


On April 19, 2016, 2:56 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45326/
> -----------------------------------------------------------
> 
> (Updated April 19, 2016, 2:56 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-4576
>     https://issues.apache.org/jira/browse/MESOS-4576
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented os::which().
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp dfe4eab2d812ef807c7416ac205573d55383c4ee 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9bd34c7508cd813c5de18028956f6a740997c266 
> 
> Diff: https://reviews.apache.org/r/45326/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> [ RUN      ] OsTest.Which
> [       OK ] OsTest.Which (0 ms)
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45326: Implemented os::which().

Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45326/
-----------------------------------------------------------

(Updated 四月 19, 2016, 2:56 p.m.)


Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.


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


Repository: mesos


Description
-------

Implemented os::which().


Diffs (updated)
-----

  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp dfe4eab2d812ef807c7416ac205573d55383c4ee 
  3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9bd34c7508cd813c5de18028956f6a740997c266 

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


Testing
-------

make
make check

[ RUN      ] OsTest.Which
[       OK ] OsTest.Which (0 ms)


Thanks,

Guangya Liu

Re: Review Request 45326: Implemented os::which().

Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45326/
-----------------------------------------------------------

(Updated 四月 16, 2016, 3:16 a.m.)


Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.


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


Repository: mesos


Description
-------

Implemented os::which().


Diffs (updated)
-----

  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 52978f37a27c6db45b71fa1a1d41bb833a76e666 
  3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9bd34c7508cd813c5de18028956f6a740997c266 

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


Testing
-------

make
make check

[ RUN      ] OsTest.Which
[       OK ] OsTest.Which (0 ms)


Thanks,

Guangya Liu

Re: Review Request 45326: Implemented os::which().

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45326/#review129156
-----------------------------------------------------------




3rdparty/libprocess/3rdparty/stout/include/stout/os/which.hpp (line 13)
<https://reviews.apache.org/r/45326/#comment192628>

    Instead of putting it in a new file, can you put it in stout/posix/os.hpp for now since `getenv` is only in stout/posix/os.hpp for now.



3rdparty/libprocess/3rdparty/stout/include/stout/os/which.hpp (line 30)
<https://reviews.apache.org/r/45326/#comment192621>

    Looks like we ignore errors. Should the return value be 'Option'?



3rdparty/libprocess/3rdparty/stout/include/stout/os/which.hpp (line 34)
<https://reviews.apache.org/r/45326/#comment192620>

    Can you do the following to avoid unnecessary indentation?
    ```
    Option<string> path = getenv("PATH");
    if (path.isNone()) {
      return None();
    }
    
    vector<string> tokens = strings::tokenize(path.get(), ":");
    ...
    ```


- Jie Yu


On April 15, 2016, 3:07 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45326/
> -----------------------------------------------------------
> 
> (Updated April 15, 2016, 3:07 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-4576
>     https://issues.apache.org/jira/browse/MESOS-4576
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented os::which().
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 33ddb06e25920096f2d16d4f372129ee2f6a8721 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp a81086860a17bd1912e55051dd7bb98a20772b51 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/which.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9bd34c7508cd813c5de18028956f6a740997c266 
> 
> Diff: https://reviews.apache.org/r/45326/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> [ RUN      ] OsTest.Which
> [       OK ] OsTest.Which (0 ms)
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45326: Implemented os::which().

Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45326/
-----------------------------------------------------------

(Updated 四月 15, 2016, 3:07 p.m.)


Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.


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


Repository: mesos


Description
-------

Implemented os::which().


Diffs
-----

  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 33ddb06e25920096f2d16d4f372129ee2f6a8721 
  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp a81086860a17bd1912e55051dd7bb98a20772b51 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/which.hpp PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9bd34c7508cd813c5de18028956f6a740997c266 

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


Testing
-------

make
make check

[ RUN      ] OsTest.Which
[       OK ] OsTest.Which (0 ms)


Thanks,

Guangya Liu

Re: Review Request 45326: Implemented os::which().

Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45326/
-----------------------------------------------------------

(Updated 四月 15, 2016, 3:06 p.m.)


Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.


Summary (updated)
-----------------

Implemented os::which().


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


Repository: mesos


Description (updated)
-------

Implemented os::which().


Diffs (updated)
-----

  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 33ddb06e25920096f2d16d4f372129ee2f6a8721 
  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp a81086860a17bd1912e55051dd7bb98a20772b51 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/which.hpp PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9bd34c7508cd813c5de18028956f6a740997c266 

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


Testing (updated)
-------

make
make check

[ RUN      ] OsTest.Which
[       OK ] OsTest.Which (0 ms)


Thanks,

Guangya Liu

Re: Review Request 45326: Added agent flag dvdcli_dir for specify dvdcli path.

Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45326/
-----------------------------------------------------------

(Updated 四月 14, 2016, 6:06 a.m.)


Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.


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


Repository: mesos


Description
-------

Added agent flag dvdcli_dir for specify dvdcli path.


Diffs (updated)
-----

  docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
  src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
  src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 

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


Testing
-------

make
make check


Thanks,

Guangya Liu

Re: Review Request 45326: Added agent flag dvdcli_dir for specify dvdcli path.

Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45326/
-----------------------------------------------------------

(Updated 四月 14, 2016, 4:57 a.m.)


Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.


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


Repository: mesos


Description
-------

Added agent flag dvdcli_dir for specify dvdcli path.


Diffs (updated)
-----

  docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
  src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
  src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 

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


Testing
-------

make
make check


Thanks,

Guangya Liu