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/04/24 19:46:40 UTC
Re: Review Request 33040: Expose qdisc statistics from libnl
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33040/
-----------------------------------------------------------
(Updated April 24, 2015, 5:46 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang.
Changes
-------
Bug fixes to libnl wrapper, address reviewers comments
Bugs: mesos-2332
https://issues.apache.org/jira/browse/mesos-2332
Repository: mesos
Description
-------
Expose qdisc statistics from libnl
Diffs (updated)
-----
src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
src/linux/routing/queueing/fq_codel.hpp 4f67ab7d64afea96a07dfcf36769a9c667749a00
src/linux/routing/queueing/fq_codel.cpp 02ad8df7814c0e549a9ca9aef39777684e6abdcb
src/linux/routing/queueing/handle.hpp 2725d0794ca29ad5dc1b0148d0f68b90ce8b8369
src/linux/routing/queueing/ingress.hpp b323a7f6daed828327d6d9e9740df81582e0ba2b
src/linux/routing/queueing/ingress.cpp 47c73376097d70819defdee31a6d1e446df6b8ba
src/linux/routing/queueing/internal.hpp 7c6c4d3d960b9a4bf44dcf482212317522353d69
src/tests/routing_tests.cpp 7cc3b57a3b71544874557d2b1cf88a241b7062ba
Diff: https://reviews.apache.org/r/33040/diff/
Testing
-------
make check
Thanks,
Paul Brett
Re: Review Request 33040: Expose qdisc statistics from libnl
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33040/#review81505
-----------------------------------------------------------
Bad patch!
Reviews applied: [33040]
Failed command: make -j3 distcheck
Error:
make dist-gzip am__post_remove_distdir='@:'
make[1]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot'
if test -d "mesos-0.23.0"; then find "mesos-0.23.0" -type d ! -perm -200 -exec chmod u+w {} ';' && rm -rf "mesos-0.23.0" || { sleep 5 && rm -rf "mesos-0.23.0"; }; else :; fi
test -d "mesos-0.23.0" || mkdir "mesos-0.23.0"
(cd 3rdparty && make top_distdir=../mesos-0.23.0 distdir=../mesos-0.23.0/3rdparty \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[2]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
(cd libprocess && make top_distdir=../../mesos-0.23.0 distdir=../../mesos-0.23.0/3rdparty/libprocess \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[3]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
:
test -d "../../mesos-0.23.0/3rdparty/libprocess" || mkdir "../../mesos-0.23.0/3rdparty/libprocess"
(cd 3rdparty && make top_distdir=../../../mesos-0.23.0 distdir=../../../mesos-0.23.0/3rdparty/libprocess/3rdparty \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[4]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty'
(cd stout && make top_distdir=../../../../mesos-0.23.0 distdir=../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[5]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout'
:
test -d "../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout" || mkdir "../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout"
(cd include && make top_distdir=../../../../../mesos-0.23.0 distdir=../../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout/include \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[6]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/include'
make[6]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/include'
test -n ":" \
|| find "../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout" -type d ! -perm -755 \
-exec chmod u+rwx,go+rx {} \; -o \
! -type d ! -perm -444 -links 1 -exec chmod a+r {} \; -o \
! -type d ! -perm -400 -exec chmod a+r {} \; -o \
! -type d ! -perm -444 -exec /bin/bash /home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/install-sh -c -m a+r {} {} \; \
|| chmod -R a+r "../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout"
make[5]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout'
make[4]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty'
(cd include && make top_distdir=../../../mesos-0.23.0 distdir=../../../mesos-0.23.0/3rdparty/libprocess/include \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[4]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/include'
make[4]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/include'
test -n ":" \
|| find "../../mesos-0.23.0/3rdparty/libprocess" -type d ! -perm -755 \
-exec chmod u+rwx,go+rx {} \; -o \
! -type d ! -perm -444 -links 1 -exec chmod a+r {} \; -o \
! -type d ! -perm -400 -exec chmod a+r {} \; -o \
! -type d ! -perm -444 -exec /bin/bash /home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/install-sh -c -m a+r {} {} \; \
|| chmod -R a+r "../../mesos-0.23.0/3rdparty/libprocess"
make[3]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
make[2]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
(cd src && make top_distdir=../mesos-0.23.0 distdir=../mesos-0.23.0/src \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[2]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/src'
make[2]: *** No rule to make target `linux/routing/queueing/statistics.cpp', needed by `distdir'. Stop.
make[2]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/src'
make[1]: *** [distdir] Error 1
make[1]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot'
make: *** [dist] Error 2
- Mesos ReviewBot
On April 24, 2015, 5:46 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33040/
> -----------------------------------------------------------
>
> (Updated April 24, 2015, 5:46 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: mesos-2332
> https://issues.apache.org/jira/browse/mesos-2332
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Expose qdisc statistics from libnl
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/linux/routing/queueing/fq_codel.hpp 4f67ab7d64afea96a07dfcf36769a9c667749a00
> src/linux/routing/queueing/fq_codel.cpp 02ad8df7814c0e549a9ca9aef39777684e6abdcb
> src/linux/routing/queueing/handle.hpp 2725d0794ca29ad5dc1b0148d0f68b90ce8b8369
> src/linux/routing/queueing/ingress.hpp b323a7f6daed828327d6d9e9740df81582e0ba2b
> src/linux/routing/queueing/ingress.cpp 47c73376097d70819defdee31a6d1e446df6b8ba
> src/linux/routing/queueing/internal.hpp 7c6c4d3d960b9a4bf44dcf482212317522353d69
> src/tests/routing_tests.cpp 7cc3b57a3b71544874557d2b1cf88a241b7062ba
>
> Diff: https://reviews.apache.org/r/33040/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 33040: Expose qdisc statistics from libnl
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33040/#review81548
-----------------------------------------------------------
src/linux/routing/queueing/statistics.cpp
<https://reviews.apache.org/r/33040/#comment131944>
Should be 'parent' not 'handle' since you search by parent.
- Cong Wang
On April 24, 2015, 7:28 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33040/
> -----------------------------------------------------------
>
> (Updated April 24, 2015, 7:28 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: mesos-2332
> https://issues.apache.org/jira/browse/mesos-2332
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Expose qdisc statistics from libnl
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a
> src/linux/routing/queueing/fq_codel.cpp 02ad8df
> src/linux/routing/queueing/ingress.cpp 47c7337
> src/linux/routing/queueing/internal.hpp 7c6c4d3
> src/linux/routing/queueing/statistics.hpp PRE-CREATION
> src/linux/routing/queueing/statistics.cpp PRE-CREATION
> src/tests/routing_tests.cpp 7cc3b57
>
> Diff: https://reviews.apache.org/r/33040/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 33040: Expose qdisc statistics from libnl
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33040/#review81549
-----------------------------------------------------------
src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/33040/#comment131945>
This fixes a real bug, should be a separated patch.
- Cong Wang
On April 24, 2015, 7:28 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33040/
> -----------------------------------------------------------
>
> (Updated April 24, 2015, 7:28 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: mesos-2332
> https://issues.apache.org/jira/browse/mesos-2332
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Expose qdisc statistics from libnl
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a
> src/linux/routing/queueing/fq_codel.cpp 02ad8df
> src/linux/routing/queueing/ingress.cpp 47c7337
> src/linux/routing/queueing/internal.hpp 7c6c4d3
> src/linux/routing/queueing/statistics.hpp PRE-CREATION
> src/linux/routing/queueing/statistics.cpp PRE-CREATION
> src/tests/routing_tests.cpp 7cc3b57
>
> Diff: https://reviews.apache.org/r/33040/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 33040: Expose qdisc statistics from libnl
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33040/#review81547
-----------------------------------------------------------
src/linux/routing/queueing/statistics.cpp
<https://reviews.apache.org/r/33040/#comment131943>
tcName is confusing, tcStatsName is better.
- Cong Wang
On April 24, 2015, 7:28 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33040/
> -----------------------------------------------------------
>
> (Updated April 24, 2015, 7:28 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: mesos-2332
> https://issues.apache.org/jira/browse/mesos-2332
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Expose qdisc statistics from libnl
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a
> src/linux/routing/queueing/fq_codel.cpp 02ad8df
> src/linux/routing/queueing/ingress.cpp 47c7337
> src/linux/routing/queueing/internal.hpp 7c6c4d3
> src/linux/routing/queueing/statistics.hpp PRE-CREATION
> src/linux/routing/queueing/statistics.cpp PRE-CREATION
> src/tests/routing_tests.cpp 7cc3b57
>
> Diff: https://reviews.apache.org/r/33040/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 33040: Expose qdisc statistics from libnl
Posted by Paul Brett <pa...@twopensource.com>.
> On April 24, 2015, 10:53 p.m., Chi Zhang wrote:
> > src/linux/routing/queueing/statistics.cpp, line 99
> > <https://reviews.apache.org/r/33040/diff/8/?file=941228#file941228line99>
> >
> > not sure if auto is whitelisted, as much as I'd like to embrace it.
> >
> > @jieyu could you comment on this?
Use of auto is ok.
> On April 24, 2015, 10:53 p.m., Chi Zhang wrote:
> > src/linux/routing/queueing/statistics.hpp, line 27
> > <https://reviews.apache.org/r/33040/diff/8/?file=941227#file941227line27>
> >
> > handle.hpp is only used in the implementation?
No, I expose handle as this is common between filters and disciplines
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33040/#review81563
-----------------------------------------------------------
On April 24, 2015, 7:28 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33040/
> -----------------------------------------------------------
>
> (Updated April 24, 2015, 7:28 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: mesos-2332
> https://issues.apache.org/jira/browse/mesos-2332
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Expose qdisc statistics from libnl
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a
> src/linux/routing/queueing/fq_codel.cpp 02ad8df
> src/linux/routing/queueing/ingress.cpp 47c7337
> src/linux/routing/queueing/internal.hpp 7c6c4d3
> src/linux/routing/queueing/statistics.hpp PRE-CREATION
> src/linux/routing/queueing/statistics.cpp PRE-CREATION
> src/tests/routing_tests.cpp 7cc3b57
>
> Diff: https://reviews.apache.org/r/33040/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 33040: Expose qdisc statistics from libnl
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33040/#review81563
-----------------------------------------------------------
src/linux/routing/queueing/statistics.hpp
<https://reviews.apache.org/r/33040/#comment131984>
handle.hpp is only used in the implementation?
src/linux/routing/queueing/statistics.cpp
<https://reviews.apache.org/r/33040/#comment131981>
not sure if auto is whitelisted, as much as I'd like to embrace it.
@jieyu could you comment on this?
src/linux/routing/queueing/statistics.cpp
<https://reviews.apache.org/r/33040/#comment131982>
nit: I don't think the style guide likes abbr. names such as ptr. Could you adjust?
src/linux/routing/queueing/statistics.cpp
<https://reviews.apache.org/r/33040/#comment131979>
Every single use of i is casted before used, wondering if you pick a different type for i, could you reduce some casting here. also, I think the c++ styple casts are preferred.
src/linux/routing/queueing/statistics.cpp
<https://reviews.apache.org/r/33040/#comment131978>
nit: const string name
src/linux/routing/queueing/statistics.cpp
<https://reviews.apache.org/r/33040/#comment131983>
should we ignore if tcName returns a ""?
src/linux/routing/queueing/statistics.cpp
<https://reviews.apache.org/r/33040/#comment131977>
nit: kill the whitespace in front of the lines?
src/linux/routing/queueing/statistics.cpp
<https://reviews.apache.org/r/33040/#comment131976>
nit: I thought we stopped using non-pod static types anymore?
src/linux/routing/queueing/statistics.cpp
<https://reviews.apache.org/r/33040/#comment131980>
nit: just s/std:string/string/g since you have the 'using std::string;' above?
- Chi Zhang
On April 24, 2015, 7:28 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33040/
> -----------------------------------------------------------
>
> (Updated April 24, 2015, 7:28 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: mesos-2332
> https://issues.apache.org/jira/browse/mesos-2332
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Expose qdisc statistics from libnl
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a
> src/linux/routing/queueing/fq_codel.cpp 02ad8df
> src/linux/routing/queueing/ingress.cpp 47c7337
> src/linux/routing/queueing/internal.hpp 7c6c4d3
> src/linux/routing/queueing/statistics.hpp PRE-CREATION
> src/linux/routing/queueing/statistics.cpp PRE-CREATION
> src/tests/routing_tests.cpp 7cc3b57
>
> Diff: https://reviews.apache.org/r/33040/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 33040: Expose qdisc statistics from libnl
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33040/#review81546
-----------------------------------------------------------
Patch looks great!
Reviews applied: [33040]
All tests passed.
- Mesos ReviewBot
On April 24, 2015, 7:28 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33040/
> -----------------------------------------------------------
>
> (Updated April 24, 2015, 7:28 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: mesos-2332
> https://issues.apache.org/jira/browse/mesos-2332
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Expose qdisc statistics from libnl
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a
> src/linux/routing/queueing/fq_codel.cpp 02ad8df
> src/linux/routing/queueing/ingress.cpp 47c7337
> src/linux/routing/queueing/internal.hpp 7c6c4d3
> src/linux/routing/queueing/statistics.hpp PRE-CREATION
> src/linux/routing/queueing/statistics.cpp PRE-CREATION
> src/tests/routing_tests.cpp 7cc3b57
>
> Diff: https://reviews.apache.org/r/33040/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 33040: Expose qdisc statistics from libnl
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33040/
-----------------------------------------------------------
(Updated April 24, 2015, 7:28 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang.
Changes
-------
Address reviewer comments (two still outstanding)
Bugs: mesos-2332
https://issues.apache.org/jira/browse/mesos-2332
Repository: mesos
Description
-------
Expose qdisc statistics from libnl
Diffs (updated)
-----
src/Makefile.am 93c7c8a
src/linux/routing/queueing/fq_codel.cpp 02ad8df
src/linux/routing/queueing/ingress.cpp 47c7337
src/linux/routing/queueing/internal.hpp 7c6c4d3
src/linux/routing/queueing/statistics.hpp PRE-CREATION
src/linux/routing/queueing/statistics.cpp PRE-CREATION
src/tests/routing_tests.cpp 7cc3b57
Diff: https://reviews.apache.org/r/33040/diff/
Testing
-------
make check
Thanks,
Paul Brett
Re: Review Request 33040: Expose qdisc statistics from libnl
Posted by Jie Yu <yu...@gmail.com>.
> On April 24, 2015, 6:09 p.m., Jie Yu wrote:
> > src/linux/routing/queueing/internal.hpp, lines 154-160
> > <https://reviews.apache.org/r/33040/diff/7/?file=941075#file941075line154>
> >
> > Why this change? What if in the future we have two versions of fq_codel qdiscs. Only using the name to distinguish seems to be insufficient. Could you please revert this change?
>
> Paul Brett wrote:
> The old method of using the handle to find the qdisc does not work in practice. The hard coded handles specified in the disciplines are not used for fq_codel (or htb for which we do not track the handle), but searching from the root by name always works because the first discipline of the correct type we find will have the statistics.
Are you refering to the case where there's already a root qdisc installed on the host and it's handle does not match the hard coded handle we use?
We can still keep the decode function here. You just need to tune the decode function for fq_codel qdisc (like removing the handle equality check).
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33040/#review81503
-----------------------------------------------------------
On April 24, 2015, 5:46 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33040/
> -----------------------------------------------------------
>
> (Updated April 24, 2015, 5:46 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: mesos-2332
> https://issues.apache.org/jira/browse/mesos-2332
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Expose qdisc statistics from libnl
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/linux/routing/queueing/fq_codel.hpp 4f67ab7d64afea96a07dfcf36769a9c667749a00
> src/linux/routing/queueing/fq_codel.cpp 02ad8df7814c0e549a9ca9aef39777684e6abdcb
> src/linux/routing/queueing/handle.hpp 2725d0794ca29ad5dc1b0148d0f68b90ce8b8369
> src/linux/routing/queueing/ingress.hpp b323a7f6daed828327d6d9e9740df81582e0ba2b
> src/linux/routing/queueing/ingress.cpp 47c73376097d70819defdee31a6d1e446df6b8ba
> src/linux/routing/queueing/internal.hpp 7c6c4d3d960b9a4bf44dcf482212317522353d69
> src/tests/routing_tests.cpp 7cc3b57a3b71544874557d2b1cf88a241b7062ba
>
> Diff: https://reviews.apache.org/r/33040/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 33040: Expose qdisc statistics from libnl
Posted by Paul Brett <pa...@twopensource.com>.
> On April 24, 2015, 6:09 p.m., Jie Yu wrote:
> > src/linux/routing/queueing/internal.hpp, lines 154-160
> > <https://reviews.apache.org/r/33040/diff/7/?file=941075#file941075line154>
> >
> > Why this change? What if in the future we have two versions of fq_codel qdiscs. Only using the name to distinguish seems to be insufficient. Could you please revert this change?
The old method of using the handle to find the qdisc does not work in practice. The hard coded handles specified in the disciplines are not used for fq_codel (or htb for which we do not track the handle), but searching from the root by name always works because the first discipline of the correct type we find will have the statistics.
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33040/#review81503
-----------------------------------------------------------
On April 24, 2015, 5:46 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33040/
> -----------------------------------------------------------
>
> (Updated April 24, 2015, 5:46 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: mesos-2332
> https://issues.apache.org/jira/browse/mesos-2332
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Expose qdisc statistics from libnl
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/linux/routing/queueing/fq_codel.hpp 4f67ab7d64afea96a07dfcf36769a9c667749a00
> src/linux/routing/queueing/fq_codel.cpp 02ad8df7814c0e549a9ca9aef39777684e6abdcb
> src/linux/routing/queueing/handle.hpp 2725d0794ca29ad5dc1b0148d0f68b90ce8b8369
> src/linux/routing/queueing/ingress.hpp b323a7f6daed828327d6d9e9740df81582e0ba2b
> src/linux/routing/queueing/ingress.cpp 47c73376097d70819defdee31a6d1e446df6b8ba
> src/linux/routing/queueing/internal.hpp 7c6c4d3d960b9a4bf44dcf482212317522353d69
> src/tests/routing_tests.cpp 7cc3b57a3b71544874557d2b1cf88a241b7062ba
>
> Diff: https://reviews.apache.org/r/33040/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 33040: Expose qdisc statistics from libnl
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33040/#review81503
-----------------------------------------------------------
src/Makefile.am
<https://reviews.apache.org/r/33040/#comment131872>
I don't see this file?
src/linux/routing/queueing/fq_codel.hpp
<https://reviews.apache.org/r/33040/#comment131871>
I don't see any code in the header uses hashmap?
src/linux/routing/queueing/fq_codel.hpp
<https://reviews.apache.org/r/33040/#comment131870>
WHy add one more blank line? Please revert.
src/linux/routing/queueing/fq_codel.cpp
<https://reviews.apache.org/r/33040/#comment131875>
Please move this above `operator ==` and add a new line between them.
src/linux/routing/queueing/fq_codel.cpp
<https://reviews.apache.org/r/33040/#comment131869>
Why add one more blank line?
src/linux/routing/queueing/handle.hpp
<https://reviews.apache.org/r/33040/#comment131868>
Why remove this blank line?
src/linux/routing/queueing/ingress.hpp
<https://reviews.apache.org/r/33040/#comment131874>
I don't see hashmap being used?
src/linux/routing/queueing/ingress.hpp
<https://reviews.apache.org/r/33040/#comment131873>
Please revert.
src/linux/routing/queueing/ingress.cpp
<https://reviews.apache.org/r/33040/#comment131877>
Ditto.
src/linux/routing/queueing/ingress.cpp
<https://reviews.apache.org/r/33040/#comment131878>
To be consistent, do you want to get rid of the "ingress" here as well. You can make 'name' a static method of Discipline and use `ingress::Discipline::name()` consistently.
src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/33040/#comment131880>
Thanks! Could you please add a test for this?
src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/33040/#comment131879>
s/if/If/
Also, add a period at the end of the sentense.
src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/33040/#comment131883>
Please revert.
src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/33040/#comment131885>
Why this change? What if in the future we have two versions of fq_codel qdiscs. Only using the name to distinguish seems to be insufficient. Could you please revert this change?
src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/33040/#comment131881>
Please revert.
src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/33040/#comment131882>
Please revert as well.
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/33040/#comment131886>
Add a period at the end of the sentense.
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/33040/#comment131887>
Could you please remove these prints?
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/33040/#comment131888>
Ditto on removal.
- Jie Yu
On April 24, 2015, 5:46 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33040/
> -----------------------------------------------------------
>
> (Updated April 24, 2015, 5:46 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: mesos-2332
> https://issues.apache.org/jira/browse/mesos-2332
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Expose qdisc statistics from libnl
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/linux/routing/queueing/fq_codel.hpp 4f67ab7d64afea96a07dfcf36769a9c667749a00
> src/linux/routing/queueing/fq_codel.cpp 02ad8df7814c0e549a9ca9aef39777684e6abdcb
> src/linux/routing/queueing/handle.hpp 2725d0794ca29ad5dc1b0148d0f68b90ce8b8369
> src/linux/routing/queueing/ingress.hpp b323a7f6daed828327d6d9e9740df81582e0ba2b
> src/linux/routing/queueing/ingress.cpp 47c73376097d70819defdee31a6d1e446df6b8ba
> src/linux/routing/queueing/internal.hpp 7c6c4d3d960b9a4bf44dcf482212317522353d69
> src/tests/routing_tests.cpp 7cc3b57a3b71544874557d2b1cf88a241b7062ba
>
> Diff: https://reviews.apache.org/r/33040/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>