You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2013/05/30 19:59:56 UTC
Review Request: Added a Bandwidth class which provides utilities for network
bandwidth computation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11542/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Vinod Kone.
Description
-------
- This implementation uses Boost::rational<uint64_t> to store both the bits and the nanoseconds as integers to preserve precision.
- It requires the boost lib to be repackaged to include Boost::rational.
- The usage 'rational' handles avoid overflow cases better (it doesn't) than simply multiply the denominators in various arithmetic operations.
- Bandwidth always >= 0
- Bandwidth constructor passes 'boost::bad_rational' up when the denominator is zero.
Diffs
-----
3rdparty/libprocess/3rdparty/Makefile.am 7a9ede62145e3150f7af6675d4384feafd9c0a88
3rdparty/libprocess/3rdparty/boost-1.53.0.tar.gz 770d837aaba23d031b04ad77658f339587174aae
3rdparty/libprocess/3rdparty/stout/Makefile.am 84062a0e1dfe4ec04bac7cac5ebaac4b945eb66e
3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/11542/diff/
Testing
-------
make check
Thanks,
Jiang Yan Xu
Re: Review Request: Added a Bandwidth class which provides utilities for
network bandwidth computation.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 3, 2013, 10:39 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp, line 10
> > <https://reviews.apache.org/r/11542/diff/2/?file=299690#file299690line10>
> >
> > I'm not sold on the benefit of using boost rational here, were there other motivations besides the bad_rational handling?
> >
> > We generally do not use exceptions in libstout, but I see why you may have wanted to given the constructor can fail. But what are the failure scenarios you would like to handle? It seems a resulting SIGFPE from a duration of 0 is sufficient, no?
I think it is intuitive because it semantically has the two components that we measure as integers: the numerator (# of bits) and the denominator (duration) but if we store them as one 'double', we lose precision in cases such as "10/3: 10 bits per 3 seconds".
Alternatively we can store the numerator and the denominator separately as two integer fields but in operators such as '+' and '==' we need to do all the math (LCM/GCD) that rational class already does for us.
=====
If we stick with rational then I am only passing the bad_rational exception up to the caller.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11542/#review21363
-----------------------------------------------------------
On May 31, 2013, 9:20 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11542/
> -----------------------------------------------------------
>
> (Updated May 31, 2013, 9:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> - This implementation uses Boost::rational<uint64_t> to store both the bits and the nanoseconds as integers to preserve precision.
> - It requires the boost lib to be repackaged to include Boost::rational.
> - The usage 'rational' handles avoid overflow cases better (it doesn't) than simply multiply the denominators in various arithmetic operations.
> - Bandwidth always >= 0
> - Bandwidth constructor passes 'boost::bad_rational' up when the denominator is zero.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 7a9ede62145e3150f7af6675d4384feafd9c0a88
> 3rdparty/libprocess/3rdparty/boost-1.53.0.tar.gz 770d837aaba23d031b04ad77658f339587174aae
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 84062a0e1dfe4ec04bac7cac5ebaac4b945eb66e
> 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11542/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Added a Bandwidth class which provides utilities for
network bandwidth computation.
Posted by Ben Mahler <be...@gmail.com>.
> On June 3, 2013, 10:39 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp, line 10
> > <https://reviews.apache.org/r/11542/diff/2/?file=299690#file299690line10>
> >
> > I'm not sold on the benefit of using boost rational here, were there other motivations besides the bad_rational handling?
> >
> > We generally do not use exceptions in libstout, but I see why you may have wanted to given the constructor can fail. But what are the failure scenarios you would like to handle? It seems a resulting SIGFPE from a duration of 0 is sufficient, no?
>
> Jiang Yan Xu wrote:
> I think it is intuitive because it semantically has the two components that we measure as integers: the numerator (# of bits) and the denominator (duration) but if we store them as one 'double', we lose precision in cases such as "10/3: 10 bits per 3 seconds".
> Alternatively we can store the numerator and the denominator separately as two integer fields but in operators such as '+' and '==' we need to do all the math (LCM/GCD) that rational class already does for us.
>
> =====
>
> If we stick with rational then I am only passing the bad_rational exception up to the caller.
I brought this up as we generally try to avoid using boost libraries where not needed.
In this case I don't think the precision loss is severe, especially considering we are dealing with discrete values under the hood (bits) with a discrete (ns) denominator. Can you convince me otherwise?
I get the sense that 95% of the Bandwidth users be using 1 second as the denominator. If so, then I'm in favor of a simpler implementation at the cost of potential precision loss in certain use-cases. What are these cases?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11542/#review21363
-----------------------------------------------------------
On May 31, 2013, 9:20 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11542/
> -----------------------------------------------------------
>
> (Updated May 31, 2013, 9:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> - This implementation uses Boost::rational<uint64_t> to store both the bits and the nanoseconds as integers to preserve precision.
> - It requires the boost lib to be repackaged to include Boost::rational.
> - The usage 'rational' handles avoid overflow cases better (it doesn't) than simply multiply the denominators in various arithmetic operations.
> - Bandwidth always >= 0
> - Bandwidth constructor passes 'boost::bad_rational' up when the denominator is zero.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 7a9ede62145e3150f7af6675d4384feafd9c0a88
> 3rdparty/libprocess/3rdparty/boost-1.53.0.tar.gz 770d837aaba23d031b04ad77658f339587174aae
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 84062a0e1dfe4ec04bac7cac5ebaac4b945eb66e
> 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11542/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Added a Bandwidth class which provides utilities for
network bandwidth computation.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 3, 2013, 10:39 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp, line 10
> > <https://reviews.apache.org/r/11542/diff/2/?file=299690#file299690line10>
> >
> > I'm not sold on the benefit of using boost rational here, were there other motivations besides the bad_rational handling?
> >
> > We generally do not use exceptions in libstout, but I see why you may have wanted to given the constructor can fail. But what are the failure scenarios you would like to handle? It seems a resulting SIGFPE from a duration of 0 is sufficient, no?
>
> Jiang Yan Xu wrote:
> I think it is intuitive because it semantically has the two components that we measure as integers: the numerator (# of bits) and the denominator (duration) but if we store them as one 'double', we lose precision in cases such as "10/3: 10 bits per 3 seconds".
> Alternatively we can store the numerator and the denominator separately as two integer fields but in operators such as '+' and '==' we need to do all the math (LCM/GCD) that rational class already does for us.
>
> =====
>
> If we stick with rational then I am only passing the bad_rational exception up to the caller.
>
> Ben Mahler wrote:
> I brought this up as we generally try to avoid using boost libraries where not needed.
>
> In this case I don't think the precision loss is severe, especially considering we are dealing with discrete values under the hood (bits) with a discrete (ns) denominator. Can you convince me otherwise?
>
> I get the sense that 95% of the Bandwidth users be using 1 second as the denominator. If so, then I'm in favor of a simpler implementation at the cost of potential precision loss in certain use-cases. What are these cases?
My first implementation used 'double' type (not submitted for review)...
The case we are concerned about not being able to precisely represent repeating decimals is when determining whether resources are fully allocated: 0.333... + 0.666... = 0.999... (!= 1) while 1/3 + 2/3 == 1
There are certainly techniques to deal with this but in general I think we are preferring to use more precise representations ('double' -> 'int64_t' in Duration).
If what makes this case special is that we are depending on the boost library class I think this is a general design issue and I am OK with either of the two approaches.
==
I think there are two cases in general
1) we measure periodically (not necessarily per 1 second but can be normalized to 1 second)
2) we measure when an event is fired, which could be any Duration
With the rational class, the math (normalization) is done automatically. You just give it a Bytes and a Duration object without calculating the bandwidth yourself.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11542/#review21363
-----------------------------------------------------------
On May 31, 2013, 9:20 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11542/
> -----------------------------------------------------------
>
> (Updated May 31, 2013, 9:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> - This implementation uses Boost::rational<uint64_t> to store both the bits and the nanoseconds as integers to preserve precision.
> - It requires the boost lib to be repackaged to include Boost::rational.
> - The usage 'rational' handles avoid overflow cases better (it doesn't) than simply multiply the denominators in various arithmetic operations.
> - Bandwidth always >= 0
> - Bandwidth constructor passes 'boost::bad_rational' up when the denominator is zero.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 7a9ede62145e3150f7af6675d4384feafd9c0a88
> 3rdparty/libprocess/3rdparty/boost-1.53.0.tar.gz 770d837aaba23d031b04ad77658f339587174aae
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 84062a0e1dfe4ec04bac7cac5ebaac4b945eb66e
> 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11542/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Added a Bandwidth class which provides utilities for
network bandwidth computation.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11542/#review21363
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp
<https://reviews.apache.org/r/11542/#comment44299>
I'm not sold on the benefit of using boost rational here, were there other motivations besides the bad_rational handling?
We generally do not use exceptions in libstout, but I see why you may have wanted to given the constructor can fail. But what are the failure scenarios you would like to handle? It seems a resulting SIGFPE from a duration of 0 is sufficient, no?
3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp
<https://reviews.apache.org/r/11542/#comment44297>
kill?
- Ben Mahler
On May 31, 2013, 9:20 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11542/
> -----------------------------------------------------------
>
> (Updated May 31, 2013, 9:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> - This implementation uses Boost::rational<uint64_t> to store both the bits and the nanoseconds as integers to preserve precision.
> - It requires the boost lib to be repackaged to include Boost::rational.
> - The usage 'rational' handles avoid overflow cases better (it doesn't) than simply multiply the denominators in various arithmetic operations.
> - Bandwidth always >= 0
> - Bandwidth constructor passes 'boost::bad_rational' up when the denominator is zero.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 7a9ede62145e3150f7af6675d4384feafd9c0a88
> 3rdparty/libprocess/3rdparty/boost-1.53.0.tar.gz 770d837aaba23d031b04ad77658f339587174aae
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 84062a0e1dfe4ec04bac7cac5ebaac4b945eb66e
> 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11542/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Added a Bandwidth class which provides utilities for
network bandwidth computation.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11542/#review22261
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp
<https://reviews.apache.org/r/11542/#comment45732>
unused?
- Ben Mahler
On May 31, 2013, 9:20 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11542/
> -----------------------------------------------------------
>
> (Updated May 31, 2013, 9:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> - This implementation uses Boost::rational<uint64_t> to store both the bits and the nanoseconds as integers to preserve precision.
> - It requires the boost lib to be repackaged to include Boost::rational.
> - The usage 'rational' handles avoid overflow cases better (it doesn't) than simply multiply the denominators in various arithmetic operations.
> - Bandwidth always >= 0
> - Bandwidth constructor passes 'boost::bad_rational' up when the denominator is zero.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 7a9ede62145e3150f7af6675d4384feafd9c0a88
> 3rdparty/libprocess/3rdparty/boost-1.53.0.tar.gz 770d837aaba23d031b04ad77658f339587174aae
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 84062a0e1dfe4ec04bac7cac5ebaac4b945eb66e
> 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11542/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Added a Bandwidth class which provides utilities for
network bandwidth computation.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 5, 2013, 3:13 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp, line 10
> > <https://reviews.apache.org/r/11542/diff/2/?file=299690#file299690line10>
> >
> > I'm not sure what you mean by "an event is fired" in your second point?
> >
> > The main hesitation is that if anyone wants to use this nice Bandwidth abstraction, they must use boost. Libstout is a 3rd party library on github, so there is an upside to not relying on boost. :)
> >
> > I like the idea of keeping things precise, but it seems like simplifying this to a double bitsPerSecond is sufficient. There's loss on the way out anyway, as you convert to doubles, correct? If it turns out that we need the extra precision, then we can consider boost/rational at that point?
> >
> > Duration was a simple double implementation, and once we realized it wasn't sufficient, we fixed it. I think the same approach here can keep things simple for now, and adjust if there's a need.
> >
> > Thinking about how this might be used, I would expect nearly all of the cases to operate with Seconds(1) as the duration. In which case, using rational doesn't give us anything, right?
> >
> > Let me know your thoughts!
By "an event is fired" I mean if we are not using a loop to monitor the tx, rx bits with a fixed interval but we check it when an event occurs such as upon receiving a message, system signal or other things if some condition holds, then the Duration could be e.g. 1231.23243243243 seconds since the starting point of the current monitoring cycle.
Ok I am all for adding things when needed so I am OK with changing it to use a double for now.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11542/#review21462
-----------------------------------------------------------
On May 31, 2013, 9:20 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11542/
> -----------------------------------------------------------
>
> (Updated May 31, 2013, 9:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> - This implementation uses Boost::rational<uint64_t> to store both the bits and the nanoseconds as integers to preserve precision.
> - It requires the boost lib to be repackaged to include Boost::rational.
> - The usage 'rational' handles avoid overflow cases better (it doesn't) than simply multiply the denominators in various arithmetic operations.
> - Bandwidth always >= 0
> - Bandwidth constructor passes 'boost::bad_rational' up when the denominator is zero.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 7a9ede62145e3150f7af6675d4384feafd9c0a88
> 3rdparty/libprocess/3rdparty/boost-1.53.0.tar.gz 770d837aaba23d031b04ad77658f339587174aae
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 84062a0e1dfe4ec04bac7cac5ebaac4b945eb66e
> 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11542/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Added a Bandwidth class which provides utilities for
network bandwidth computation.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11542/#review21462
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp
<https://reviews.apache.org/r/11542/#comment44460>
I'm not sure what you mean by "an event is fired" in your second point?
The main hesitation is that if anyone wants to use this nice Bandwidth abstraction, they must use boost. Libstout is a 3rd party library on github, so there is an upside to not relying on boost. :)
I like the idea of keeping things precise, but it seems like simplifying this to a double bitsPerSecond is sufficient. There's loss on the way out anyway, as you convert to doubles, correct? If it turns out that we need the extra precision, then we can consider boost/rational at that point?
Duration was a simple double implementation, and once we realized it wasn't sufficient, we fixed it. I think the same approach here can keep things simple for now, and adjust if there's a need.
Thinking about how this might be used, I would expect nearly all of the cases to operate with Seconds(1) as the duration. In which case, using rational doesn't give us anything, right?
Let me know your thoughts!
- Ben Mahler
On May 31, 2013, 9:20 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11542/
> -----------------------------------------------------------
>
> (Updated May 31, 2013, 9:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> - This implementation uses Boost::rational<uint64_t> to store both the bits and the nanoseconds as integers to preserve precision.
> - It requires the boost lib to be repackaged to include Boost::rational.
> - The usage 'rational' handles avoid overflow cases better (it doesn't) than simply multiply the denominators in various arithmetic operations.
> - Bandwidth always >= 0
> - Bandwidth constructor passes 'boost::bad_rational' up when the denominator is zero.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 7a9ede62145e3150f7af6675d4384feafd9c0a88
> 3rdparty/libprocess/3rdparty/boost-1.53.0.tar.gz 770d837aaba23d031b04ad77658f339587174aae
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 84062a0e1dfe4ec04bac7cac5ebaac4b945eb66e
> 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11542/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Added a Bandwidth class which provides utilities for
network bandwidth computation.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11542/
-----------------------------------------------------------
(Updated May 31, 2013, 9:20 p.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Addressed Vinod's comments.
Description
-------
- This implementation uses Boost::rational<uint64_t> to store both the bits and the nanoseconds as integers to preserve precision.
- It requires the boost lib to be repackaged to include Boost::rational.
- The usage 'rational' handles avoid overflow cases better (it doesn't) than simply multiply the denominators in various arithmetic operations.
- Bandwidth always >= 0
- Bandwidth constructor passes 'boost::bad_rational' up when the denominator is zero.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/Makefile.am 7a9ede62145e3150f7af6675d4384feafd9c0a88
3rdparty/libprocess/3rdparty/boost-1.53.0.tar.gz 770d837aaba23d031b04ad77658f339587174aae
3rdparty/libprocess/3rdparty/stout/Makefile.am 84062a0e1dfe4ec04bac7cac5ebaac4b945eb66e
3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/11542/diff/
Testing
-------
make check
Thanks,
Jiang Yan Xu
Re: Review Request: Added a Bandwidth class which provides utilities for
network bandwidth computation.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On May 30, 2013, 6:24 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp, line 62
> > <https://reviews.apache.org/r/11542/diff/1/?file=299032#file299032line62>
> >
> > was the compiler complaining about the lack of cast here?
Yes, the compiler complains about type mismatch because duration.ns() returns an int64_t.
I tried to use '0LL' but it doesn't work either so then decided to use casting.
> On May 30, 2013, 6:24 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp, lines 126-149
> > <https://reviews.apache.org/r/11542/diff/1/?file=299032#file299032line126>
> >
> > how about calling these functions bps(), kbps() etc?
I'd love to, but see the explanation above.
> On May 30, 2013, 6:24 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp, lines 39-49
> > <https://reviews.apache.org/r/11542/diff/1/?file=299032#file299032line39>
> >
> > how about "bps", "kbps" etc too?
> >
> > also kbit, mbit etc doesn't make sense for bandwidth no?
The units are very confusing...
According to BenH, we want to comply with the units used in "tc".
In tc http://linux.die.net/man/8/tc
"mbit" means "Megabits per second" while "mbps" means "Megabytes per second"
This contradicts how the unit "mbps" is often used... it is however, not a standard.
The standard says we should be "Mibit/s" to mean 2^20 bits per second and "Mbit/s" to mean 10^6 bits per second but every programmer seems to use "Mbit/s" to mean "2^20 bits per second"...
http://en.wikipedia.org/wiki/Bit_rate
I'd prefer to use kbps etc to mean "kilobits per second" and elaborate on this in the documentation if there's no objection.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11542/#review21198
-----------------------------------------------------------
On May 30, 2013, 5:59 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11542/
> -----------------------------------------------------------
>
> (Updated May 30, 2013, 5:59 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> - This implementation uses Boost::rational<uint64_t> to store both the bits and the nanoseconds as integers to preserve precision.
> - It requires the boost lib to be repackaged to include Boost::rational.
> - The usage 'rational' handles avoid overflow cases better (it doesn't) than simply multiply the denominators in various arithmetic operations.
> - Bandwidth always >= 0
> - Bandwidth constructor passes 'boost::bad_rational' up when the denominator is zero.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 7a9ede62145e3150f7af6675d4384feafd9c0a88
> 3rdparty/libprocess/3rdparty/boost-1.53.0.tar.gz 770d837aaba23d031b04ad77658f339587174aae
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 84062a0e1dfe4ec04bac7cac5ebaac4b945eb66e
> 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11542/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Added a Bandwidth class which provides utilities for
network bandwidth computation.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On May 30, 2013, 6:24 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp, lines 98-101
> > <https://reviews.apache.org/r/11542/diff/1/?file=299033#file299033line98>
> >
> > woah. this is hard to comprehend. can we simpler numbers here?
I added some comments here. The numbers don't really mean much but but basically I just wanted to show/check what the outputs of multiple bandwidths of different units added together look like. (15 digits of full precision, no trailing zeros, etc).
> On May 30, 2013, 6:24 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp, line 32
> > <https://reviews.apache.org/r/11542/diff/1/?file=299033#file299033line32>
> >
> > can we test a good constructor too?
Added two EXPECT_NO_THROW cases. Good constructors are used throughout the test.
> On May 30, 2013, 6:24 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp, lines 126-149
> > <https://reviews.apache.org/r/11542/diff/1/?file=299032#file299032line126>
> >
> > how about calling these functions bps(), kbps() etc?
>
> Jiang Yan Xu wrote:
> I'd love to, but see the explanation above.
Keeping the method names in the longer form to avoid ambiguity.
> On May 30, 2013, 6:24 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp, lines 39-49
> > <https://reviews.apache.org/r/11542/diff/1/?file=299032#file299032line39>
> >
> > how about "bps", "kbps" etc too?
> >
> > also kbit, mbit etc doesn't make sense for bandwidth no?
>
> Jiang Yan Xu wrote:
> The units are very confusing...
>
> According to BenH, we want to comply with the units used in "tc".
> In tc http://linux.die.net/man/8/tc
> "mbit" means "Megabits per second" while "mbps" means "Megabytes per second"
>
> This contradicts how the unit "mbps" is often used... it is however, not a standard.
>
> The standard says we should be "Mibit/s" to mean 2^20 bits per second and "Mbit/s" to mean 10^6 bits per second but every programmer seems to use "Mbit/s" to mean "2^20 bits per second"...
> http://en.wikipedia.org/wiki/Bit_rate
>
> I'd prefer to use kbps etc to mean "kilobits per second" and elaborate on this in the documentation if there's no objection.
Improved documentation to explain 'mbit' is 'tc' style.
Changed 'kbit/s' to 'Kbit/s'.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11542/#review21198
-----------------------------------------------------------
On May 31, 2013, 9:20 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11542/
> -----------------------------------------------------------
>
> (Updated May 31, 2013, 9:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> - This implementation uses Boost::rational<uint64_t> to store both the bits and the nanoseconds as integers to preserve precision.
> - It requires the boost lib to be repackaged to include Boost::rational.
> - The usage 'rational' handles avoid overflow cases better (it doesn't) than simply multiply the denominators in various arithmetic operations.
> - Bandwidth always >= 0
> - Bandwidth constructor passes 'boost::bad_rational' up when the denominator is zero.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 7a9ede62145e3150f7af6675d4384feafd9c0a88
> 3rdparty/libprocess/3rdparty/boost-1.53.0.tar.gz 770d837aaba23d031b04ad77658f339587174aae
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 84062a0e1dfe4ec04bac7cac5ebaac4b945eb66e
> 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11542/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Added a Bandwidth class which provides utilities for
network bandwidth computation.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11542/#review21198
-----------------------------------------------------------
loved the tests!
3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp
<https://reviews.apache.org/r/11542/#comment44024>
s/support/supports/
3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp
<https://reviews.apache.org/r/11542/#comment44025>
how about "bps", "kbps" etc too?
also kbit, mbit etc doesn't make sense for bandwidth no?
3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp
<https://reviews.apache.org/r/11542/#comment44028>
was the compiler complaining about the lack of cast here?
3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp
<https://reviews.apache.org/r/11542/#comment44029>
how about calling these functions bps(), kbps() etc?
3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp
<https://reviews.apache.org/r/11542/#comment44030>
outer elements should be separated by 2 lines.
here and everywhere below.
3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp
<https://reviews.apache.org/r/11542/#comment44031>
can we test a good constructor too?
3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp
<https://reviews.apache.org/r/11542/#comment44032>
sweet!
3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp
<https://reviews.apache.org/r/11542/#comment44033>
woah. this is hard to comprehend. can we simpler numbers here?
- Vinod Kone
On May 30, 2013, 5:59 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11542/
> -----------------------------------------------------------
>
> (Updated May 30, 2013, 5:59 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> - This implementation uses Boost::rational<uint64_t> to store both the bits and the nanoseconds as integers to preserve precision.
> - It requires the boost lib to be repackaged to include Boost::rational.
> - The usage 'rational' handles avoid overflow cases better (it doesn't) than simply multiply the denominators in various arithmetic operations.
> - Bandwidth always >= 0
> - Bandwidth constructor passes 'boost::bad_rational' up when the denominator is zero.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 7a9ede62145e3150f7af6675d4384feafd9c0a88
> 3rdparty/libprocess/3rdparty/boost-1.53.0.tar.gz 770d837aaba23d031b04ad77658f339587174aae
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 84062a0e1dfe4ec04bac7cac5ebaac4b945eb66e
> 3rdparty/libprocess/3rdparty/stout/include/stout/bandwidth.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/bandwidth_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11542/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>