Re: Review Request 34830: Fix routing ingress/fq_codel search returning wrong qdisc.
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.
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.
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.
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.
--- 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.
--- 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.
--- 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.
--- 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.
--- 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
