You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@gmail.com> on 2018/10/11 13:56:43 UTC
Review Request 68991: Added a test `FsTest.Lsof`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68991/
-----------------------------------------------------------
Review request for mesos, Gilbert Song and James Peach.
Bugs: MESOS-9152
https://issues.apache.org/jira/browse/MESOS-9152
Repository: mesos
Description
-------
Added a test `FsTest.Lsof`.
Diffs
-----
3rdparty/stout/tests/os/filesystem_tests.cpp 29f06f3dd126007d6b3a81f31795270f0654b847
Diff: https://reviews.apache.org/r/68991/diff/1/
Testing
-------
sudo make check
Thanks,
Qian Zhang
Re: Review Request 68991: Added a test `FsTest.Lsof`.
Posted by Qian Zhang <zh...@gmail.com>.
> On Oct. 13, 2018, 1:40 a.m., James Peach wrote:
> > 3rdparty/stout/tests/os/filesystem_tests.cpp
> > Lines 830 (patched)
> > <https://reviews.apache.org/r/68991/diff/1/?file=2096374#file2096374line830>
> >
> > Maybe also add:
> > ```
> > EXPECT_FALSE(std::find(fds->begin(), fds->end(), -1) != fds->end())
> > ```
>
> Qian Zhang wrote:
> Can you please elaborate a bit why we need to add this?
>
> James Peach wrote:
> I was just thinking about a negative test to ensure that we don't have anything in the result set that shouldn't be there. Maybe an alternative approach would be to loop over the returned descriptors and make a system call, e.g. fcntl(F_GETFL), to verify that every entry is an open descriptor.
>
> If you don't think adding more checks helps the test, I'm OK with dropping this issue :)
> Maybe an alternative approach would be to loop over the returned descriptors and make a system call, e.g. fcntl(F_GETFL), to verify that every entry is an open descriptor.
This is a good idea! Let me update the patch accordingly.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68991/#review209492
-----------------------------------------------------------
On Oct. 14, 2018, 9:35 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68991/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2018, 9:35 p.m.)
>
>
> Review request for mesos, Gilbert Song and James Peach.
>
>
> Bugs: MESOS-9152
> https://issues.apache.org/jira/browse/MESOS-9152
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test `FsTest.Lsof`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/tests/os/filesystem_tests.cpp 29f06f3dd126007d6b3a81f31795270f0654b847
>
>
> Diff: https://reviews.apache.org/r/68991/diff/2/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 68991: Added a test `FsTest.Lsof`.
Posted by Qian Zhang <zh...@gmail.com>.
> On Oct. 13, 2018, 1:40 a.m., James Peach wrote:
> > 3rdparty/stout/tests/os/filesystem_tests.cpp
> > Lines 830 (patched)
> > <https://reviews.apache.org/r/68991/diff/1/?file=2096374#file2096374line830>
> >
> > Maybe also add:
> > ```
> > EXPECT_FALSE(std::find(fds->begin(), fds->end(), -1) != fds->end())
> > ```
Can you please elaborate a bit why we need to add this?
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68991/#review209492
-----------------------------------------------------------
On Oct. 11, 2018, 9:56 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68991/
> -----------------------------------------------------------
>
> (Updated Oct. 11, 2018, 9:56 p.m.)
>
>
> Review request for mesos, Gilbert Song and James Peach.
>
>
> Bugs: MESOS-9152
> https://issues.apache.org/jira/browse/MESOS-9152
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test `FsTest.Lsof`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/tests/os/filesystem_tests.cpp 29f06f3dd126007d6b3a81f31795270f0654b847
>
>
> Diff: https://reviews.apache.org/r/68991/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 68991: Added a test `FsTest.Lsof`.
Posted by James Peach <jp...@apache.org>.
> On Oct. 12, 2018, 5:40 p.m., James Peach wrote:
> > 3rdparty/stout/tests/os/filesystem_tests.cpp
> > Lines 830 (patched)
> > <https://reviews.apache.org/r/68991/diff/1/?file=2096374#file2096374line830>
> >
> > Maybe also add:
> > ```
> > EXPECT_FALSE(std::find(fds->begin(), fds->end(), -1) != fds->end())
> > ```
>
> Qian Zhang wrote:
> Can you please elaborate a bit why we need to add this?
I was just thinking about a negative test to ensure that we don't have anything in the result set that shouldn't be there. Maybe an alternative approach would be to loop over the returned descriptors and make a system call, e.g. fcntl(F_GETFL), to verify that every entry is an open descriptor.
If you don't think adding more checks helps the test, I'm OK with dropping this issue :)
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68991/#review209492
-----------------------------------------------------------
On Oct. 14, 2018, 1:35 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68991/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2018, 1:35 p.m.)
>
>
> Review request for mesos, Gilbert Song and James Peach.
>
>
> Bugs: MESOS-9152
> https://issues.apache.org/jira/browse/MESOS-9152
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test `FsTest.Lsof`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/tests/os/filesystem_tests.cpp 29f06f3dd126007d6b3a81f31795270f0654b847
>
>
> Diff: https://reviews.apache.org/r/68991/diff/2/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 68991: Added a test `FsTest.Lsof`.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68991/#review209492
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/stout/tests/os/filesystem_tests.cpp
Lines 828 (patched)
<https://reviews.apache.org/r/68991/#comment293962>
These can all be `EXPECT_TRUE`. If it compiles, could you use `EXPECT_NE`?
```
EXPECT_NE(std::find(fds->begin(), fds->end(), 0), fds->end());
```
3rdparty/stout/tests/os/filesystem_tests.cpp
Lines 830 (patched)
<https://reviews.apache.org/r/68991/#comment293961>
Maybe also add:
```
EXPECT_FALSE(std::find(fds->begin(), fds->end(), -1) != fds->end())
```
- James Peach
On Oct. 11, 2018, 1:56 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68991/
> -----------------------------------------------------------
>
> (Updated Oct. 11, 2018, 1:56 p.m.)
>
>
> Review request for mesos, Gilbert Song and James Peach.
>
>
> Bugs: MESOS-9152
> https://issues.apache.org/jira/browse/MESOS-9152
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test `FsTest.Lsof`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/tests/os/filesystem_tests.cpp 29f06f3dd126007d6b3a81f31795270f0654b847
>
>
> Diff: https://reviews.apache.org/r/68991/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 68991: Added a test `FsTest.Lsof`.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68991/#review209700
-----------------------------------------------------------
3rdparty/stout/tests/os/filesystem_tests.cpp
Lines 833 (patched)
<https://reviews.apache.org/r/68991/#comment294247>
I missed that semantic in the original review. I don't think that `lsof` should return its own fs, since that is guaranteed to either be stale or re-used by the time the caller gets it.
- James Peach
On Oct. 17, 2018, 6:41 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68991/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2018, 6:41 a.m.)
>
>
> Review request for mesos, Gilbert Song and James Peach.
>
>
> Bugs: MESOS-9152
> https://issues.apache.org/jira/browse/MESOS-9152
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test `FsTest.Lsof`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/tests/os/filesystem_tests.cpp 29f06f3dd126007d6b3a81f31795270f0654b847
>
>
> Diff: https://reviews.apache.org/r/68991/diff/3/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 68991: Added a test `FsTest.Lsof`.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68991/#review210406
-----------------------------------------------------------
Ship it!
Ship It!
- Gilbert Song
On Oct. 18, 2018, 11:58 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68991/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2018, 11:58 p.m.)
>
>
> Review request for mesos, Gilbert Song and James Peach.
>
>
> Bugs: MESOS-9152
> https://issues.apache.org/jira/browse/MESOS-9152
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test `FsTest.Lsof`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/tests/os/filesystem_tests.cpp 29f06f3dd126007d6b3a81f31795270f0654b847
>
>
> Diff: https://reviews.apache.org/r/68991/diff/4/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 68991: Added a test `FsTest.Lsof`.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68991/#review209787
-----------------------------------------------------------
Ship it!
Ship It!
- James Peach
On Oct. 19, 2018, 6:58 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68991/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2018, 6:58 a.m.)
>
>
> Review request for mesos, Gilbert Song and James Peach.
>
>
> Bugs: MESOS-9152
> https://issues.apache.org/jira/browse/MESOS-9152
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test `FsTest.Lsof`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/tests/os/filesystem_tests.cpp 29f06f3dd126007d6b3a81f31795270f0654b847
>
>
> Diff: https://reviews.apache.org/r/68991/diff/4/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 68991: Added a test `FsTest.Lsof`.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68991/
-----------------------------------------------------------
(Updated Oct. 19, 2018, 2:58 p.m.)
Review request for mesos, Gilbert Song and James Peach.
Changes
-------
Addressed review comments.
Bugs: MESOS-9152
https://issues.apache.org/jira/browse/MESOS-9152
Repository: mesos
Description
-------
Added a test `FsTest.Lsof`.
Diffs (updated)
-----
3rdparty/stout/tests/os/filesystem_tests.cpp 29f06f3dd126007d6b3a81f31795270f0654b847
Diff: https://reviews.apache.org/r/68991/diff/4/
Changes: https://reviews.apache.org/r/68991/diff/3-4/
Testing
-------
sudo make check
Thanks,
Qian Zhang
Re: Review Request 68991: Added a test `FsTest.Lsof`.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68991/
-----------------------------------------------------------
(Updated Oct. 17, 2018, 2:41 p.m.)
Review request for mesos, Gilbert Song and James Peach.
Changes
-------
Addressed review comment.
Bugs: MESOS-9152
https://issues.apache.org/jira/browse/MESOS-9152
Repository: mesos
Description
-------
Added a test `FsTest.Lsof`.
Diffs (updated)
-----
3rdparty/stout/tests/os/filesystem_tests.cpp 29f06f3dd126007d6b3a81f31795270f0654b847
Diff: https://reviews.apache.org/r/68991/diff/3/
Changes: https://reviews.apache.org/r/68991/diff/2-3/
Testing
-------
sudo make check
Thanks,
Qian Zhang
Re: Review Request 68991: Added a test `FsTest.Lsof`.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68991/
-----------------------------------------------------------
(Updated Oct. 14, 2018, 9:35 p.m.)
Review request for mesos, Gilbert Song and James Peach.
Changes
-------
Addressed review comments
Bugs: MESOS-9152
https://issues.apache.org/jira/browse/MESOS-9152
Repository: mesos
Description
-------
Added a test `FsTest.Lsof`.
Diffs (updated)
-----
3rdparty/stout/tests/os/filesystem_tests.cpp 29f06f3dd126007d6b3a81f31795270f0654b847
Diff: https://reviews.apache.org/r/68991/diff/2/
Changes: https://reviews.apache.org/r/68991/diff/1-2/
Testing
-------
sudo make check
Thanks,
Qian Zhang