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/05/29 23:57:30 UTC
Review Request 34830: Fix routing ingress/fq_codel search returning
wrong qdisc.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34830/
-----------------------------------------------------------
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: MESOS-2781
https://issues.apache.org/jira/browse/MESOS-2781
Repository: mesos
Description
-------
Fix routing ingress/fq_codel search returning wrong qdisc.
Add new checking to verify that operations occur on the correct interface and change the behaviour of create() operation to return false rather than an error if creation fails if the interface does not exist (now consistent with exists() and remove()).
Diffs
-----
src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900
src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc
src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed
src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699
src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70
Diff: https://reviews.apache.org/r/34830/diff/
Testing
-------
make check
Thanks,
Paul Brett
Re: Review Request 34830: Fix routing ingress/fq_codel search
returning wrong qdisc.
Posted by Jie Yu <yu...@gmail.com>.
> On June 1, 2015, 5:59 p.m., Chi Zhang wrote:
> > src/linux/routing/queueing/ingress.hpp, lines 55-56
> > <https://reviews.apache.org/r/34830/diff/2/?file=975052#file975052line55>
> >
> > a bit confused by "an ingress qdisc on the egress side of the link"
> >
> > I saw this in other places too.
+1
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34830/#review86006
-----------------------------------------------------------
On May 30, 2015, 9 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34830/
> -----------------------------------------------------------
>
> (Updated May 30, 2015, 9 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2781
> https://issues.apache.org/jira/browse/MESOS-2781
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix routing ingress/fq_codel search returning wrong qdisc.
>
> Add new checking to verify that operations occur on the correct interface and change the behaviour of create() operation to return false rather than an error if creation fails if the interface does not exist (now consistent with exists() and remove()).
>
>
> Diffs
> -----
>
> src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900
> src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc
> src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed
> src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699
> src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70
>
> Diff: https://reviews.apache.org/r/34830/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34830: Fix routing ingress/fq_codel search
returning wrong qdisc.
Posted by Chi Zhang <ch...@gmail.com>.
> On June 1, 2015, 5:59 p.m., Chi Zhang wrote:
> > src/linux/routing/queueing/fq_codel.cpp, line 102
> > <https://reviews.apache.org/r/34830/diff/2/?file=975051#file975051line102>
> >
> > ignore if you have done in a different patch:
> >
> > egress::ROOT? (You had ingress::ROOT)
feel free to drop this and do it in a different patch though
> On June 1, 2015, 5:59 p.m., Chi Zhang wrote:
> > src/linux/routing/queueing/ingress.hpp, lines 55-56
> > <https://reviews.apache.org/r/34830/diff/2/?file=975052#file975052line55>
> >
> > a bit confused by "an ingress qdisc on the egress side of the link"
duped.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34830/#review86006
-----------------------------------------------------------
On May 30, 2015, 9 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34830/
> -----------------------------------------------------------
>
> (Updated May 30, 2015, 9 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2781
> https://issues.apache.org/jira/browse/MESOS-2781
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix routing ingress/fq_codel search returning wrong qdisc.
>
> Add new checking to verify that operations occur on the correct interface and change the behaviour of create() operation to return false rather than an error if creation fails if the interface does not exist (now consistent with exists() and remove()).
>
>
> Diffs
> -----
>
> src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900
> src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc
> src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed
> src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699
> src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70
>
> Diff: https://reviews.apache.org/r/34830/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34830: Fix routing ingress/fq_codel search
returning wrong qdisc.
Posted by Jie Yu <yu...@gmail.com>.
> On June 1, 2015, 5:59 p.m., Chi Zhang wrote:
> > src/linux/routing/queueing/internal.hpp, lines 128-131
> > <https://reviews.apache.org/r/34830/diff/2/?file=975054#file975054line128>
> >
> > reinterpret_cast is the preferred c++ way, but I am not sure if the codebase has done similiar casting with this before?.
> >
> > If using this introdue consistency, use another patch to do it all?
+1
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34830/#review86006
-----------------------------------------------------------
On May 30, 2015, 9 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34830/
> -----------------------------------------------------------
>
> (Updated May 30, 2015, 9 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2781
> https://issues.apache.org/jira/browse/MESOS-2781
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix routing ingress/fq_codel search returning wrong qdisc.
>
> Add new checking to verify that operations occur on the correct interface and change the behaviour of create() operation to return false rather than an error if creation fails if the interface does not exist (now consistent with exists() and remove()).
>
>
> Diffs
> -----
>
> src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900
> src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc
> src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed
> src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699
> src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70
>
> Diff: https://reviews.apache.org/r/34830/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34830: Fix routing ingress/fq_codel search
returning wrong qdisc.
Posted by Paul Brett <pa...@twopensource.com>.
> On June 1, 2015, 5:59 p.m., Chi Zhang wrote:
> > src/linux/routing/queueing/fq_codel.cpp, line 102
> > <https://reviews.apache.org/r/34830/diff/2/?file=975051#file975051line102>
> >
> > ignore if you have done in a different patch:
> >
> > egress::ROOT? (You had ingress::ROOT)
>
> Chi Zhang wrote:
> feel free to drop this and do it in a different patch though
Ideally yes, but it is probaly not worth doing until we need more in the egress namespace than just this one symbol.
> On June 1, 2015, 5:59 p.m., Chi Zhang wrote:
> > src/linux/routing/queueing/internal.hpp, lines 128-131
> > <https://reviews.apache.org/r/34830/diff/2/?file=975054#file975054line128>
> >
> > reinterpret_cast is the preferred c++ way, but I am not sure if the codebase has done similiar casting with this before?.
> >
> > If using this introdue consistency, use another patch to do it all?
>
> Jie Yu wrote:
> +1
reinterpret_case is used in stout and twice in mesos (in jvm.hpp and common/thread.cpp). Converting all the C style casts to C++ would be a significant effort.
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34830/#review86006
-----------------------------------------------------------
On May 30, 2015, 9 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34830/
> -----------------------------------------------------------
>
> (Updated May 30, 2015, 9 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2781
> https://issues.apache.org/jira/browse/MESOS-2781
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix routing ingress/fq_codel search returning wrong qdisc.
>
> Add new checking to verify that operations occur on the correct interface and change the behaviour of create() operation to return false rather than an error if creation fails if the interface does not exist (now consistent with exists() and remove()).
>
>
> Diffs
> -----
>
> src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900
> src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc
> src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed
> src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699
> src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70
>
> Diff: https://reviews.apache.org/r/34830/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34830: Fix routing ingress/fq_codel search
returning wrong qdisc.
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On June 1, 2015, 5:59 p.m., Chi Zhang wrote:
> > src/linux/routing/queueing/ingress.cpp, lines 44-51
> > <https://reviews.apache.org/r/34830/diff/2/?file=975053#file975053line44>
> >
> > Two blank lines between the functions.
> >
> > here and other places.
Since the functions are inside a struct, it is common through the code base to add just one blank line.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34830/#review86006
-----------------------------------------------------------
On May 30, 2015, 9 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34830/
> -----------------------------------------------------------
>
> (Updated May 30, 2015, 9 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2781
> https://issues.apache.org/jira/browse/MESOS-2781
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix routing ingress/fq_codel search returning wrong qdisc.
>
> Add new checking to verify that operations occur on the correct interface and change the behaviour of create() operation to return false rather than an error if creation fails if the interface does not exist (now consistent with exists() and remove()).
>
>
> Diffs
> -----
>
> src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900
> src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc
> src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed
> src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699
> src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70
>
> Diff: https://reviews.apache.org/r/34830/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34830: Fix routing ingress/fq_codel search
returning wrong qdisc.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34830/#review86006
-----------------------------------------------------------
src/linux/routing/queueing/fq_codel.cpp
<https://reviews.apache.org/r/34830/#comment137853>
ignore if you have done in a different patch:
egress::ROOT? (You had ingress::ROOT)
src/linux/routing/queueing/ingress.hpp
<https://reviews.apache.org/r/34830/#comment137850>
a bit confused by "an ingress qdisc on the egress side of the link"
src/linux/routing/queueing/ingress.hpp
<https://reviews.apache.org/r/34830/#comment137851>
a bit confused by "an ingress qdisc on the egress side of the link"
I saw this in other places too.
src/linux/routing/queueing/ingress.cpp
<https://reviews.apache.org/r/34830/#comment137845>
Two blank lines between the functions.
here and other places.
src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/34830/#comment137855>
reinterpret_cast is the preferred c++ way, but I am not sure if the codebase has done similiar casting with this before?.
If using this introdue consistency, use another patch to do it all?
src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/34830/#comment137844>
Fix the space before "This"
- Chi Zhang
On May 30, 2015, 9 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34830/
> -----------------------------------------------------------
>
> (Updated May 30, 2015, 9 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2781
> https://issues.apache.org/jira/browse/MESOS-2781
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix routing ingress/fq_codel search returning wrong qdisc.
>
> Add new checking to verify that operations occur on the correct interface and change the behaviour of create() operation to return false rather than an error if creation fails if the interface does not exist (now consistent with exists() and remove()).
>
>
> Diffs
> -----
>
> src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900
> src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc
> src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed
> src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699
> src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70
>
> Diff: https://reviews.apache.org/r/34830/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34830: Fix routing ingress/fq_codel search
returning wrong qdisc.
Posted by Paul Brett <pa...@twopensource.com>.
> On June 1, 2015, 6:33 p.m., Jie Yu wrote:
> > src/linux/routing/queueing/internal.hpp, line 207
> > <https://reviews.apache.org/r/34830/diff/2/?file=975054#file975054line207>
> >
> > OK, this is confusing to me. We return false in the following two cases:
> >
> > 1) link does not exist
> > 2) qdisc already exists
> >
> > Is that expected?
Yes, that it what I was proposing, Error() are reserved for catch abnormal situations which is consistent with the other operations.
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34830/#review86029
-----------------------------------------------------------
On May 30, 2015, 9 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34830/
> -----------------------------------------------------------
>
> (Updated May 30, 2015, 9 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2781
> https://issues.apache.org/jira/browse/MESOS-2781
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix routing ingress/fq_codel search returning wrong qdisc.
>
> Add new checking to verify that operations occur on the correct interface and change the behaviour of create() operation to return false rather than an error if creation fails if the interface does not exist (now consistent with exists() and remove()).
>
>
> Diffs
> -----
>
> src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900
> src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc
> src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed
> src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699
> src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70
>
> Diff: https://reviews.apache.org/r/34830/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34830: Fix routing ingress/fq_codel search
returning wrong qdisc.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34830/#review86029
-----------------------------------------------------------
src/linux/routing/queueing/fq_codel.cpp
<https://reviews.apache.org/r/34830/#comment137878>
This seems irrelavent to the purpose of this patch. Could you please split it out. BTW, this change is related to https://reviews.apache.org/r/34426/ (see the summary part of the comments).
src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/34830/#comment137885>
OK, this is confusing to me. We return false in the following two cases:
1) link does not exist
2) qdisc already exists
Is that expected?
- Jie Yu
On May 30, 2015, 9 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34830/
> -----------------------------------------------------------
>
> (Updated May 30, 2015, 9 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2781
> https://issues.apache.org/jira/browse/MESOS-2781
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix routing ingress/fq_codel search returning wrong qdisc.
>
> Add new checking to verify that operations occur on the correct interface and change the behaviour of create() operation to return false rather than an error if creation fails if the interface does not exist (now consistent with exists() and remove()).
>
>
> Diffs
> -----
>
> src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900
> src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc
> src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed
> src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699
> src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70
>
> Diff: https://reviews.apache.org/r/34830/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34830: Fix routing ingress/fq_codel search
returning wrong qdisc.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34830/#review86060
-----------------------------------------------------------
src/linux/routing/queueing/fq_codel.hpp
<https://reviews.apache.org/r/34830/#comment137939>
Please be consistent. You document the error case for exists() but not for create().
src/linux/routing/queueing/ingress.hpp
<https://reviews.apache.org/r/34830/#comment137940>
and error if?
src/linux/routing/queueing/ingress.hpp
<https://reviews.apache.org/r/34830/#comment137941>
Error when?
src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/34830/#comment137942>
s/Heirachical/Hierarchical/
- Ian Downes
On June 1, 2015, 12:49 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34830/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 12:49 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2781
> https://issues.apache.org/jira/browse/MESOS-2781
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix routing ingress/fq_codel search returning wrong qdisc.
>
> Add new checking to verify that operations occur on the correct interface and change the behaviour of create() operation to return false rather than an error if creation fails if the interface does not exist (now consistent with exists() and remove()).
>
>
> Diffs
> -----
>
> src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900
> src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc
> src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed
> src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70
>
> Diff: https://reviews.apache.org/r/34830/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34830: Fix routing ingress/fq_codel search
returning wrong qdisc.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34830/
-----------------------------------------------------------
(Updated June 2, 2015, 12:23 a.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Changes
-------
Rebase
Bugs: MESOS-2781
https://issues.apache.org/jira/browse/MESOS-2781
Repository: mesos
Description
-------
Fix routing ingress/fq_codel search returning wrong qdisc.
Add new checking to verify that operations occur on the correct interface and change the behaviour of create() operation to return false rather than an error if creation fails if the interface does not exist (now consistent with exists() and remove()).
Diffs (updated)
-----
src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900
src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed
src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70
Diff: https://reviews.apache.org/r/34830/diff/
Testing
-------
make check
Thanks,
Paul Brett
Re: Review Request 34830: Fix routing ingress/fq_codel search
returning wrong qdisc.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34830/
-----------------------------------------------------------
(Updated June 1, 2015, 8:40 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Changes
-------
Incorporate reviewers comments
Bugs: MESOS-2781
https://issues.apache.org/jira/browse/MESOS-2781
Repository: mesos
Description
-------
Fix routing ingress/fq_codel search returning wrong qdisc.
Add new checking to verify that operations occur on the correct interface and change the behaviour of create() operation to return false rather than an error if creation fails if the interface does not exist (now consistent with exists() and remove()).
Diffs (updated)
-----
src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900
src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed
src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70
Diff: https://reviews.apache.org/r/34830/diff/
Testing
-------
make check
Thanks,
Paul Brett
Re: Review Request 34830: Fix routing ingress/fq_codel search
returning wrong qdisc.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34830/
-----------------------------------------------------------
(Updated June 1, 2015, 7:49 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Changes
-------
Break up patch for easier review.
Bugs: MESOS-2781
https://issues.apache.org/jira/browse/MESOS-2781
Repository: mesos
Description
-------
Fix routing ingress/fq_codel search returning wrong qdisc.
Add new checking to verify that operations occur on the correct interface and change the behaviour of create() operation to return false rather than an error if creation fails if the interface does not exist (now consistent with exists() and remove()).
Diffs (updated)
-----
src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900
src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc
src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed
src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70
Diff: https://reviews.apache.org/r/34830/diff/
Testing
-------
make check
Thanks,
Paul Brett
Re: Review Request 34830: Fix routing ingress/fq_codel search
returning wrong qdisc.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34830/
-----------------------------------------------------------
(Updated May 30, 2015, 9 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: MESOS-2781
https://issues.apache.org/jira/browse/MESOS-2781
Repository: mesos
Description
-------
Fix routing ingress/fq_codel search returning wrong qdisc.
Add new checking to verify that operations occur on the correct interface and change the behaviour of create() operation to return false rather than an error if creation fails if the interface does not exist (now consistent with exists() and remove()).
Diffs (updated)
-----
src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900
src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc
src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed
src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699
src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70
Diff: https://reviews.apache.org/r/34830/diff/
Testing
-------
make check
Thanks,
Paul Brett
Re: Review Request 34830: Fix routing ingress/fq_codel search
returning wrong qdisc.
Posted by Paul Brett <pa...@twopensource.com>.
> On May 30, 2015, 6:12 p.m., Isabel Jimenez wrote:
> > src/linux/routing/queueing/internal.hpp, line 128
> > <https://reviews.apache.org/r/34830/diff/1/?file=974718#file974718line128>
> >
> > Why use reinterpret_cast here? am I missing something that needs changing the bit interpretation of this in the machine?
libnl defined the output of nl_cache_get_first to be struct nl_object* but the input to rtnl_tc_get_ifindex is a struct rtnl_tc*. Since these are unrelated pointers, there is no conversion function or class heirarchy so we need a reinterpret_cast.
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34830/#review85828
-----------------------------------------------------------
On May 29, 2015, 9:57 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34830/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 9:57 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2781
> https://issues.apache.org/jira/browse/MESOS-2781
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix routing ingress/fq_codel search returning wrong qdisc.
>
> Add new checking to verify that operations occur on the correct interface and change the behaviour of create() operation to return false rather than an error if creation fails if the interface does not exist (now consistent with exists() and remove()).
>
>
> Diffs
> -----
>
> src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900
> src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc
> src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed
> src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699
> src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70
>
> Diff: https://reviews.apache.org/r/34830/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 34830: Fix routing ingress/fq_codel search
returning wrong qdisc.
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34830/#review85828
-----------------------------------------------------------
Ship it!
src/linux/routing/queueing/fq_codel.hpp
<https://reviews.apache.org/r/34830/#comment137660>
Add a '.' at the end of comments
src/linux/routing/queueing/ingress.hpp
<https://reviews.apache.org/r/34830/#comment137662>
see comment above
src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/34830/#comment137668>
Why use reinterpret_cast here? am I missing something that needs changing the bit interpretation of this in the machine?
src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/34830/#comment137666>
same as above
- Isabel Jimenez
On May 29, 2015, 9:57 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34830/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 9:57 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2781
> https://issues.apache.org/jira/browse/MESOS-2781
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix routing ingress/fq_codel search returning wrong qdisc.
>
> Add new checking to verify that operations occur on the correct interface and change the behaviour of create() operation to return false rather than an error if creation fails if the interface does not exist (now consistent with exists() and remove()).
>
>
> Diffs
> -----
>
> src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900
> src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc
> src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed
> src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699
> src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70
>
> Diff: https://reviews.apache.org/r/34830/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Paul Brett
>
>