You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jiang Yan Xu <ya...@twitter.com> on 2013/04/02 01:47:38 UTC
Review Request: Added os::sleep which takes a Duration object.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10231/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Description
-------
This is part 1.
Part 2 would be to replace sleep() call in Mesos to use os::sleep().
This addresses bug MESOS-415.
https://issues.apache.org/jira/browse/MESOS-415
Diffs
-----
third_party/libprocess/third_party/stout/include/stout/os.hpp 5c217836a4a60402945544cba862b4c936ff0f83
third_party/libprocess/third_party/stout/tests/os_tests.cpp d543b6d585097fbd6ee455d4807f23eb3b532a23
Diff: https://reviews.apache.org/r/10231/diff/
Testing
-------
Ran make check.
Thanks,
Jiang Yan Xu
Re: Review Request: Added os::sleep which takes a Duration object.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10231/#review18665
-----------------------------------------------------------
third_party/libprocess/third_party/stout/include/stout/os.hpp
<https://reviews.apache.org/r/10231/#comment39043>
We don't use periods at the end of log lines.
third_party/libprocess/third_party/stout/include/stout/os.hpp
<https://reviews.apache.org/r/10231/#comment39044>
We wrap comments at 70 column width and code at 80!
third_party/libprocess/third_party/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/10231/#comment39045>
Wrap this in ASSERT_SOME.
third_party/libprocess/third_party/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/10231/#comment39046>
ditto
third_party/libprocess/third_party/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/10231/#comment39047>
ditto.
- Vinod Kone
On April 1, 2013, 11:47 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10231/
> -----------------------------------------------------------
>
> (Updated April 1, 2013, 11:47 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> This is part 1.
> Part 2 would be to replace sleep() call in Mesos to use os::sleep().
>
>
> This addresses bug MESOS-415.
> https://issues.apache.org/jira/browse/MESOS-415
>
>
> Diffs
> -----
>
> third_party/libprocess/third_party/stout/include/stout/os.hpp 5c217836a4a60402945544cba862b4c936ff0f83
> third_party/libprocess/third_party/stout/tests/os_tests.cpp d543b6d585097fbd6ee455d4807f23eb3b532a23
>
> Diff: https://reviews.apache.org/r/10231/diff/
>
>
> Testing
> -------
>
> Ran make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Added os::sleep which takes a Duration object.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On April 5, 2013, 7:35 p.m., Ben Mahler wrote:
> > third_party/libprocess/third_party/stout/tests/os_tests.cpp, line 199
> > <https://reviews.apache.org/r/10231/diff/2/?file=277766#file277766line199>
> >
> > Have you run these tests with a high number of iterations?
Yes.
./mesos/build/third_party/libprocess/third_party/stout-tests --gtest_filter="OsTest.sleep" --gtest_repeat=500
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10231/#review18726
-----------------------------------------------------------
On April 9, 2013, 7:28 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10231/
> -----------------------------------------------------------
>
> (Updated April 9, 2013, 7:28 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> This is part 1.
> Part 2 would be to replace sleep() call in Mesos to use os::sleep().
>
>
> This addresses bug MESOS-415.
> https://issues.apache.org/jira/browse/MESOS-415
>
>
> Diffs
> -----
>
> third_party/libprocess/configure.ac cb7f0d73c6ddc1e75f7a3120858362c56a003aa7
> third_party/libprocess/third_party/stout/include/stout/os.hpp 5c217836a4a60402945544cba862b4c936ff0f83
> third_party/libprocess/third_party/stout/tests/os_tests.cpp d543b6d585097fbd6ee455d4807f23eb3b532a23
>
> Diff: https://reviews.apache.org/r/10231/diff/
>
>
> Testing
> -------
>
> Ran make check and
> ./mesos/build/third_party/libprocess/third_party/stout-tests --gtest_filter="OsTest.sleep" --gtest_repeat=500
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Added os::sleep which takes a Duration object.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10231/#review18726
-----------------------------------------------------------
third_party/libprocess/third_party/stout/include/stout/os.hpp
<https://reviews.apache.org/r/10231/#comment39142>
Why do you need to check this?
"EINVAL
The value in the tv_nsec field was not in the range 0 to 999999999 or tv_sec was negative."
In which case, users asking to sleep with negative values will get the ErrnoError() from below anyway.
third_party/libprocess/third_party/stout/include/stout/os.hpp
<https://reviews.apache.org/r/10231/#comment39145>
s/requested/remaining
I think that reads better in the code here, no?
third_party/libprocess/third_party/stout/include/stout/os.hpp
<https://reviews.apache.org/r/10231/#comment39143>
As a note, Duration currently uses a double for storage. I'd like Duration to instead be storing an integral number of nanoseconds (the base unit).
Similarly to Bytes, it would be good for Duration to return integral values instead of doubles. That would eliminate your casts here and simplify the math.
So for now this looks good.
third_party/libprocess/third_party/stout/include/stout/os.hpp
<https://reviews.apache.org/r/10231/#comment39144>
Note Vinod's TODO in Duration, to implement the arithmetic operators, those might help you here as well.
Just as a note, no need to fix this.
third_party/libprocess/third_party/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/10231/#comment39146>
Have you run these tests with a high number of iterations?
third_party/libprocess/third_party/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/10231/#comment39147>
I think you're assuming too much granularity in these tests. I don't think you can be guaranteed sub-millisecond precision, see:
http://www.drdobbs.com/soft-real-time-programming-with-linux/184402031
- Ben Mahler
On April 3, 2013, 11:17 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10231/
> -----------------------------------------------------------
>
> (Updated April 3, 2013, 11:17 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> This is part 1.
> Part 2 would be to replace sleep() call in Mesos to use os::sleep().
>
>
> This addresses bug MESOS-415.
> https://issues.apache.org/jira/browse/MESOS-415
>
>
> Diffs
> -----
>
> third_party/libprocess/third_party/stout/include/stout/os.hpp 5c217836a4a60402945544cba862b4c936ff0f83
> third_party/libprocess/third_party/stout/tests/os_tests.cpp d543b6d585097fbd6ee455d4807f23eb3b532a23
>
> Diff: https://reviews.apache.org/r/10231/diff/
>
>
> Testing
> -------
>
> Ran make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Added os::sleep which takes a Duration object.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10231/#review18899
-----------------------------------------------------------
Ship it!
third_party/libprocess/configure.ac
<https://reviews.apache.org/r/10231/#comment39367>
s/hpp/hpp./
In general, all of our comments ends with periods.
third_party/libprocess/third_party/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/10231/#comment39368>
Instead of storing the duration here, this can be just one line:
ASSERT_ERROR(os::sleep(Milliseconds(-10));
- Ben Mahler
On April 9, 2013, 10:25 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10231/
> -----------------------------------------------------------
>
> (Updated April 9, 2013, 10:25 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> This is part 1.
> Part 2 would be to replace sleep() call in Mesos to use os::sleep().
>
>
> This addresses bug MESOS-415.
> https://issues.apache.org/jira/browse/MESOS-415
>
>
> Diffs
> -----
>
> third_party/libprocess/configure.ac cb7f0d73c6ddc1e75f7a3120858362c56a003aa7
> third_party/libprocess/third_party/stout/include/stout/os.hpp 5c217836a4a60402945544cba862b4c936ff0f83
> third_party/libprocess/third_party/stout/tests/os_tests.cpp d543b6d585097fbd6ee455d4807f23eb3b532a23
>
> Diff: https://reviews.apache.org/r/10231/diff/
>
>
> Testing
> -------
>
> Ran make check and
> ./mesos/build/third_party/libprocess/third_party/stout-tests --gtest_filter="OsTest.sleep" --gtest_repeat=500
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Added os::sleep() which takes a Duration object.
(libstout)
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10231/
-----------------------------------------------------------
(Updated April 13, 2013, 7:52 a.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Separated out libstout specific changes.
Summary (updated)
-----------------
Added os::sleep() which takes a Duration object. (libstout)
Description
-------
This is part 1.
Part 2 would be to replace sleep() call in Mesos to use os::sleep().
This addresses bug MESOS-415.
https://issues.apache.org/jira/browse/MESOS-415
Diffs (updated)
-----
third_party/libprocess/third_party/stout/include/stout/os.hpp 112edfd9955bd2c2f8e10fbd39d3d0a2e942909a
third_party/libprocess/third_party/stout/tests/os_tests.cpp d543b6d585097fbd6ee455d4807f23eb3b532a23
Diff: https://reviews.apache.org/r/10231/diff/
Testing (updated)
-------
Ran make check
Thanks,
Jiang Yan Xu
Re: Review Request: Added os::sleep which takes a Duration object.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10231/
-----------------------------------------------------------
(Updated April 9, 2013, 11:11 p.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Description
-------
This is part 1.
Part 2 would be to replace sleep() call in Mesos to use os::sleep().
This addresses bug MESOS-415.
https://issues.apache.org/jira/browse/MESOS-415
Diffs (updated)
-----
third_party/libprocess/configure.ac cb7f0d73c6ddc1e75f7a3120858362c56a003aa7
third_party/libprocess/third_party/stout/include/stout/os.hpp 5c217836a4a60402945544cba862b4c936ff0f83
third_party/libprocess/third_party/stout/tests/os_tests.cpp d543b6d585097fbd6ee455d4807f23eb3b532a23
Diff: https://reviews.apache.org/r/10231/diff/
Testing
-------
Ran make check and
./mesos/build/third_party/libprocess/third_party/stout-tests --gtest_filter="OsTest.sleep" --gtest_repeat=500
Thanks,
Jiang Yan Xu
Re: Review Request: Added os::sleep which takes a Duration object.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10231/
-----------------------------------------------------------
(Updated April 9, 2013, 10:25 p.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Addressed more bmahler's comments.
Description
-------
This is part 1.
Part 2 would be to replace sleep() call in Mesos to use os::sleep().
This addresses bug MESOS-415.
https://issues.apache.org/jira/browse/MESOS-415
Diffs (updated)
-----
third_party/libprocess/configure.ac cb7f0d73c6ddc1e75f7a3120858362c56a003aa7
third_party/libprocess/third_party/stout/include/stout/os.hpp 5c217836a4a60402945544cba862b4c936ff0f83
third_party/libprocess/third_party/stout/tests/os_tests.cpp d543b6d585097fbd6ee455d4807f23eb3b532a23
Diff: https://reviews.apache.org/r/10231/diff/
Testing
-------
Ran make check and
./mesos/build/third_party/libprocess/third_party/stout-tests --gtest_filter="OsTest.sleep" --gtest_repeat=500
Thanks,
Jiang Yan Xu
Re: Review Request: Added os::sleep which takes a Duration object.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On April 9, 2013, 7:56 p.m., Ben Mahler wrote:
> > third_party/libprocess/third_party/stout/tests/os_tests.cpp, line 205
> > <https://reviews.apache.org/r/10231/diff/3/?file=279778#file279778line205>
> >
> > Why not ASSERT_LE?
The assumption was that some time must have elapsed between the measurement of the time and the sleep itself, but there is no reason ASSERT_LE can't be used. So I changed it to ASSERT_LE.
> On April 9, 2013, 7:56 p.m., Ben Mahler wrote:
> > third_party/libprocess/configure.ac, line 104
> > <https://reviews.apache.org/r/10231/diff/3/?file=279776#file279776line104>
> >
> > Curious.. why did the need for this only surface now?
Because stout tests were compiled separately and previously no (stout) tests were using the rt library?
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10231/#review18877
-----------------------------------------------------------
On April 9, 2013, 7:28 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10231/
> -----------------------------------------------------------
>
> (Updated April 9, 2013, 7:28 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> This is part 1.
> Part 2 would be to replace sleep() call in Mesos to use os::sleep().
>
>
> This addresses bug MESOS-415.
> https://issues.apache.org/jira/browse/MESOS-415
>
>
> Diffs
> -----
>
> third_party/libprocess/configure.ac cb7f0d73c6ddc1e75f7a3120858362c56a003aa7
> third_party/libprocess/third_party/stout/include/stout/os.hpp 5c217836a4a60402945544cba862b4c936ff0f83
> third_party/libprocess/third_party/stout/tests/os_tests.cpp d543b6d585097fbd6ee455d4807f23eb3b532a23
>
> Diff: https://reviews.apache.org/r/10231/diff/
>
>
> Testing
> -------
>
> Ran make check and
> ./mesos/build/third_party/libprocess/third_party/stout-tests --gtest_filter="OsTest.sleep" --gtest_repeat=500
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Added os::sleep which takes a Duration object.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10231/#review18877
-----------------------------------------------------------
third_party/libprocess/configure.ac
<https://reviews.apache.org/r/10231/#comment39329>
Curious.. why did the need for this only surface now?
third_party/libprocess/configure.ac
<https://reviews.apache.org/r/10231/#comment39332>
You can kill this TODO since I'm assuming this works on OSX without -lrt?
third_party/libprocess/third_party/stout/include/stout/os.hpp
<https://reviews.apache.org/r/10231/#comment39335>
Now that this is called remaining, I think you can kill the comment.
third_party/libprocess/third_party/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/10231/#comment39336>
Why not ASSERT_LE?
- Ben Mahler
On April 9, 2013, 7:28 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10231/
> -----------------------------------------------------------
>
> (Updated April 9, 2013, 7:28 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> This is part 1.
> Part 2 would be to replace sleep() call in Mesos to use os::sleep().
>
>
> This addresses bug MESOS-415.
> https://issues.apache.org/jira/browse/MESOS-415
>
>
> Diffs
> -----
>
> third_party/libprocess/configure.ac cb7f0d73c6ddc1e75f7a3120858362c56a003aa7
> third_party/libprocess/third_party/stout/include/stout/os.hpp 5c217836a4a60402945544cba862b4c936ff0f83
> third_party/libprocess/third_party/stout/tests/os_tests.cpp d543b6d585097fbd6ee455d4807f23eb3b532a23
>
> Diff: https://reviews.apache.org/r/10231/diff/
>
>
> Testing
> -------
>
> Ran make check and
> ./mesos/build/third_party/libprocess/third_party/stout-tests --gtest_filter="OsTest.sleep" --gtest_repeat=500
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Added os::sleep which takes a Duration object.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10231/
-----------------------------------------------------------
(Updated April 9, 2013, 7:28 p.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Addressed bmahler's comments. Added flags -lrt to the linker because calling Stopwatch::now() from os_tests.cpp caused "undefined reference to `clock_gettime'".
Description
-------
This is part 1.
Part 2 would be to replace sleep() call in Mesos to use os::sleep().
This addresses bug MESOS-415.
https://issues.apache.org/jira/browse/MESOS-415
Diffs (updated)
-----
third_party/libprocess/configure.ac cb7f0d73c6ddc1e75f7a3120858362c56a003aa7
third_party/libprocess/third_party/stout/include/stout/os.hpp 5c217836a4a60402945544cba862b4c936ff0f83
third_party/libprocess/third_party/stout/tests/os_tests.cpp d543b6d585097fbd6ee455d4807f23eb3b532a23
Diff: https://reviews.apache.org/r/10231/diff/
Testing (updated)
-------
Ran make check and
./mesos/build/third_party/libprocess/third_party/stout-tests --gtest_filter="OsTest.sleep" --gtest_repeat=500
Thanks,
Jiang Yan Xu
Re: Review Request: Added os::sleep which takes a Duration object.
Posted by Jiang Yan Xu <jy...@twitter.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10231/
-----------------------------------------------------------
(Updated April 3, 2013, 11:17 p.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Addressed vinod's comments.
Description
-------
This is part 1.
Part 2 would be to replace sleep() call in Mesos to use os::sleep().
This addresses bug MESOS-415.
https://issues.apache.org/jira/browse/MESOS-415
Diffs (updated)
-----
third_party/libprocess/third_party/stout/include/stout/os.hpp 5c217836a4a60402945544cba862b4c936ff0f83
third_party/libprocess/third_party/stout/tests/os_tests.cpp d543b6d585097fbd6ee455d4807f23eb3b532a23
Diff: https://reviews.apache.org/r/10231/diff/
Testing
-------
Ran make check.
Thanks,
Jiang Yan Xu