You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2014/03/01 08:31:44 UTC
Re: Review Request 18093: Added stout/interval.hpp to model boost interval
set.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/
-----------------------------------------------------------
(Updated March 1, 2014, 7:31 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Removed element iteration. Added interval iteration support.
Standardized lower (inclusive) and upper (exclusive) bounds.
Bugs: MESOS-993
https://issues.apache.org/jira/browse/MESOS-993
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/Makefile.am 5d5a760
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/18093/diff/
Testing
-------
make check
repeat 1000 times for the IntervalTest
Repeating all tests (iteration 100) . . .
Note: Google Test filter = *Interval*
[==========] Running 7 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 7 tests from IntervalTest
[ RUN ] IntervalTest.Interval
[ OK ] IntervalTest.Interval (0 ms)
[ RUN ] IntervalTest.Addition
[ OK ] IntervalTest.Addition (0 ms)
[ RUN ] IntervalTest.Subtraction
[ OK ] IntervalTest.Subtraction (0 ms)
[ RUN ] IntervalTest.Intersection
[ OK ] IntervalTest.Intersection (0 ms)
[ RUN ] IntervalTest.LargeInterval
[ OK ] IntervalTest.LargeInterval (0 ms)
[ RUN ] IntervalTest.ElementIteration
[ OK ] IntervalTest.ElementIteration (0 ms)
[ RUN ] IntervalTest.IntervalIteration
[ OK ] IntervalTest.IntervalIteration (0 ms)
[----------] 7 tests from IntervalTest (0 ms total)
[----------] Global test environment tear-down
[==========] 7 tests from 1 test case ran. (0 ms total)
[ PASSED ] 7 tests.
Thanks,
Jie Yu
Re: Review Request 18093: Added stout/interval.hpp to model boost
interval set.
Posted by Jie Yu <yu...@gmail.com>.
Thank you guys for the great comments! It's now committed.
- Jie
On Tue, Mar 4, 2014 at 11:06 AM, Ben Mahler <be...@gmail.com>wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/#review36142
> -----------------------------------------------------------
>
> Ship it!
>
>
> Ship It!
>
> - Ben Mahler
>
>
> On March 4, 2014, 6:58 p.m., Jie Yu wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/18093/
> > -----------------------------------------------------------
> >
> > (Updated March 4, 2014, 6:58 p.m.)
> >
> >
> > Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> >
> >
> > Bugs: MESOS-993
> > https://issues.apache.org/jira/browse/MESOS-993
> >
> >
> > Repository: mesos-git
> >
> >
> > Description
> > -------
> >
> > See summary.
> >
> >
> > Diffs
> > -----
> >
> > 3rdparty/libprocess/3rdparty/stout/Makefile.am 5d5a760
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
> PRE-CREATION
> > 3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp
> PRE-CREATION
> >
> > Diff: https://reviews.apache.org/r/18093/diff/
> >
> >
> > Testing
> > -------
> >
> > make check
> >
> > repeat 1000 times for the IntervalTest
> >
> > Repeating all tests (iteration 100) . . .
> >
> > Note: Google Test filter = *Interval*
> > [==========] Running 7 tests from 1 test case.
> > [----------] Global test environment set-up.
> > [----------] 7 tests from IntervalTest
> > [ RUN ] IntervalTest.Interval
> > [ OK ] IntervalTest.Interval (0 ms)
> > [ RUN ] IntervalTest.Addition
> > [ OK ] IntervalTest.Addition (0 ms)
> > [ RUN ] IntervalTest.Subtraction
> > [ OK ] IntervalTest.Subtraction (0 ms)
> > [ RUN ] IntervalTest.Intersection
> > [ OK ] IntervalTest.Intersection (0 ms)
> > [ RUN ] IntervalTest.LargeInterval
> > [ OK ] IntervalTest.LargeInterval (0 ms)
> > [ RUN ] IntervalTest.ElementIteration
> > [ OK ] IntervalTest.ElementIteration (0 ms)
> > [ RUN ] IntervalTest.IntervalIteration
> > [ OK ] IntervalTest.IntervalIteration (0 ms)
> > [----------] 7 tests from IntervalTest (0 ms total)
> >
> > [----------] Global test environment tear-down
> > [==========] 7 tests from 1 test case ran. (0 ms total)
> > [ PASSED ] 7 tests.
> >
> >
> > Thanks,
> >
> > Jie Yu
> >
> >
>
>
Re: Review Request 18093: Added stout/interval.hpp to model boost interval
set.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review36142
-----------------------------------------------------------
Ship it!
Ship It!
- Ben Mahler
On March 4, 2014, 6:58 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated March 4, 2014, 6:58 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-993
> https://issues.apache.org/jira/browse/MESOS-993
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 5d5a760
> 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/18093/diff/
>
>
> Testing
> -------
>
> make check
>
> repeat 1000 times for the IntervalTest
>
> Repeating all tests (iteration 100) . . .
>
> Note: Google Test filter = *Interval*
> [==========] Running 7 tests from 1 test case.
> [----------] Global test environment set-up.
> [----------] 7 tests from IntervalTest
> [ RUN ] IntervalTest.Interval
> [ OK ] IntervalTest.Interval (0 ms)
> [ RUN ] IntervalTest.Addition
> [ OK ] IntervalTest.Addition (0 ms)
> [ RUN ] IntervalTest.Subtraction
> [ OK ] IntervalTest.Subtraction (0 ms)
> [ RUN ] IntervalTest.Intersection
> [ OK ] IntervalTest.Intersection (0 ms)
> [ RUN ] IntervalTest.LargeInterval
> [ OK ] IntervalTest.LargeInterval (0 ms)
> [ RUN ] IntervalTest.ElementIteration
> [ OK ] IntervalTest.ElementIteration (0 ms)
> [ RUN ] IntervalTest.IntervalIteration
> [ OK ] IntervalTest.IntervalIteration (0 ms)
> [----------] 7 tests from IntervalTest (0 ms total)
>
> [----------] Global test environment tear-down
> [==========] 7 tests from 1 test case ran. (0 ms total)
> [ PASSED ] 7 tests.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 18093: Added stout/interval.hpp to model boost interval
set.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/
-----------------------------------------------------------
(Updated March 4, 2014, 6:58 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
BenM's comments.
Bugs: MESOS-993
https://issues.apache.org/jira/browse/MESOS-993
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/Makefile.am 5d5a760
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/18093/diff/
Testing
-------
make check
repeat 1000 times for the IntervalTest
Repeating all tests (iteration 100) . . .
Note: Google Test filter = *Interval*
[==========] Running 7 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 7 tests from IntervalTest
[ RUN ] IntervalTest.Interval
[ OK ] IntervalTest.Interval (0 ms)
[ RUN ] IntervalTest.Addition
[ OK ] IntervalTest.Addition (0 ms)
[ RUN ] IntervalTest.Subtraction
[ OK ] IntervalTest.Subtraction (0 ms)
[ RUN ] IntervalTest.Intersection
[ OK ] IntervalTest.Intersection (0 ms)
[ RUN ] IntervalTest.LargeInterval
[ OK ] IntervalTest.LargeInterval (0 ms)
[ RUN ] IntervalTest.ElementIteration
[ OK ] IntervalTest.ElementIteration (0 ms)
[ RUN ] IntervalTest.IntervalIteration
[ OK ] IntervalTest.IntervalIteration (0 ms)
[----------] 7 tests from IntervalTest (0 ms total)
[----------] Global test environment tear-down
[==========] 7 tests from 1 test case ran. (0 ms total)
[ PASSED ] 7 tests.
Thanks,
Jie Yu
Re: Review Request 18093: Added stout/interval.hpp to model boost interval
set.
Posted by Jie Yu <yu...@gmail.com>.
> On March 4, 2014, 1:36 a.m., Ben Mahler wrote:
> > I think the only remaining bit that might be tricky is that we expose 'Bound' for 'Interval' construction but then we only expose inclusive lower() and exclusive upper(). I think this is better than exposing the 'Bounds' from interval given the inherent complexity that arises from checking all 'Bound' combinations, so LGTM.
> >
> > I would be curious to know whether you ended up using different 'Bound' types in your code that uses IntervalSet. Was it useful?
Please refer to the following review. Seems that we use different Bound types in different cases. Therefore, I think it's useful to expose all combinations.
https://reviews.apache.org/r/18368/
src/log/replica.cpp +276
positions += (Bound<uint64_t>::open(end), Bound<uint64_t>::closed(to));
src/log/replica.cpp +280
positions &= (Bound<uint64_t>::closed(from), Bound<uint64_t>::closed(to));
src/log/replica.cpp +679
holes -= (Bound<uint64_t>::open(0), Bound<uint64_t>::open(action.truncate().to()));
> On March 4, 2014, 1:36 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 167-172
> > <https://reviews.apache.org/r/18093/diff/9/?file=507713#file507713line167>
> >
> > Could we kill this and just use std::distance in the tests?
In fact, i wanna expose this function and remove the TODO. It makes sense to expose the number of intervals given that we have exposed Interval. What do you think?
> On March 4, 2014, 1:36 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 174-187
> > <https://reviews.apache.org/r/18093/diff/9/?file=507713#file507713line174>
> >
> > In set.hpp, we provided '+' for individual item additions and '|' for set union. hashset.hpp also provides only '|' for set union.
> >
> > I would prefer to only see 1 way to do union, even though boost supplies both operators for union, we should restrict the number of ways of doing the same thing.
> >
> > How about we:
> > -only use '|' (as Python does: http://docs.python.org/2/library/sets.html#set-objects), or
> > -only use '+' (inconsistent with (hash)set.hpp), or
> > -use | for IntervalSet, + for T, + for Interval (this seems confusing and I'm not sure we made the right decision in set.hpp).
> >
> > Seems like we could avoid potential confusion here where one wonders if + and | are different operators.
Removed '|' to avoid confusion.
> On March 4, 2014, 1:36 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 90-91
> > <https://reviews.apache.org/r/18093/diff/9/?file=507713#file507713line90>
> >
> > Can you add a comment as to why you are storing the right_open_interval as opposed to inheriting from it? Would the latter be cleaner?
No convincing reason:) Just don't wanna Interval to be passed to bare boost icl containers.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review36067
-----------------------------------------------------------
On March 1, 2014, 7:31 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated March 1, 2014, 7:31 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-993
> https://issues.apache.org/jira/browse/MESOS-993
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 5d5a760
> 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/18093/diff/
>
>
> Testing
> -------
>
> make check
>
> repeat 1000 times for the IntervalTest
>
> Repeating all tests (iteration 100) . . .
>
> Note: Google Test filter = *Interval*
> [==========] Running 7 tests from 1 test case.
> [----------] Global test environment set-up.
> [----------] 7 tests from IntervalTest
> [ RUN ] IntervalTest.Interval
> [ OK ] IntervalTest.Interval (0 ms)
> [ RUN ] IntervalTest.Addition
> [ OK ] IntervalTest.Addition (0 ms)
> [ RUN ] IntervalTest.Subtraction
> [ OK ] IntervalTest.Subtraction (0 ms)
> [ RUN ] IntervalTest.Intersection
> [ OK ] IntervalTest.Intersection (0 ms)
> [ RUN ] IntervalTest.LargeInterval
> [ OK ] IntervalTest.LargeInterval (0 ms)
> [ RUN ] IntervalTest.ElementIteration
> [ OK ] IntervalTest.ElementIteration (0 ms)
> [ RUN ] IntervalTest.IntervalIteration
> [ OK ] IntervalTest.IntervalIteration (0 ms)
> [----------] 7 tests from IntervalTest (0 ms total)
>
> [----------] Global test environment tear-down
> [==========] 7 tests from 1 test case ran. (0 ms total)
> [ PASSED ] 7 tests.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 18093: Added stout/interval.hpp to model boost interval
set.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review36067
-----------------------------------------------------------
I think the only remaining bit that might be tricky is that we expose 'Bound' for 'Interval' construction but then we only expose inclusive lower() and exclusive upper(). I think this is better than exposing the 'Bounds' from interval given the inherent complexity that arises from checking all 'Bound' combinations, so LGTM.
I would be curious to know whether you ended up using different 'Bound' types in your code that uses IntervalSet. Was it useful?
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment66930>
Can you add a comment as to why you are storing the right_open_interval as opposed to inheriting from it? Would the latter be cleaner?
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment66932>
Maybe some inline comments here?
right_open_interval // Target.
is_discrete<T>
static_open // Input bounds.
static_right_open // Output bounds.
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment66900>
Could we kill this and just use std::distance in the tests?
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment66944>
In set.hpp, we provided '+' for individual item additions and '|' for set union. hashset.hpp also provides only '|' for set union.
I would prefer to only see 1 way to do union, even though boost supplies both operators for union, we should restrict the number of ways of doing the same thing.
How about we:
-only use '|' (as Python does: http://docs.python.org/2/library/sets.html#set-objects), or
-only use '+' (inconsistent with (hash)set.hpp), or
-use | for IntervalSet, + for T, + for Interval (this seems confusing and I'm not sure we made the right decision in set.hpp).
Seems like we could avoid potential confusion here where one wonders if + and | are different operators.
- Ben Mahler
On March 1, 2014, 7:31 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated March 1, 2014, 7:31 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-993
> https://issues.apache.org/jira/browse/MESOS-993
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 5d5a760
> 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/18093/diff/
>
>
> Testing
> -------
>
> make check
>
> repeat 1000 times for the IntervalTest
>
> Repeating all tests (iteration 100) . . .
>
> Note: Google Test filter = *Interval*
> [==========] Running 7 tests from 1 test case.
> [----------] Global test environment set-up.
> [----------] 7 tests from IntervalTest
> [ RUN ] IntervalTest.Interval
> [ OK ] IntervalTest.Interval (0 ms)
> [ RUN ] IntervalTest.Addition
> [ OK ] IntervalTest.Addition (0 ms)
> [ RUN ] IntervalTest.Subtraction
> [ OK ] IntervalTest.Subtraction (0 ms)
> [ RUN ] IntervalTest.Intersection
> [ OK ] IntervalTest.Intersection (0 ms)
> [ RUN ] IntervalTest.LargeInterval
> [ OK ] IntervalTest.LargeInterval (0 ms)
> [ RUN ] IntervalTest.ElementIteration
> [ OK ] IntervalTest.ElementIteration (0 ms)
> [ RUN ] IntervalTest.IntervalIteration
> [ OK ] IntervalTest.IntervalIteration (0 ms)
> [----------] 7 tests from IntervalTest (0 ms total)
>
> [----------] Global test environment tear-down
> [==========] 7 tests from 1 test case ran. (0 ms total)
> [ PASSED ] 7 tests.
>
>
> Thanks,
>
> Jie Yu
>
>