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/02/13 21:13:54 UTC
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/
-----------------------------------------------------------
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/Makefile.am 168ecea
3rdparty/libprocess/3rdparty/boost-1.53.0.tar.gz 770d837aaba23d031b04ad77658f339587174aae
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
Thanks,
Jie Yu
Re: Review Request 18093: Added stout/interval.hpp to model boost interval
set.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review34416
-----------------------------------------------------------
Two thoughts:
(1) We should be more opinionated about how we want people to construct an interval, rather than providing multiple different ways and then trying to decide later which one we want people to use. Given the current API you've proposed, I would at least remove the leftOpen and rightOpen or remove the builder style since they accomplish the same thing (and in fact, the builder style also lets you do just 'open' and 'closed' too).
(2) C++ allows you to overload the "comma" operator which could make for a very interesting way to construct intervals, consider:
struct Interval
{
struct Open;
struct Closed;
struct Open
{
Open(int i) : i(i) {}
Interval operator , (const Open& open) { return Interval(...); }
Interval operator , (const Closed& closed) { return Interval(...); }
const int i;
};
struct Closed
{
Closed(int i) : i(i) {}
Interval operator , (const Open& open) { return Interval(...); }
Interval operator , (const Closed& closed) { return Interval(...); }
const int i;
}
private:
Interval(...) {}
...;
};
Then you can do:
Interval interval = (Interval::Open(10), Interval::Closed(42));
But note you _won't_ be able to do:
Interval interval = Interval::Open(10), Interval::Closed(42);
And then the only way to construct an interval is to finish the "building" by invoking one of the comma operators.
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment64507>
s/Syntax/Syntactic/
- Benjamin Hindman
On Feb. 14, 2014, 5:38 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2014, 5:38 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/Makefile.am 51b118c
> 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
>
>
> 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
>
>
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 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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/
-----------------------------------------------------------
(Updated Feb. 25, 2014, 1:08 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Added a few constructors for IntervalSet.
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/
-----------------------------------------------------------
(Updated Feb. 21, 2014, 7:23 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Reverted element iteration. Seems that it is needed (see https://reviews.apache.org/r/18368/, src/log/catchup.cpp).
Added overloads for IntervalSet. Added tests accordingly.
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/
-----------------------------------------------------------
(Updated Feb. 21, 2014, 1:16 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
BenH and BenM's comments. Rebased.
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 Feb. 21, 2014, 12:13 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 99-104
> > <https://reviews.apache.org/r/18093/diff/5/?file=499572#file499572line99>
> >
> > Is this useful outside of testing..?
It's only used in testing. Added a TODO.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review35114
-----------------------------------------------------------
On Feb. 20, 2014, 11:46 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 11:46 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>.
> On Feb. 21, 2014, 12:13 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 99-104
> > <https://reviews.apache.org/r/18093/diff/5/?file=499572#file499572line99>
> >
> > Is this useful outside of testing..?
>
> Jie Yu wrote:
> It's only used in testing. Added a TODO.
If it's just for testing can we remove it in favor of just using boost::icl::interval_count directly in the tests?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review35114
-----------------------------------------------------------
On Feb. 25, 2014, 1:08 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2014, 1:08 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/#review35114
-----------------------------------------------------------
Ship it!
This is a great first step to commit, now that you only have the functionality you'll be using. But, would it be beneficial to add TODOs for things we may need? ('Interval' iteration seems pretty essential for those looking to process the full set of values).
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment65522>
Is this useful outside of testing..?
3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp
<https://reviews.apache.org/r/18093/#comment65521>
#include <gtest/gtest.h>?
- Ben Mahler
On Feb. 20, 2014, 11:46 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 11:46 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>.
> On Feb. 21, 2014, 12:12 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, line 107
> > <https://reviews.apache.org/r/18093/diff/5/?file=499572#file499572line107>
> >
> > IMHO, providing element iteration can be sneaky and cause unintended performance issues. For example, consider a using IntervalSet for a very large interval which turns iteration (or the construction of a vector from such iteration) into an extremely expensive loop or a huge memory footprint. If we can get away without element iteration that would be preferred (but passing IntervalSet everyplace we used to use vector) or at least providing "interval" iteration instead of "element" (but let's not do this unless we really have a need.
>
> Jie Yu wrote:
> Killed.
>
> Ben Mahler wrote:
> I see you've re-added element iteration because you're other patch needed it. I would personally rather see Interval based iteration as we went through earlier in this review to avoid the issues of iterating over large IntervalSets:
>
> foreach (const Interval& interval, intervalSet) {
> // Deal with interval, preferably not with all Bound combinations.
> }
>
> This is safer than element iteration and would be sufficient for your other patch, right?
>
> Jie Yu wrote:
> Yes. Without the element iteration, the code in src/log/catchup.cpp (https://reviews.apache.org/r/18368/diff/#1.14) will become very ugly: you probably need to build a vector from the foreach loop above and iterate over that vector. That might blow up the memory if the interval is large. Using element iterator at least will not blow up the memory.
>
> Since we've already made using element iteration very explicit, I prefer leave it there for this case.
>
> Let me know your thoughts!
>
> Ben Mahler wrote:
> So the catchup code presumably is not dealing with a large set of positions, otherwise iterating over all of them would be prohibitively expensive? I'm ok with leaving element iteration given how explicit you've made it. We could leave a TODO to implement Interval iteration instead but it looks like your catchup code will be a bit messier if it has to deal with Intervals coming from begin() / end().
Per BenM's request, I am sketching here what the catchup code would look like w/ and w/o element iteration:
1) w/ element iteration:
class BulkCatchUpProcess : public Process<BulkCatchUpProcess> {
...
virtual void initialize() {
...
it = positions.elementsBegin();
catchup();
}
// Called before we try to catch-up a position.
void catchup() {
if (it == positions.elementsEnd()) {
promise.set(Nothing());
terminate(self());
return;
}
...
}
// Called after a position has been caught-up.
void succeeded() {
++it;
...
}
const IntervalSet<uint64_t> positions;
IntervalSet<uint64_t>::element_const_iterator it;
};
2) w/o element iteration. Assume that we have interval iteration and standardized bounds (i.e., [begin, end) ).
class BulkCatchUpProcess : public Process<BulkCatchUpProcess> {
...
virtual void initialize() {
...
it = positions.begin();
if (it != positions.end()) {
current = it->begin();
}
catchup();
}
// Called before we try to catch-up a position.
void catchup() {
if (current == it->end()) {
if (++it == positions.end()) {
promise.set(Nothing());
terminate(self());
return;
} else {
current = it->begin();
}
}
...
}
// Called after a position has been caught-up.
void succeeded() {
++current;
...
}
const IntervalSet<uint64_t> positions;
IntervalSet<uint64_t>::iterator it;
uint64_t current;
};
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review35111
-----------------------------------------------------------
On Feb. 25, 2014, 1:08 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2014, 1:08 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>.
> On Feb. 21, 2014, 12:12 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, line 107
> > <https://reviews.apache.org/r/18093/diff/5/?file=499572#file499572line107>
> >
> > IMHO, providing element iteration can be sneaky and cause unintended performance issues. For example, consider a using IntervalSet for a very large interval which turns iteration (or the construction of a vector from such iteration) into an extremely expensive loop or a huge memory footprint. If we can get away without element iteration that would be preferred (but passing IntervalSet everyplace we used to use vector) or at least providing "interval" iteration instead of "element" (but let's not do this unless we really have a need.
>
> Jie Yu wrote:
> Killed.
I see you've re-added element iteration because you're other patch needed it. I would personally rather see Interval based iteration as we went through earlier in this review to avoid the issues of iterating over large IntervalSets:
foreach (const Interval& interval, intervalSet) {
// Deal with interval, preferably not with all Bound combinations.
}
This is safer than element iteration and would be sufficient for your other patch, right?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review35111
-----------------------------------------------------------
On Feb. 25, 2014, 1:08 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2014, 1:08 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 Jie Yu <yu...@gmail.com>.
> On Feb. 21, 2014, 12:12 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, line 107
> > <https://reviews.apache.org/r/18093/diff/5/?file=499572#file499572line107>
> >
> > IMHO, providing element iteration can be sneaky and cause unintended performance issues. For example, consider a using IntervalSet for a very large interval which turns iteration (or the construction of a vector from such iteration) into an extremely expensive loop or a huge memory footprint. If we can get away without element iteration that would be preferred (but passing IntervalSet everyplace we used to use vector) or at least providing "interval" iteration instead of "element" (but let's not do this unless we really have a need.
>
> Jie Yu wrote:
> Killed.
>
> Ben Mahler wrote:
> I see you've re-added element iteration because you're other patch needed it. I would personally rather see Interval based iteration as we went through earlier in this review to avoid the issues of iterating over large IntervalSets:
>
> foreach (const Interval& interval, intervalSet) {
> // Deal with interval, preferably not with all Bound combinations.
> }
>
> This is safer than element iteration and would be sufficient for your other patch, right?
Yes. Without the element iteration, the code in src/log/catchup.cpp (https://reviews.apache.org/r/18368/diff/#1.14) will become very ugly: you probably need to build a vector from the foreach loop above and iterate over that vector. That might blow up the memory if the interval is large. Using element iterator at least will not blow up the memory.
Since we've already made using element iteration very explicit, I prefer leave it there for this case.
Let me know your thoughts!
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review35111
-----------------------------------------------------------
On Feb. 25, 2014, 1:08 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2014, 1:08 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>.
> On Feb. 21, 2014, 12:12 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, line 107
> > <https://reviews.apache.org/r/18093/diff/5/?file=499572#file499572line107>
> >
> > IMHO, providing element iteration can be sneaky and cause unintended performance issues. For example, consider a using IntervalSet for a very large interval which turns iteration (or the construction of a vector from such iteration) into an extremely expensive loop or a huge memory footprint. If we can get away without element iteration that would be preferred (but passing IntervalSet everyplace we used to use vector) or at least providing "interval" iteration instead of "element" (but let's not do this unless we really have a need.
>
> Jie Yu wrote:
> Killed.
>
> Ben Mahler wrote:
> I see you've re-added element iteration because you're other patch needed it. I would personally rather see Interval based iteration as we went through earlier in this review to avoid the issues of iterating over large IntervalSets:
>
> foreach (const Interval& interval, intervalSet) {
> // Deal with interval, preferably not with all Bound combinations.
> }
>
> This is safer than element iteration and would be sufficient for your other patch, right?
>
> Jie Yu wrote:
> Yes. Without the element iteration, the code in src/log/catchup.cpp (https://reviews.apache.org/r/18368/diff/#1.14) will become very ugly: you probably need to build a vector from the foreach loop above and iterate over that vector. That might blow up the memory if the interval is large. Using element iterator at least will not blow up the memory.
>
> Since we've already made using element iteration very explicit, I prefer leave it there for this case.
>
> Let me know your thoughts!
>
> Ben Mahler wrote:
> So the catchup code presumably is not dealing with a large set of positions, otherwise iterating over all of them would be prohibitively expensive? I'm ok with leaving element iteration given how explicit you've made it. We could leave a TODO to implement Interval iteration instead but it looks like your catchup code will be a bit messier if it has to deal with Intervals coming from begin() / end().
>
> Jie Yu wrote:
> Per BenM's request, I am sketching here what the catchup code would look like w/ and w/o element iteration:
>
> 1) w/ element iteration:
>
> class BulkCatchUpProcess : public Process<BulkCatchUpProcess> {
> ...
> virtual void initialize() {
> ...
> it = positions.elementsBegin();
> catchup();
> }
>
> // Called before we try to catch-up a position.
> void catchup() {
> if (it == positions.elementsEnd()) {
> promise.set(Nothing());
> terminate(self());
> return;
> }
> ...
> }
>
> // Called after a position has been caught-up.
> void succeeded() {
> ++it;
> ...
> }
>
> const IntervalSet<uint64_t> positions;
> IntervalSet<uint64_t>::element_const_iterator it;
> };
>
> 2) w/o element iteration. Assume that we have interval iteration and standardized bounds (i.e., [begin, end) ).
>
> class BulkCatchUpProcess : public Process<BulkCatchUpProcess> {
> ...
> virtual void initialize() {
> ...
> it = positions.begin();
> if (it != positions.end()) {
> current = it->begin();
> }
>
> catchup();
> }
>
> // Called before we try to catch-up a position.
> void catchup() {
> if (current == it->end()) {
> if (++it == positions.end()) {
> promise.set(Nothing());
> terminate(self());
> return;
> } else {
> current = it->begin();
> }
> }
> ...
> }
>
> // Called after a position has been caught-up.
> void succeeded() {
> ++current;
> ...
> }
>
> const IntervalSet<uint64_t> positions;
> IntervalSet<uint64_t>::iterator it;
> uint64_t current;
> };
Thanks! And some options for avoiding the need for IntervalSet in catchup:
1. Provide a cast operator for IntervalSet -> std::set so that catchup can take an std::set instead.
2. Provide a more explicit 'std::set elements() const' method for converting to std::set for passing in to catchup.
What I'm concerned about is that we're saying: use IntervalSet when dealing with possibly very large sets / ranges of numbers. The caveat being, be careful to avoid iterating over the elements since it might take a very long time! It seems like we should consider converting away from IntervalSet when we know the space is not large. For example, in catchup, why bother using an IntervalSet? It seems the only reason is that the caller wants to pass an IntervalSet in.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review35111
-----------------------------------------------------------
On Feb. 25, 2014, 1:08 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2014, 1:08 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>.
> On Feb. 21, 2014, 12:12 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, line 107
> > <https://reviews.apache.org/r/18093/diff/5/?file=499572#file499572line107>
> >
> > IMHO, providing element iteration can be sneaky and cause unintended performance issues. For example, consider a using IntervalSet for a very large interval which turns iteration (or the construction of a vector from such iteration) into an extremely expensive loop or a huge memory footprint. If we can get away without element iteration that would be preferred (but passing IntervalSet everyplace we used to use vector) or at least providing "interval" iteration instead of "element" (but let's not do this unless we really have a need.
>
> Jie Yu wrote:
> Killed.
>
> Ben Mahler wrote:
> I see you've re-added element iteration because you're other patch needed it. I would personally rather see Interval based iteration as we went through earlier in this review to avoid the issues of iterating over large IntervalSets:
>
> foreach (const Interval& interval, intervalSet) {
> // Deal with interval, preferably not with all Bound combinations.
> }
>
> This is safer than element iteration and would be sufficient for your other patch, right?
>
> Jie Yu wrote:
> Yes. Without the element iteration, the code in src/log/catchup.cpp (https://reviews.apache.org/r/18368/diff/#1.14) will become very ugly: you probably need to build a vector from the foreach loop above and iterate over that vector. That might blow up the memory if the interval is large. Using element iterator at least will not blow up the memory.
>
> Since we've already made using element iteration very explicit, I prefer leave it there for this case.
>
> Let me know your thoughts!
So the catchup code presumably is not dealing with a large set of positions, otherwise iterating over all of them would be prohibitively expensive? I'm ok with leaving element iteration given how explicit you've made it. We could leave a TODO to implement Interval iteration instead but it looks like your catchup code will be a bit messier if it has to deal with Intervals coming from begin() / end().
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review35111
-----------------------------------------------------------
On Feb. 25, 2014, 1:08 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2014, 1:08 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 Jie Yu <yu...@gmail.com>.
> On Feb. 21, 2014, 12:12 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, line 107
> > <https://reviews.apache.org/r/18093/diff/5/?file=499572#file499572line107>
> >
> > IMHO, providing element iteration can be sneaky and cause unintended performance issues. For example, consider a using IntervalSet for a very large interval which turns iteration (or the construction of a vector from such iteration) into an extremely expensive loop or a huge memory footprint. If we can get away without element iteration that would be preferred (but passing IntervalSet everyplace we used to use vector) or at least providing "interval" iteration instead of "element" (but let's not do this unless we really have a need.
Killed.
> On Feb. 21, 2014, 12:12 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, line 136
> > <https://reviews.apache.org/r/18093/diff/5/?file=499572#file499572line136>
> >
> > Did you consider about making these member operators? It seems that then we could do:
> >
> > template <typename X>
> > IntervalSet<T>& operator += (const X& x)
> > {
> > operator += (static_cast<boost::icl::interval_set<T>&>(*this), x));
> > return *this;
> > }
> >
> > Thoughts?
I liked the idea! But your code does not compile because the compiler will try to match += (recursively) in IntervalSet. I changed a little bit, and it works:
template <typename X>
IntervalSet<T>& operator += (const X& x)
{
static_cast<boost::icl::interval_set<T>&>(*this) += x;
return *this;
}
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review35111
-----------------------------------------------------------
On Feb. 20, 2014, 11:46 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 11:46 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 Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review35111
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment65523>
IMHO, providing element iteration can be sneaky and cause unintended performance issues. For example, consider a using IntervalSet for a very large interval which turns iteration (or the construction of a vector from such iteration) into an extremely expensive loop or a huge memory footprint. If we can get away without element iteration that would be preferred (but passing IntervalSet everyplace we used to use vector) or at least providing "interval" iteration instead of "element" (but let's not do this unless we really have a need.
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment65516>
Did you consider about making these member operators? It seems that then we could do:
template <typename X>
IntervalSet<T>& operator += (const X& x)
{
operator += (static_cast<boost::icl::interval_set<T>&>(*this), x));
return *this;
}
Thoughts?
- Benjamin Hindman
On Feb. 20, 2014, 11:46 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 11:46 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 Feb. 20, 2014, 11:46 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Rebased. Removed the unnecessary custom interval abstraction.
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/
-----------------------------------------------------------
(Updated Feb. 19, 2014, 8:48 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Removed interval iteration related code as no one is using that interface.
Tested the std container integration.
Bugs: MESOS-993
https://issues.apache.org/jira/browse/MESOS-993
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/Makefile.am 51b118c
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>.
> On Feb. 17, 2014, 9:24 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp, line 166
> > <https://reviews.apache.org/r/18093/diff/3/?file=487340#file487340line166>
> >
> > Is there support for iterating over an individual interval as well? Or did you want to hide that for the reasons you mentioned earlier with respect to the interval being potentially very large?
> >
> > If there's support, perhaps we should show how to do it in the tests:
> >
> > foreach (int i, interval) {
> > ...
> > }
>
> Jie Yu wrote:
> In fact, foreach only supports one type of iteration (element or segment). I prefer segment iteration because element iteration is dangerous if an interval is very large. If a user wants to do element iteration, it has to be explicit (see IntervalTest.ElementIteration).
What I meant was being able to iterate on an Interval, not an IntervalSet, like:
foreach (const Interval& interval, intervalSet) {
foreach (int i, interval) { // This.
// ...
}
}
I agree for IntervalSet we should do segment iteration, we're on the same page there. :)
Just wondering how someone could iterate over an Interval, especially given the left / right bound type permutations ({OPEN,OPEN}, {OPEN,CLOSED}, ...).
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review34665
-----------------------------------------------------------
On Feb. 17, 2014, 1:46 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2014, 1:46 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/Makefile.am 51b118c
> 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 Feb. 17, 2014, 9:24 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp, lines 167-168
> > <https://reviews.apache.org/r/18093/diff/3/?file=487340#file487340line167>
> >
> > Can these be called lower() and upper()?
That's gonna hide the lower() and upper() in discrete_interval, on which type traits depend.
> On Feb. 17, 2014, 9:24 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp, line 166
> > <https://reviews.apache.org/r/18093/diff/3/?file=487340#file487340line166>
> >
> > Is there support for iterating over an individual interval as well? Or did you want to hide that for the reasons you mentioned earlier with respect to the interval being potentially very large?
> >
> > If there's support, perhaps we should show how to do it in the tests:
> >
> > foreach (int i, interval) {
> > ...
> > }
In fact, foreach only supports one type of iteration (element or segment). I prefer segment iteration because element iteration is dangerous if an interval is very large. If a user wants to do element iteration, it has to be explicit (see IntervalTest.ElementIteration).
> On Feb. 17, 2014, 9:24 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp, lines 170-182
> > <https://reviews.apache.org/r/18093/diff/3/?file=487340#file487340line170>
> >
> > I'm trying to think of how a caller would interact with this API. It seems a little cumbersome to work with, since you need to deal with all the permutations of left and right bound types, right?
> >
> > Is it possible to standardize the bounds as [lower, upper) as is done in the standard library?
> >
> > int lower = interval.lower(); // Returns inclusive lower.
> > int upper = interval.upper(); // Returns exclusive upper.
Hum... Are you asking for an iterator for Interval?
To get the inclusive left bound, returns *begin();
To get the exclusive right bound, returns *end();
And, we can also do:
foreach (const T& value, interval) {
...
}
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review34665
-----------------------------------------------------------
On Feb. 17, 2014, 1:46 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2014, 1:46 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/Makefile.am 51b118c
> 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/#review34665
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp
<https://reviews.apache.org/r/18093/#comment64872>
Is there support for iterating over an individual interval as well? Or did you want to hide that for the reasons you mentioned earlier with respect to the interval being potentially very large?
If there's support, perhaps we should show how to do it in the tests:
foreach (int i, interval) {
...
}
3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp
<https://reviews.apache.org/r/18093/#comment64871>
Can these be called lower() and upper()?
3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp
<https://reviews.apache.org/r/18093/#comment64873>
I'm trying to think of how a caller would interact with this API. It seems a little cumbersome to work with, since you need to deal with all the permutations of left and right bound types, right?
Is it possible to standardize the bounds as [lower, upper) as is done in the standard library?
int lower = interval.lower(); // Returns inclusive lower.
int upper = interval.upper(); // Returns exclusive upper.
- Ben Mahler
On Feb. 17, 2014, 1:46 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2014, 1:46 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/Makefile.am 51b118c
> 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 Feb. 18, 2014, 5:46 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, line 147
> > <https://reviews.apache.org/r/18093/diff/3/?file=487339#file487339line147>
> >
> > could you use composition instead of inheritance here? It might avoid some of the awkward static_cast<...>(*this) constructions. It also means that users of the class won't be tempted to try to use the underlying interval_set directly.
I agree that using composition is, in general, better than inheritance when extending a core data structure because it avoids some subtle issues (e.g. base class does not have a virtual destructor, etc.). However, using composition makes the code really verbose as you need to provide a proxy for every single method you wanna use.
Therefore, in our code base, we use inheritance in many places (e.g. hashmap, hashset, etc.) if we are not adding any new fields. I guess our philology is: more code -> more bugs. The users of this API should not try to use boost directly, which is pretty easy to enforce and check.
> On Feb. 18, 2014, 5:46 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, line 84
> > <https://reviews.apache.org/r/18093/diff/3/?file=487339#file487339line84>
> >
> > could this be replaced by a method that gets both bounds in a pair?
> >
> > ie: std::pair<Bound<T>, Bound<T>> bounds() const;
We are not fond of std::pair because:
To get the left bound, you need to do: pair.first
This makes the code a little hard to read.
> On Feb. 18, 2014, 5:46 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, line 53
> > <https://reviews.apache.org/r/18093/diff/3/?file=487339#file487339line53>
> >
> > This is clever, but falls in the 'just because you can, doesn't mean you should' category. You can get the same result by replacing the factory constructors above with:
> >
> > openOpen, openClosed, closedOpen, closedClosed.
> >
> > This is more explicit, less verbose, and less surprising.
I am OK with both. What do other think here? Please weigh in:)
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review34713
-----------------------------------------------------------
On Feb. 17, 2014, 1:46 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2014, 1:46 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/Makefile.am 51b118c
> 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 Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review34713
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment64928>
This is clever, but falls in the 'just because you can, doesn't mean you should' category. You can get the same result by replacing the factory constructors above with:
openOpen, openClosed, closedOpen, closedClosed.
This is more explicit, less verbose, and less surprising.
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment64929>
const T& return?
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment64932>
could this be replaced by a method that gets both bounds in a pair?
ie: std::pair<Bound<T>, Bound<T>> bounds() const;
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment64930>
you can avoid the 'else' here as the 'if' is returning.
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment64934>
could you use composition instead of inheritance here? It might avoid some of the awkward static_cast<...>(*this) constructions. It also means that users of the class won't be tempted to try to use the underlying interval_set directly.
- Dominic Hamon
On Feb. 16, 2014, 5:46 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2014, 5:46 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/Makefile.am 51b118c
> 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 Feb. 17, 2014, 1:46 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
BenH and BenM. Introduced "Interval" so that we can more easily do segment iteration:
foreach (const Interval<int>& interval, intervalSet) {
Bound<int> left = interval.left();
Bound<int> right = interval.right();
...
}
Also, according to BenH's suggestion, we now can do the following:
intervalSet += (Bound<int>::open(2), Bound<int>::closed(5));
Bugs: MESOS-993
https://issues.apache.org/jira/browse/MESOS-993
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/Makefile.am 51b118c
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 (updated)
-------
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>.
> On Feb. 14, 2014, 5:54 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 143-146
> > <https://reviews.apache.org/r/18093/diff/2/?file=484865#file484865line143>
> >
> > I'm wondering about the use-cases here to see if the current iterator semantics are the easiest to work with.
> >
> > Do you see most people using this as a plain 'set' (with a compact representation) or mostly as a 'set of intervals', or both?
> >
> > For the first use case, I can imagine calling this 'IntervalSet', but exposing the same 'Set' like interface as, say, hashset.
> >
> > For the second use case, I can imagine calling this 'Intervals', and exposing the current interface.
> >
> > For example, what should the default iterator behavior be?
> >
> > // Case 1:
> > foreach (const T& t, set) {
> > ...
> > }
> >
> > // Case 2:
> > foreach (const Interval& i, intervals) {
> > ...
> > }
> >
> > Just something to think about. It seems like you will be using this mostly as a 'Set'.
>
> Jie Yu wrote:
> There is a note in the doc:
>
> "So we advice you to choose element iteration over interval containers judiciously. Do not use element iteration by default or habitual. Always try to achieve results using member functions, global functions or operators (preferably inplace versions) or iteration over segments first."
>
> I guess the reason is because doing iteration on elements are somewhat dangerous because you might have a very large interval (e.g. imaging that you insert an interval [1,100000000]). Therefore, I think maybe we should stick to the original boost semantics and make element iteration explicit.
>
> Also, changing the default behavior of begin()/end() is a bit tricky since you cannot simply extend from boost interval_set anymore :(
>
> Regarding the Intervals idea, I liked that! My concern is: we haven't seen use cases currently. Maybe we should delay that until we see some actual use cases which then can help us design a good API. What do you think?
>
> Ben Mahler wrote:
> Sounds great, given your comment can you also add interval iteration in the tests? I'm curious what that looks like.
>
> // Would be nice to be able to do something like this for iteration.
> foreach (const Interval& interval, intervalSet) {
> ..
> }
>
> Jie Yu wrote:
> You have a good point. I haven't thought about the interval iteration enough. To do the interval iteration now, you'll have to do the following:
>
> foreach (const discrete_interval<T>& interval, intervalSet) {
> T left = interval.lower();
> T right = interval.upper();
>
> if (interval.bounds() == interval_bounds::right_open()) {
>
> ...
> } else if (interval.bounds() == interval_bounds::closed()) {
> ...
> }
> ...
> }
>
> Seems that we need to abstract Interval as well, which might need to create a customized interval type.
> http://www.boost.org/doc/libs/1_53_0/libs/icl/doc/html/boost_icl/examples/custom_interval.html
>
> Any suggestion?
Hm, is it possible to have Interval inherit from discrete_interval, but provide simpler accessors for upper() and lower()?
What I would find most intuitive is an inclusive (closed) lower() and exclusive (open) upper(), since this is how things like begin() and end() work. With these, the caller does not need to know about the bounds().
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review34507
-----------------------------------------------------------
On Feb. 14, 2014, 5:38 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2014, 5:38 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/Makefile.am 51b118c
> 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
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 18093: Added stout/interval.hpp to model boost interval
set.
Posted by Jie Yu <yu...@gmail.com>.
> On Feb. 14, 2014, 5:54 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 143-146
> > <https://reviews.apache.org/r/18093/diff/2/?file=484865#file484865line143>
> >
> > I'm wondering about the use-cases here to see if the current iterator semantics are the easiest to work with.
> >
> > Do you see most people using this as a plain 'set' (with a compact representation) or mostly as a 'set of intervals', or both?
> >
> > For the first use case, I can imagine calling this 'IntervalSet', but exposing the same 'Set' like interface as, say, hashset.
> >
> > For the second use case, I can imagine calling this 'Intervals', and exposing the current interface.
> >
> > For example, what should the default iterator behavior be?
> >
> > // Case 1:
> > foreach (const T& t, set) {
> > ...
> > }
> >
> > // Case 2:
> > foreach (const Interval& i, intervals) {
> > ...
> > }
> >
> > Just something to think about. It seems like you will be using this mostly as a 'Set'.
>
> Jie Yu wrote:
> There is a note in the doc:
>
> "So we advice you to choose element iteration over interval containers judiciously. Do not use element iteration by default or habitual. Always try to achieve results using member functions, global functions or operators (preferably inplace versions) or iteration over segments first."
>
> I guess the reason is because doing iteration on elements are somewhat dangerous because you might have a very large interval (e.g. imaging that you insert an interval [1,100000000]). Therefore, I think maybe we should stick to the original boost semantics and make element iteration explicit.
>
> Also, changing the default behavior of begin()/end() is a bit tricky since you cannot simply extend from boost interval_set anymore :(
>
> Regarding the Intervals idea, I liked that! My concern is: we haven't seen use cases currently. Maybe we should delay that until we see some actual use cases which then can help us design a good API. What do you think?
>
> Ben Mahler wrote:
> Sounds great, given your comment can you also add interval iteration in the tests? I'm curious what that looks like.
>
> // Would be nice to be able to do something like this for iteration.
> foreach (const Interval& interval, intervalSet) {
> ..
> }
You have a good point. I haven't thought about the interval iteration enough. To do the interval iteration now, you'll have to do the following:
foreach (const discrete_interval<T>& interval, intervalSet) {
T left = interval.lower();
T right = interval.upper();
if (interval.bounds() == interval_bounds::right_open()) {
...
} else if (interval.bounds() == interval_bounds::closed()) {
...
}
...
}
Seems that we need to abstract Interval as well, which might need to create a customized interval type.
http://www.boost.org/doc/libs/1_53_0/libs/icl/doc/html/boost_icl/examples/custom_interval.html
Any suggestion?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review34507
-----------------------------------------------------------
On Feb. 14, 2014, 5:38 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2014, 5:38 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/Makefile.am 51b118c
> 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
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 18093: Added stout/interval.hpp to model boost interval
set.
Posted by Jie Yu <yu...@gmail.com>.
> On Feb. 14, 2014, 5:54 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 143-146
> > <https://reviews.apache.org/r/18093/diff/2/?file=484865#file484865line143>
> >
> > I'm wondering about the use-cases here to see if the current iterator semantics are the easiest to work with.
> >
> > Do you see most people using this as a plain 'set' (with a compact representation) or mostly as a 'set of intervals', or both?
> >
> > For the first use case, I can imagine calling this 'IntervalSet', but exposing the same 'Set' like interface as, say, hashset.
> >
> > For the second use case, I can imagine calling this 'Intervals', and exposing the current interface.
> >
> > For example, what should the default iterator behavior be?
> >
> > // Case 1:
> > foreach (const T& t, set) {
> > ...
> > }
> >
> > // Case 2:
> > foreach (const Interval& i, intervals) {
> > ...
> > }
> >
> > Just something to think about. It seems like you will be using this mostly as a 'Set'.
>
> Jie Yu wrote:
> There is a note in the doc:
>
> "So we advice you to choose element iteration over interval containers judiciously. Do not use element iteration by default or habitual. Always try to achieve results using member functions, global functions or operators (preferably inplace versions) or iteration over segments first."
>
> I guess the reason is because doing iteration on elements are somewhat dangerous because you might have a very large interval (e.g. imaging that you insert an interval [1,100000000]). Therefore, I think maybe we should stick to the original boost semantics and make element iteration explicit.
>
> Also, changing the default behavior of begin()/end() is a bit tricky since you cannot simply extend from boost interval_set anymore :(
>
> Regarding the Intervals idea, I liked that! My concern is: we haven't seen use cases currently. Maybe we should delay that until we see some actual use cases which then can help us design a good API. What do you think?
>
> Ben Mahler wrote:
> Sounds great, given your comment can you also add interval iteration in the tests? I'm curious what that looks like.
>
> // Would be nice to be able to do something like this for iteration.
> foreach (const Interval& interval, intervalSet) {
> ..
> }
>
> Jie Yu wrote:
> You have a good point. I haven't thought about the interval iteration enough. To do the interval iteration now, you'll have to do the following:
>
> foreach (const discrete_interval<T>& interval, intervalSet) {
> T left = interval.lower();
> T right = interval.upper();
>
> if (interval.bounds() == interval_bounds::right_open()) {
>
> ...
> } else if (interval.bounds() == interval_bounds::closed()) {
> ...
> }
> ...
> }
>
> Seems that we need to abstract Interval as well, which might need to create a customized interval type.
> http://www.boost.org/doc/libs/1_53_0/libs/icl/doc/html/boost_icl/examples/custom_interval.html
>
> Any suggestion?
>
> Ben Mahler wrote:
> Hm, is it possible to have Interval inherit from discrete_interval, but provide simpler accessors for upper() and lower()?
>
> What I would find most intuitive is an inclusive (closed) lower() and exclusive (open) upper(), since this is how things like begin() and end() work. With these, the caller does not need to know about the bounds().
I did a research on how to provide custom interval (not trivial, but doable). This is the only option that allows us to do the following:
foreach (const Interval<T>& interval, intervalSet) { ... }
We can do whatever we want in class Interval (e.g. as you suggested). However, when creating intervals, we need to do the following (by specifying the type):
intervalSet += Interval<int>::open(2).closed(4); // Notice that we need to specify '<int>' here.
Or, we can create another class called "IntervalBuilder":
intervalSet += IntervalBuilder::open(3).closed(4);
Which one do you guys prefer?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review34507
-----------------------------------------------------------
On Feb. 14, 2014, 5:38 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2014, 5:38 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/Makefile.am 51b118c
> 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
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 18093: Added stout/interval.hpp to model boost interval
set.
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 14, 2014, 5:54 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 143-146
> > <https://reviews.apache.org/r/18093/diff/2/?file=484865#file484865line143>
> >
> > I'm wondering about the use-cases here to see if the current iterator semantics are the easiest to work with.
> >
> > Do you see most people using this as a plain 'set' (with a compact representation) or mostly as a 'set of intervals', or both?
> >
> > For the first use case, I can imagine calling this 'IntervalSet', but exposing the same 'Set' like interface as, say, hashset.
> >
> > For the second use case, I can imagine calling this 'Intervals', and exposing the current interface.
> >
> > For example, what should the default iterator behavior be?
> >
> > // Case 1:
> > foreach (const T& t, set) {
> > ...
> > }
> >
> > // Case 2:
> > foreach (const Interval& i, intervals) {
> > ...
> > }
> >
> > Just something to think about. It seems like you will be using this mostly as a 'Set'.
>
> Jie Yu wrote:
> There is a note in the doc:
>
> "So we advice you to choose element iteration over interval containers judiciously. Do not use element iteration by default or habitual. Always try to achieve results using member functions, global functions or operators (preferably inplace versions) or iteration over segments first."
>
> I guess the reason is because doing iteration on elements are somewhat dangerous because you might have a very large interval (e.g. imaging that you insert an interval [1,100000000]). Therefore, I think maybe we should stick to the original boost semantics and make element iteration explicit.
>
> Also, changing the default behavior of begin()/end() is a bit tricky since you cannot simply extend from boost interval_set anymore :(
>
> Regarding the Intervals idea, I liked that! My concern is: we haven't seen use cases currently. Maybe we should delay that until we see some actual use cases which then can help us design a good API. What do you think?
Sounds great, given your comment can you also add interval iteration in the tests? I'm curious what that looks like.
// Would be nice to be able to do something like this for iteration.
foreach (const Interval& interval, intervalSet) {
..
}
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review34507
-----------------------------------------------------------
On Feb. 14, 2014, 5:38 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2014, 5:38 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/Makefile.am 51b118c
> 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
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 18093: Added stout/interval.hpp to model boost interval
set.
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 14, 2014, 5:54 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 143-146
> > <https://reviews.apache.org/r/18093/diff/2/?file=484865#file484865line143>
> >
> > I'm wondering about the use-cases here to see if the current iterator semantics are the easiest to work with.
> >
> > Do you see most people using this as a plain 'set' (with a compact representation) or mostly as a 'set of intervals', or both?
> >
> > For the first use case, I can imagine calling this 'IntervalSet', but exposing the same 'Set' like interface as, say, hashset.
> >
> > For the second use case, I can imagine calling this 'Intervals', and exposing the current interface.
> >
> > For example, what should the default iterator behavior be?
> >
> > // Case 1:
> > foreach (const T& t, set) {
> > ...
> > }
> >
> > // Case 2:
> > foreach (const Interval& i, intervals) {
> > ...
> > }
> >
> > Just something to think about. It seems like you will be using this mostly as a 'Set'.
>
> Jie Yu wrote:
> There is a note in the doc:
>
> "So we advice you to choose element iteration over interval containers judiciously. Do not use element iteration by default or habitual. Always try to achieve results using member functions, global functions or operators (preferably inplace versions) or iteration over segments first."
>
> I guess the reason is because doing iteration on elements are somewhat dangerous because you might have a very large interval (e.g. imaging that you insert an interval [1,100000000]). Therefore, I think maybe we should stick to the original boost semantics and make element iteration explicit.
>
> Also, changing the default behavior of begin()/end() is a bit tricky since you cannot simply extend from boost interval_set anymore :(
>
> Regarding the Intervals idea, I liked that! My concern is: we haven't seen use cases currently. Maybe we should delay that until we see some actual use cases which then can help us design a good API. What do you think?
>
> Ben Mahler wrote:
> Sounds great, given your comment can you also add interval iteration in the tests? I'm curious what that looks like.
>
> // Would be nice to be able to do something like this for iteration.
> foreach (const Interval& interval, intervalSet) {
> ..
> }
>
> Jie Yu wrote:
> You have a good point. I haven't thought about the interval iteration enough. To do the interval iteration now, you'll have to do the following:
>
> foreach (const discrete_interval<T>& interval, intervalSet) {
> T left = interval.lower();
> T right = interval.upper();
>
> if (interval.bounds() == interval_bounds::right_open()) {
>
> ...
> } else if (interval.bounds() == interval_bounds::closed()) {
> ...
> }
> ...
> }
>
> Seems that we need to abstract Interval as well, which might need to create a customized interval type.
> http://www.boost.org/doc/libs/1_53_0/libs/icl/doc/html/boost_icl/examples/custom_interval.html
>
> Any suggestion?
>
> Ben Mahler wrote:
> Hm, is it possible to have Interval inherit from discrete_interval, but provide simpler accessors for upper() and lower()?
>
> What I would find most intuitive is an inclusive (closed) lower() and exclusive (open) upper(), since this is how things like begin() and end() work. With these, the caller does not need to know about the bounds().
>
> Jie Yu wrote:
> I did a research on how to provide custom interval (not trivial, but doable). This is the only option that allows us to do the following:
>
> foreach (const Interval<T>& interval, intervalSet) { ... }
>
> We can do whatever we want in class Interval (e.g. as you suggested). However, when creating intervals, we need to do the following (by specifying the type):
>
> intervalSet += Interval<int>::open(2).closed(4); // Notice that we need to specify '<int>' here.
>
> Or, we can create another class called "IntervalBuilder":
>
> intervalSet += IntervalBuilder::open(3).closed(4);
>
> Which one do you guys prefer?
>
I think requiring the type qualifier is an acceptable tradeoff for achieving clean iteration. So I would avoid the separate IntervalBuilder for now.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review34507
-----------------------------------------------------------
On Feb. 14, 2014, 5:38 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2014, 5:38 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/Makefile.am 51b118c
> 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
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 18093: Added stout/interval.hpp to model boost interval
set.
Posted by Jie Yu <yu...@gmail.com>.
> On Feb. 14, 2014, 5:54 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 194-207
> > <https://reviews.apache.org/r/18093/diff/2/?file=484865#file484865line194>
> >
> > Interesting that they provided both + and |, are they not the same operation? (In set.hpp, we provided | & as the set operators, and + for inserting single elements).
They are the same!
"operator |= and operator | are behavioral identical to operator += and operator +. This is a redundancy that has been introduced deliberately, because a set union semantics is often attached operators |= and |."
> On Feb. 14, 2014, 5:54 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 138-139
> > <https://reviews.apache.org/r/18093/diff/2/?file=484865#file484865line138>
> >
> > non-const for copying, or?
Should be "const T left". Will fix that.
> On Feb. 14, 2014, 5:54 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 143-146
> > <https://reviews.apache.org/r/18093/diff/2/?file=484865#file484865line143>
> >
> > I'm wondering about the use-cases here to see if the current iterator semantics are the easiest to work with.
> >
> > Do you see most people using this as a plain 'set' (with a compact representation) or mostly as a 'set of intervals', or both?
> >
> > For the first use case, I can imagine calling this 'IntervalSet', but exposing the same 'Set' like interface as, say, hashset.
> >
> > For the second use case, I can imagine calling this 'Intervals', and exposing the current interface.
> >
> > For example, what should the default iterator behavior be?
> >
> > // Case 1:
> > foreach (const T& t, set) {
> > ...
> > }
> >
> > // Case 2:
> > foreach (const Interval& i, intervals) {
> > ...
> > }
> >
> > Just something to think about. It seems like you will be using this mostly as a 'Set'.
There is a note in the doc:
"So we advice you to choose element iteration over interval containers judiciously. Do not use element iteration by default or habitual. Always try to achieve results using member functions, global functions or operators (preferably inplace versions) or iteration over segments first."
I guess the reason is because doing iteration on elements are somewhat dangerous because you might have a very large interval (e.g. imaging that you insert an interval [1,100000000]). Therefore, I think maybe we should stick to the original boost semantics and make element iteration explicit.
Also, changing the default behavior of begin()/end() is a bit tricky since you cannot simply extend from boost interval_set anymore :(
Regarding the Intervals idea, I liked that! My concern is: we haven't seen use cases currently. Maybe we should delay that until we see some actual use cases which then can help us design a good API. What do you think?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18093/#review34507
-----------------------------------------------------------
On Feb. 14, 2014, 5:38 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2014, 5:38 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/Makefile.am 51b118c
> 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
>
>
> 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/#review34507
-----------------------------------------------------------
Ship it!
Looks great, I'm wondering if we should provide 'IntervalSet' as more of a Set-like abstraction (in terms of default foreach() behavior).
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment64604>
What about just using an 'else' here and in closed?
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment64605>
non-const for copying, or?
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment64608>
I'm wondering about the use-cases here to see if the current iterator semantics are the easiest to work with.
Do you see most people using this as a plain 'set' (with a compact representation) or mostly as a 'set of intervals', or both?
For the first use case, I can imagine calling this 'IntervalSet', but exposing the same 'Set' like interface as, say, hashset.
For the second use case, I can imagine calling this 'Intervals', and exposing the current interface.
For example, what should the default iterator behavior be?
// Case 1:
foreach (const T& t, set) {
...
}
// Case 2:
foreach (const Interval& i, intervals) {
...
}
Just something to think about. It seems like you will be using this mostly as a 'Set'.
3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp
<https://reviews.apache.org/r/18093/#comment64611>
Interesting that they provided both + and |, are they not the same operation? (In set.hpp, we provided | & as the set operators, and + for inserting single elements).
- Ben Mahler
On Feb. 14, 2014, 5:38 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18093/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2014, 5:38 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/Makefile.am 51b118c
> 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
>
>
> 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 Feb. 14, 2014, 5:38 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Introduced interval builder style. Rebased on master.
Bugs: MESOS-993
https://issues.apache.org/jira/browse/MESOS-993
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/Makefile.am 51b118c
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
Thanks,
Jie Yu