You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2014/06/18 20:30:28 UTC
Review Request 22740: Added more tests for framework rate limiting.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22740/
-----------------------------------------------------------
Review request for mesos, Ben Mahler and Vinod Kone.
Bugs: MESOS-1445
https://issues.apache.org/jira/browse/MESOS-1445
Repository: mesos-git
Description
-------
See summary.
Diffs
-----
src/tests/rate_limiting_tests.cpp 9a544618e70e7ce3091fa303597eb1bb3d161bf9
Diff: https://reviews.apache.org/r/22740/diff/
Testing
-------
make check all test and *RateLimitingTest* for high iterations.
Thanks,
Jiang Yan Xu
Re: Review Request 22740: Added more tests for framework rate limiting.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 19, 2014, 6:41 p.m., Vinod Kone wrote:
> > Only partial review. But after looking at some tests, this parameterizing test behavior inside the tests seems more complicated than its worth. I think its better to just convert all these tests to rate limiting tests. We have lot of other tests in our code base that test the code without rate limiting. You could still ensure that the counters are correct when you are using rate limiting. The correctness of the counters code doesn't depend on whether you are rate limiting or not, so it should be fine. Thoughts?
Other tests, without modifications, do not test the counters and explicit unlimited rates.
In my tests, with throttling == false, counters directly go to the "processed" state whereas with throttling they go through the intermediate "received but not processed" stage and transition only happens with Clock advances. That's how I am using the the template parameter.
But I agree it's simpler not to have the template and just test rate limiting for most tests. I'll make the change.
I'll turn RateLimitingTest.FrameworkMessageCounterMetrics into RateLimitingTest.NoRateLimiting so we have the test for the "explicit unlimited rates" case too.
> On June 19, 2014, 6:41 p.m., Vinod Kone wrote:
> > src/tests/rate_limiting_tests.cpp, lines 113-119
> > <https://reviews.apache.org/r/22740/diff/2/?file=613101#file613101line113>
> >
> > Can you do it after you start the master?
Yeah I realized this after I saw a few flaky Jenkins tests but the issue here I believe is that I need to do a Clock::settle after the AWAIT_READY.
StartMaster() already waits for Master's recovery to finish but it's "not quite" finished when FUTUER_DISPATCH(_recover) is ready because _recover has yet to finish execution.
Maybe we should add that settle into StartMaster().
> On June 19, 2014, 6:41 p.m., Vinod Kone wrote:
> > src/tests/rate_limiting_tests.cpp, line 194
> > <https://reviews.apache.org/r/22740/diff/2/?file=613101#file613101line194>
> >
> > you can kill this. this is done by the test teardown by default.
Done.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22740/#review46243
-----------------------------------------------------------
On June 19, 2014, 12:03 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22740/
> -----------------------------------------------------------
>
> (Updated June 19, 2014, 12:03 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1445
> https://issues.apache.org/jira/browse/MESOS-1445
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/rate_limiting_tests.cpp 9a544618e70e7ce3091fa303597eb1bb3d161bf9
>
> Diff: https://reviews.apache.org/r/22740/diff/
>
>
> Testing
> -------
>
> make check all test and *RateLimitingTest* for high iterations.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22740: Added more tests for framework rate limiting.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22740/#review46243
-----------------------------------------------------------
Only partial review. But after looking at some tests, this parameterizing test behavior inside the tests seems more complicated than its worth. I think its better to just convert all these tests to rate limiting tests. We have lot of other tests in our code base that test the code without rate limiting. You could still ensure that the counters are correct when you are using rate limiting. The correctness of the counters code doesn't depend on whether you are rate limiting or not, so it should be fine. Thoughts?
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22740/#comment81555>
Instead of doing this..why not just overload CreateMasterFlags() like we did in slave recovery tests?
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22740/#comment81561>
Can you do it after you start the master?
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22740/#comment81560>
you can kill this. this is done by the test teardown by default.
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22740/#comment81562>
ditto.
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22740/#comment81563>
s/and and/and/
- Vinod Kone
On June 19, 2014, 7:03 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22740/
> -----------------------------------------------------------
>
> (Updated June 19, 2014, 7:03 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1445
> https://issues.apache.org/jira/browse/MESOS-1445
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/rate_limiting_tests.cpp 9a544618e70e7ce3091fa303597eb1bb3d161bf9
>
> Diff: https://reviews.apache.org/r/22740/diff/
>
>
> Testing
> -------
>
> make check all test and *RateLimitingTest* for high iterations.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22740: Added more tests for framework rate limiting.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22740/
-----------------------------------------------------------
(Updated June 22, 2014, 8:49 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Addressed Vinod's comments and went through the review for comments not updated after removing the test template.
NNFR.
Bugs: MESOS-1445
https://issues.apache.org/jira/browse/MESOS-1445
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/tests/rate_limiting_tests.cpp 9a544618e70e7ce3091fa303597eb1bb3d161bf9
Diff: https://reviews.apache.org/r/22740/diff/
Testing
-------
make check all test and *RateLimitingTest* for high iterations.
Thanks,
Jiang Yan Xu
Re: Review Request 22740: Added more tests for framework rate limiting.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 20, 2014, 5:19 p.m., Vinod Kone wrote:
> > src/tests/rate_limiting_tests.cpp, line 521
> > <https://reviews.apache.org/r/22740/diff/3/?file=614447#file614447line521>
> >
> > s/if we are throttling//
> >
> > Looks some of these comments were not updated after removal of templatization. Can you make a pass and make sure they are updated?
Thanks for catching these :) I did a another pass and should be OK now.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22740/#review46349
-----------------------------------------------------------
On June 22, 2014, 8:49 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22740/
> -----------------------------------------------------------
>
> (Updated June 22, 2014, 8:49 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1445
> https://issues.apache.org/jira/browse/MESOS-1445
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/rate_limiting_tests.cpp 9a544618e70e7ce3091fa303597eb1bb3d161bf9
>
> Diff: https://reviews.apache.org/r/22740/diff/
>
>
> Testing
> -------
>
> make check all test and *RateLimitingTest* for high iterations.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22740: Added more tests for framework rate limiting.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22740/#review46349
-----------------------------------------------------------
Ship it!
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22740/#comment81680>
Kill this.
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22740/#comment81682>
s/+/ +/
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22740/#comment81683>
update the comment.
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22740/#comment81684>
s/could/should/
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22740/#comment81685>
s/if we are throttling//
Looks some of these comments were not updated after removal of templatization. Can you make a pass and make sure they are updated?
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22740/#comment81687>
s/it's/ as it's/
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22740/#comment81688>
Why do you need to pass CreateMasterFlags() here?
- Vinod Kone
On June 20, 2014, 11:37 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22740/
> -----------------------------------------------------------
>
> (Updated June 20, 2014, 11:37 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1445
> https://issues.apache.org/jira/browse/MESOS-1445
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/rate_limiting_tests.cpp 9a544618e70e7ce3091fa303597eb1bb3d161bf9
>
> Diff: https://reviews.apache.org/r/22740/diff/
>
>
> Testing
> -------
>
> make check all test and *RateLimitingTest* for high iterations.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22740: Added more tests for framework rate limiting.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22740/
-----------------------------------------------------------
(Updated June 20, 2014, 4:37 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Vinod's comments.
- Removed templatization.
Bugs: MESOS-1445
https://issues.apache.org/jira/browse/MESOS-1445
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/tests/rate_limiting_tests.cpp 9a544618e70e7ce3091fa303597eb1bb3d161bf9
Diff: https://reviews.apache.org/r/22740/diff/
Testing
-------
make check all test and *RateLimitingTest* for high iterations.
Thanks,
Jiang Yan Xu
Re: Review Request 22740: Added more tests for framework rate limiting.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22740/
-----------------------------------------------------------
(Updated June 19, 2014, 12:03 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Bugs: MESOS-1445
https://issues.apache.org/jira/browse/MESOS-1445
Repository: mesos-git
Description
-------
See summary.
Diffs
-----
src/tests/rate_limiting_tests.cpp 9a544618e70e7ce3091fa303597eb1bb3d161bf9
Diff: https://reviews.apache.org/r/22740/diff/
Testing
-------
make check all test and *RateLimitingTest* for high iterations.
Thanks,
Jiang Yan Xu
Re: Review Request 22740: Added more tests for framework rate limiting.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22740/
-----------------------------------------------------------
(Updated June 19, 2014, 12:03 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Minor fix.
Bugs: MESOS-1445
https://issues.apache.org/jira/browse/MESOS-1445
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/tests/rate_limiting_tests.cpp 9a544618e70e7ce3091fa303597eb1bb3d161bf9
Diff: https://reviews.apache.org/r/22740/diff/
Testing
-------
make check all test and *RateLimitingTest* for high iterations.
Thanks,
Jiang Yan Xu
Re: Review Request 22740: Added more tests for framework rate limiting.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22740/#review46107
-----------------------------------------------------------
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22740/#comment81269>
I am still using the METRICS_SNAPSHOT macro from https://reviews.apache.org/r/22639/
Will address the comments in r22639 soon.
- Jiang Yan Xu
On June 18, 2014, 11:30 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22740/
> -----------------------------------------------------------
>
> (Updated June 18, 2014, 11:30 a.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1445
> https://issues.apache.org/jira/browse/MESOS-1445
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/rate_limiting_tests.cpp 9a544618e70e7ce3091fa303597eb1bb3d161bf9
>
> Diff: https://reviews.apache.org/r/22740/diff/
>
>
> Testing
> -------
>
> make check all test and *RateLimitingTest* for high iterations.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>