You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2018/08/16 23:51:44 UTC
Review Request 68398: Added `fs::used` helper API to stout.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68398/
-----------------------------------------------------------
Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
Bugs: MESOS-5158
https://issues.apache.org/jira/browse/MESOS-5158
Repository: mesos
Description
-------
Added a `fs::used` API that returns the amount of used space on a
given filesystem.
Diffs
-----
3rdparty/stout/include/stout/posix/fs.hpp 269a4f50f1df8d68be9e11030f885cf2c254c9d8
3rdparty/stout/include/stout/windows/fs.hpp cc9a56012bd17cf962c8bc6d59ae221fcf19ecba
3rdparty/stout/tests/os_tests.cpp b80c34e8e84cab68de7c843c7eafefbd84c3328c
Diff: https://reviews.apache.org/r/68398/diff/1/
Testing
-------
sudo make check (Fedora 28)
Thanks,
James Peach
Re: Review Request 68398: Added `fs::used` helper API to stout.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68398/#review207786
-----------------------------------------------------------
3rdparty/stout/include/stout/posix/fs.hpp
Lines 49 (patched)
<https://reviews.apache.org/r/68398/#comment291285>
Nit: A space after the minus sign.
Also the subexpression would result in an `unsigned long`, which is just "at least 32 bits" according to the C++ standard:
https://en.cppreference.com/w/cpp/language/types#Properties
It's probably fine but I was wondering if we should do the following instead:
```
(buf.f_blocks - buf.f_bfree) * static_cast<size_t>(buf.f_frsize)
```
3rdparty/stout/include/stout/windows/fs.hpp
Lines 55 (patched)
<https://reviews.apache.org/r/68398/#comment291286>
If the code in this chain is unix-only, should we just not introduce this function for now?
3rdparty/stout/tests/os_tests.cpp
Lines 1154 (patched)
<https://reviews.apache.org/r/68398/#comment291287>
If we don't introduce the `used` function, we could guard the whole test with `#ifndef __WINDOWS__`.
- Chun-Hung Hsiao
On Aug. 16, 2018, 11:51 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68398/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 11:51 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-5158
> https://issues.apache.org/jira/browse/MESOS-5158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a `fs::used` API that returns the amount of used space on a
> given filesystem.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/fs.hpp 269a4f50f1df8d68be9e11030f885cf2c254c9d8
> 3rdparty/stout/include/stout/windows/fs.hpp cc9a56012bd17cf962c8bc6d59ae221fcf19ecba
> 3rdparty/stout/tests/os_tests.cpp b80c34e8e84cab68de7c843c7eafefbd84c3328c
>
>
> Diff: https://reviews.apache.org/r/68398/diff/1/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 68398: Added `fs::used` helper API to stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68398/#review207631
-----------------------------------------------------------
3rdparty/stout/include/stout/windows/fs.hpp
Lines 57 (patched)
<https://reviews.apache.org/r/68398/#comment291076>
`UNIMPLEMENTED()`. Should cause a compiler warning if used in a code-path on Windows.
- Andrew Schwartzmeyer
On Aug. 16, 2018, 4:51 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68398/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 4:51 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-5158
> https://issues.apache.org/jira/browse/MESOS-5158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a `fs::used` API that returns the amount of used space on a
> given filesystem.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/fs.hpp 269a4f50f1df8d68be9e11030f885cf2c254c9d8
> 3rdparty/stout/include/stout/windows/fs.hpp cc9a56012bd17cf962c8bc6d59ae221fcf19ecba
> 3rdparty/stout/tests/os_tests.cpp b80c34e8e84cab68de7c843c7eafefbd84c3328c
>
>
> Diff: https://reviews.apache.org/r/68398/diff/1/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 68398: Added `fs::used` helper API to stout.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68398/
-----------------------------------------------------------
(Updated Sept. 5, 2018, 9:58 p.m.)
Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
Bugs: MESOS-5158
https://issues.apache.org/jira/browse/MESOS-5158
Repository: mesos
Description (updated)
-------
Added a `fs::used` API that returns the amount of used space
on a given filesystem.
Diffs (updated)
-----
3rdparty/stout/include/stout/posix/fs.hpp 269a4f50f1df8d68be9e11030f885cf2c254c9d8
3rdparty/stout/tests/os/filesystem_tests.cpp 880b551b65eceff9492dd414157694b986608115
Diff: https://reviews.apache.org/r/68398/diff/3/
Changes: https://reviews.apache.org/r/68398/diff/2-3/
Testing
-------
sudo make check (Fedora 28)
Thanks,
James Peach
Re: Review Request 68398: Added `fs::used` helper API to stout.
Posted by Ilya Pronin <ip...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68398/#review208007
-----------------------------------------------------------
Fix it, then Ship it!
LGTM! Only 1 minor comment about the new test.
3rdparty/stout/tests/os_tests.cpp
Lines 1157-1164 (patched)
<https://reviews.apache.org/r/68398/#comment291726>
What if current directory is a different partition than `/`?
- Ilya Pronin
On Aug. 24, 2018, 4:24 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68398/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2018, 4:24 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-5158
> https://issues.apache.org/jira/browse/MESOS-5158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a `fs::used` API that returns the amount of used space on a
> given filesystem.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/fs.hpp 269a4f50f1df8d68be9e11030f885cf2c254c9d8
> 3rdparty/stout/tests/os_tests.cpp b80c34e8e84cab68de7c843c7eafefbd84c3328c
>
>
> Diff: https://reviews.apache.org/r/68398/diff/2/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 68398: Added `fs::used` helper API to stout.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68398/#review208004
-----------------------------------------------------------
Ship it!
Ship It!
- Chun-Hung Hsiao
On Aug. 24, 2018, 11:24 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68398/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2018, 11:24 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-5158
> https://issues.apache.org/jira/browse/MESOS-5158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a `fs::used` API that returns the amount of used space on a
> given filesystem.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/fs.hpp 269a4f50f1df8d68be9e11030f885cf2c254c9d8
> 3rdparty/stout/tests/os_tests.cpp b80c34e8e84cab68de7c843c7eafefbd84c3328c
>
>
> Diff: https://reviews.apache.org/r/68398/diff/2/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 68398: Added `fs::used` helper API to stout.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68398/#review208012
-----------------------------------------------------------
3rdparty/stout/tests/os_tests.cpp
Lines 1154-1172 (patched)
<https://reviews.apache.org/r/68398/#comment291728>
It's probably more appropriate to put this in `3rdparty/stout/tests/os/filesystem_tests.cpp`:
```
#ifndef __WINDOWS__
TEST_F(FsTest, Used)
{
...
}
#endif // __WINDOWS__
```
- Chun-Hung Hsiao
On Aug. 24, 2018, 11:24 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68398/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2018, 11:24 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-5158
> https://issues.apache.org/jira/browse/MESOS-5158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a `fs::used` API that returns the amount of used space on a
> given filesystem.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/fs.hpp 269a4f50f1df8d68be9e11030f885cf2c254c9d8
> 3rdparty/stout/tests/os_tests.cpp b80c34e8e84cab68de7c843c7eafefbd84c3328c
>
>
> Diff: https://reviews.apache.org/r/68398/diff/2/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 68398: Added `fs::used` helper API to stout.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68398/
-----------------------------------------------------------
(Updated Aug. 24, 2018, 11:24 p.m.)
Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
Bugs: MESOS-5158
https://issues.apache.org/jira/browse/MESOS-5158
Repository: mesos
Description
-------
Added a `fs::used` API that returns the amount of used space on a
given filesystem.
Diffs (updated)
-----
3rdparty/stout/include/stout/posix/fs.hpp 269a4f50f1df8d68be9e11030f885cf2c254c9d8
3rdparty/stout/tests/os_tests.cpp b80c34e8e84cab68de7c843c7eafefbd84c3328c
Diff: https://reviews.apache.org/r/68398/diff/2/
Changes: https://reviews.apache.org/r/68398/diff/1-2/
Testing
-------
sudo make check (Fedora 28)
Thanks,
James Peach