Re: Review Request 68991: Added a test `FsTest.Lsof`.

2018-11-07 Thread Gilbert Song 

---
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`.

2018-10-19 Thread James Peach 

---
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`.

2018-10-19 Thread Qian Zhang 

---
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`.

2018-10-17 Thread James Peach 

---
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)


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`.

2018-10-17 Thread Qian Zhang 

---
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`.

2018-10-17 Thread Qian Zhang 


> On Oct. 13, 2018, 1:40 a.m., James Peach wrote:
> > 3rdparty/stout/tests/os/filesystem_tests.cpp
> > Lines 830 (patched)
> > 
> >
> > 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`.

2018-10-16 Thread James Peach 


> On Oct. 12, 2018, 5:40 p.m., James Peach wrote:
> > 3rdparty/stout/tests/os/filesystem_tests.cpp
> > Lines 830 (patched)
> > 
> >
> > 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`.

2018-10-14 Thread Qian Zhang 

---
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

Re: Review Request 68991: Added a test `FsTest.Lsof`.

2018-10-14 Thread Qian Zhang 


> On Oct. 13, 2018, 1:40 a.m., James Peach wrote:
> > 3rdparty/stout/tests/os/filesystem_tests.cpp
> > Lines 830 (patched)
> > 
> >
> > 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`.

2018-10-12 Thread James Peach 

---
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)


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)


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
> 
>