You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Paul Brett <pa...@twopensource.com> on 2015/06/01 18:40:56 UTC
Review Request 34894: Add new message for Traffic Control statistics
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/
-----------------------------------------------------------
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Repository: mesos
Description
-------
Add new message for Traffic Control statistics
Diffs
-----
include/mesos/mesos.proto 5cf81e20bc77288eb4c183722c608827b5000cb4
Diff: https://reviews.apache.org/r/34894/diff/
Testing
-------
make check
Thanks,
Paul Brett
Re: Review Request 34894: Add new message for Traffic Control
statistics
Posted by Paul Brett <pa...@twopensource.com>.
> On June 1, 2015, 7:17 p.m., Ian Downes wrote:
> > include/mesos/mesos.proto, line 529
> > <https://reviews.apache.org/r/34894/diff/1/?file=975745#file975745line529>
> >
> > This page doesn't seem to document these statistics. Is there a more relevant page?
Only slightly more relevant (http://www.lartc.org/lartc.html). The data structure that carries this information is common between qdiscs but there is no written guideline on when to increment each counter beyond reading the existing qdiscs and copying their behaviour.
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/#review86048
-----------------------------------------------------------
On June 2, 2015, 8:52 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 8:52 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add new message for Traffic Control statistics
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 5cf81e20bc77288eb4c183722c608827b5000cb4
>
> Diff: https://reviews.apache.org/r/34894/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34894: Add new message for Traffic Control
statistics
Posted by Paul Brett <pa...@twopensource.com>.
> On June 1, 2015, 7:17 p.m., Ian Downes wrote:
> > include/mesos/mesos.proto, lines 543-545
> > <https://reviews.apache.org/r/34894/diff/1/?file=975745#file975745line543>
> >
> > What's possible here: can more than one be present or is it only any one? What's the difference between these?
> >
> > If it's only one then why not flatten this and include a type?
All three can (and normally will) be present.
> On June 1, 2015, 7:17 p.m., Ian Downes wrote:
> > include/mesos/mesos.proto, lines 538-539
> > <https://reviews.apache.org/r/34894/diff/1/?file=975745#file975745line538>
> >
> > rate_bps? or is this how it's named in the kernel?
Thats the kernel name.
> On June 1, 2015, 7:17 p.m., Ian Downes wrote:
> > include/mesos/mesos.proto, line 537
> > <https://reviews.apache.org/r/34894/diff/1/?file=975745#file975745line537>
> >
> > queue_length?
Thats the kernel name.
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/#review86048
-----------------------------------------------------------
On June 1, 2015, 4:40 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 4:40 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add new message for Traffic Control statistics
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 5cf81e20bc77288eb4c183722c608827b5000cb4
>
> Diff: https://reviews.apache.org/r/34894/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34894: Add new message for Traffic Control
statistics
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/#review86048
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/34894/#comment137909>
This page doesn't seem to document these statistics. Is there a more relevant page?
include/mesos/mesos.proto
<https://reviews.apache.org/r/34894/#comment137912>
queue_length?
include/mesos/mesos.proto
<https://reviews.apache.org/r/34894/#comment137911>
rate_bps? or is this how it's named in the kernel?
include/mesos/mesos.proto
<https://reviews.apache.org/r/34894/#comment137907>
What's possible here: can more than one be present or is it only any one? What's the difference between these?
If it's only one then why not flatten this and include a type?
include/mesos/mesos.proto
<https://reviews.apache.org/r/34894/#comment137908>
This shouldn't be named including isolator. If the previously flatten is possible then perhaps NetworkControlStatistics?
- Ian Downes
On June 1, 2015, 9:40 a.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 9:40 a.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add new message for Traffic Control statistics
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 5cf81e20bc77288eb4c183722c608827b5000cb4
>
> Diff: https://reviews.apache.org/r/34894/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34894: Add new message for Traffic Control
statistics
Posted by Paul Brett <pa...@twopensource.com>.
> On June 1, 2015, 6:32 p.m., Chi Zhang wrote:
> > include/mesos/mesos.proto, lines 543-545
> > <https://reviews.apache.org/r/34894/diff/1/?file=975745#file975745line543>
> >
> > Maybe some comments for the three names? (kinda the reason you are not using 'repeated' of TrafficControlStatistics in ResourceStatistics?)
Having talk this over, we will convert this to a repeated structure with id fields which are defined in the port mapping isolator (disk volume reporting will need to use a similar structure).
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/#review86033
-----------------------------------------------------------
On June 1, 2015, 4:40 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 4:40 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add new message for Traffic Control statistics
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 5cf81e20bc77288eb4c183722c608827b5000cb4
>
> Diff: https://reviews.apache.org/r/34894/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34894: Add new message for Traffic Control
statistics
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/#review86033
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/34894/#comment137881>
two? lines in between :)
include/mesos/mesos.proto
<https://reviews.apache.org/r/34894/#comment137883>
Maybe some comments for the three names? (kinda the reason you are not using 'repeated' of TrafficControlStatistics in ResourceStatistics?)
- Chi Zhang
On June 1, 2015, 4:40 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 4:40 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add new message for Traffic Control statistics
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 5cf81e20bc77288eb4c183722c608827b5000cb4
>
> Diff: https://reviews.apache.org/r/34894/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34894: Add new message for Traffic Control
statistics
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/#review86042
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/34894/#comment137894>
Per discussion, the name 'fair_queueing' is too confusing, please either rename or remove it.
- Cong Wang
On June 1, 2015, 4:40 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 4:40 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add new message for Traffic Control statistics
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 5cf81e20bc77288eb4c183722c608827b5000cb4
>
> Diff: https://reviews.apache.org/r/34894/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34894: Add new message for Traffic Control
statistics
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/#review86013
-----------------------------------------------------------
Patch looks great!
Reviews applied: [34830, 34832, 34426, 34863, 34894]
All tests passed.
- Mesos ReviewBot
On June 1, 2015, 4:40 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 4:40 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add new message for Traffic Control statistics
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 5cf81e20bc77288eb4c183722c608827b5000cb4
>
> Diff: https://reviews.apache.org/r/34894/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34894: Add new message for Traffic Control
statistics
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/#review86287
-----------------------------------------------------------
Bad patch!
Reviews applied: [34899, 34830, 34832, 34426]
Failed command: ./support/apply-review.sh -n -r 34426
Error:
2015-06-02 21:24:30 URL:https://reviews.apache.org/r/34426/diff/raw/ [9373/9373] -> "34426.patch" [1]
error: patch failed: src/linux/routing/queueing/fq_codel.cpp:97
error: src/linux/routing/queueing/fq_codel.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On June 2, 2015, 8:52 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 8:52 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add new message for Traffic Control statistics
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 5cf81e20bc77288eb4c183722c608827b5000cb4
>
> Diff: https://reviews.apache.org/r/34894/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34894: Add new message for Traffic Control
statistics
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/#review87062
-----------------------------------------------------------
Ship it!
Ship It!
- Ian Downes
On June 3, 2015, 2:54 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> -----------------------------------------------------------
>
> (Updated June 3, 2015, 2:54 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add new message for Traffic Control statistics
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 6186c92c6fe9cf8fa136d93d5af43e0377406baf
>
> Diff: https://reviews.apache.org/r/34894/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34894: Add new message for Traffic Control
statistics
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/#review86503
-----------------------------------------------------------
Patch looks great!
Reviews applied: [34894]
All tests passed.
- Mesos ReviewBot
On June 3, 2015, 9:54 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> -----------------------------------------------------------
>
> (Updated June 3, 2015, 9:54 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add new message for Traffic Control statistics
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 6186c92c6fe9cf8fa136d93d5af43e0377406baf
>
> Diff: https://reviews.apache.org/r/34894/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34894: Add new message for Traffic Control
statistics
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/#review86860
-----------------------------------------------------------
Ship it!
LGTM. I'll wait for Ian's shipit.
- Jie Yu
On June 3, 2015, 9:54 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> -----------------------------------------------------------
>
> (Updated June 3, 2015, 9:54 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add new message for Traffic Control statistics
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 6186c92c6fe9cf8fa136d93d5af43e0377406baf
>
> Diff: https://reviews.apache.org/r/34894/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34894: Add new message for Traffic Control
statistics
Posted by Jie Yu <yu...@gmail.com>.
> On June 6, 2015, 12:10 a.m., Ian Downes wrote:
> > include/mesos/mesos.proto, line 533
> > <https://reviews.apache.org/r/34894/diff/3/?file=977694#file977694line533>
> >
> > What's the set of possible ids? If it's the well-defined set that we discussed offline then we should probably enumerate them? @jieyu, thoughts? Otherwise we'll be string matching on id. For example (with not very good naming):
> > ```
> > message TrafficControlStatistics {
> > enum Type {
> > tx_internal = 1;
> > tx_external = 2;
> > rx_internal = 3;
> > rx_external = 4;
> > }
> > required Type type = 1;
> > optional ...
> > }
> > ```
Ian, I think it's hard to enumerate eventually. What if in the future, we want to change the qdisc hierarchy to be more sophisticated (e.g., more intermediate filters/qdiscs), and we want to get statistics for all of them? I just feel that it'll be very hard to enumerate.
Also, disk usage for volumes, we can not enumerate anyway.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/#review86875
-----------------------------------------------------------
On June 3, 2015, 9:54 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> -----------------------------------------------------------
>
> (Updated June 3, 2015, 9:54 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add new message for Traffic Control statistics
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 6186c92c6fe9cf8fa136d93d5af43e0377406baf
>
> Diff: https://reviews.apache.org/r/34894/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34894: Add new message for Traffic Control
statistics
Posted by Paul Brett <pa...@twopensource.com>.
> On June 6, 2015, 12:10 a.m., Ian Downes wrote:
> > include/mesos/mesos.proto, line 533
> > <https://reviews.apache.org/r/34894/diff/3/?file=977694#file977694line533>
> >
> > What's the set of possible ids? If it's the well-defined set that we discussed offline then we should probably enumerate them? @jieyu, thoughts? Otherwise we'll be string matching on id. For example (with not very good naming):
> > ```
> > message TrafficControlStatistics {
> > enum Type {
> > tx_internal = 1;
> > tx_external = 2;
> > rx_internal = 3;
> > rx_external = 4;
> > }
> > required Type type = 1;
> > optional ...
> > }
> > ```
>
> Jie Yu wrote:
> Ian, I think it's hard to enumerate eventually. What if in the future, we want to change the qdisc hierarchy to be more sophisticated (e.g., more intermediate filters/qdiscs), and we want to get statistics for all of them? I just feel that it'll be very hard to enumerate.
>
> Also, disk usage for volumes, we can not enumerate anyway.
In addition, there are two separate qdiscs on tx_internal at the moment that need to be reported separatly.
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/#review86875
-----------------------------------------------------------
On June 3, 2015, 9:54 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> -----------------------------------------------------------
>
> (Updated June 3, 2015, 9:54 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add new message for Traffic Control statistics
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 6186c92c6fe9cf8fa136d93d5af43e0377406baf
>
> Diff: https://reviews.apache.org/r/34894/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34894: Add new message for Traffic Control
statistics
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/#review86875
-----------------------------------------------------------
Much cleaner! Just a question about enumerating the ids/types.
include/mesos/mesos.proto
<https://reviews.apache.org/r/34894/#comment139007>
What's the set of possible ids? If it's the well-defined set that we discussed offline then we should probably enumerate them? @jieyu, thoughts? Otherwise we'll be string matching on id. For example (with not very good naming):
```
message TrafficControlStatistics {
enum Type {
tx_internal = 1;
tx_external = 2;
rx_internal = 3;
rx_external = 4;
}
required Type type = 1;
optional ...
}
```
- Ian Downes
On June 3, 2015, 2:54 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> -----------------------------------------------------------
>
> (Updated June 3, 2015, 2:54 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add new message for Traffic Control statistics
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 6186c92c6fe9cf8fa136d93d5af43e0377406baf
>
> Diff: https://reviews.apache.org/r/34894/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34894: Add new message for Traffic Control
statistics
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/
-----------------------------------------------------------
(Updated June 3, 2015, 9:54 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Changes
-------
Incorporate review feedback.
Repository: mesos
Description
-------
Add new message for Traffic Control statistics
Diffs (updated)
-----
include/mesos/mesos.proto 6186c92c6fe9cf8fa136d93d5af43e0377406baf
Diff: https://reviews.apache.org/r/34894/diff/
Testing
-------
make check
Thanks,
Paul Brett
Re: Review Request 34894: Add new message for Traffic Control
statistics
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34894/
-----------------------------------------------------------
(Updated June 2, 2015, 8:52 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Changes
-------
Switch to returning statistics as an array
Repository: mesos
Description
-------
Add new message for Traffic Control statistics
Diffs (updated)
-----
include/mesos/mesos.proto 5cf81e20bc77288eb4c183722c608827b5000cb4
Diff: https://reviews.apache.org/r/34894/diff/
Testing
-------
make check
Thanks,
Paul Brett