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/10 20:17:17 UTC

Review Request 22427: Implemented framework API rate limiting.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22427/
-----------------------------------------------------------

Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.


Repository: mesos-git


Description
-------

To simplify the implementation, RateLimiters are added statically during initialize() and never removed.

visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.


Diffs
-----

  src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f 
  src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613 
  src/tests/rate_limiting_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/22427/diff/


Testing
-------

Added a test for MESOS-1444. Will add more for MESOS-1445


Thanks,

Jiang Yan Xu


Re: Review Request 22427: Implemented framework API rate limiting.

Posted by Dominic Hamon <dh...@twopensource.com>.

> On June 10, 2014, 11:38 a.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 383
> > <https://reviews.apache.org/r/22427/diff/1/?file=606521#file606521line383>
> >
> >     why not clean these up during finalize?
> 
> Jiang Yan Xu wrote:
>     Because they are owned?

good point! i'm a little concerned about mixing the two lifetime scopes of a process. We have ctor to dtor and initialize to finalize. Now this lives from initialize to dtor, which is a little strange but probably harmless.

as an aside, owned doesn't mean scoped. it is scoped as a side-effect of RAII but it is meant to indicate unique ownership.


- Dominic


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22427/#review45268
-----------------------------------------------------------


On June 11, 2014, 10:39 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22427/
> -----------------------------------------------------------
> 
> (Updated June 11, 2014, 10:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-1444
>     https://issues.apache.org/jira/browse/MESOS-1444
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> To simplify the implementation, RateLimiters are added statically during initialize() and never removed.
> 
> visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59 
>   src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09 
>   src/tests/rate_limiting_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22427/diff/
> 
> 
> Testing
> -------
> 
> Added a test for MESOS-1444. Will add more for MESOS-1445
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Re: Review Request 22427: Implemented framework API rate limiting.

Posted by Jiang Yan Xu <ya...@jxu.me>.

> On June 10, 2014, 11:38 a.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 380
> > <https://reviews.apache.org/r/22427/diff/1/?file=606521#file606521line380>
> >
> >     This seems arbitrary - why do we need to do this?

It's no longer necessary as I have revised RateLimiter to take a double permitsPerSecond.


> On June 10, 2014, 11:38 a.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 383
> > <https://reviews.apache.org/r/22427/diff/1/?file=606521#file606521line383>
> >
> >     why not clean these up during finalize?

Because they are owned?


> On June 10, 2014, 11:38 a.m., Dominic Hamon wrote:
> > src/tests/rate_limiting_tests.cpp, line 590
> > <https://reviews.apache.org/r/22427/diff/1/?file=606522#file606522line590>
> >
> >     consider advancing just a little more than one second to avoid any floating point inaccuracies in the rate limiter.

OK but this generally hasn't been an issue previously.


> On June 10, 2014, 11:38 a.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 276
> > <https://reviews.apache.org/r/22427/diff/1/?file=606521#file606521line276>
> >
> >     please try to keep method definitions in the same order as their declarations in the header. It make it much easier to navigate the source.

Done.


- Jiang Yan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22427/#review45268
-----------------------------------------------------------


On June 11, 2014, 10:39 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22427/
> -----------------------------------------------------------
> 
> (Updated June 11, 2014, 10:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-1444
>     https://issues.apache.org/jira/browse/MESOS-1444
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> To simplify the implementation, RateLimiters are added statically during initialize() and never removed.
> 
> visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59 
>   src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09 
>   src/tests/rate_limiting_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22427/diff/
> 
> 
> Testing
> -------
> 
> Added a test for MESOS-1444. Will add more for MESOS-1445
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Re: Review Request 22427: Implemented framework API rate limiting.

Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22427/#review45268
-----------------------------------------------------------



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80011>

    please try to keep method definitions in the same order as their declarations in the header. It make it much easier to navigate the source.



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80012>

    This seems arbitrary - why do we need to do this?



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80014>

    why not clean these up during finalize?



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80015>

    you can avoid a double lookup here using hashmap::get



src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22427/#comment80019>

    consider advancing just a little more than one second to avoid any floating point inaccuracies in the rate limiter.


- Dominic Hamon


On June 10, 2014, 11:17 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22427/
> -----------------------------------------------------------
> 
> (Updated June 10, 2014, 11:17 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> To simplify the implementation, RateLimiters are added statically during initialize() and never removed.
> 
> visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f 
>   src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613 
>   src/tests/rate_limiting_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22427/diff/
> 
> 
> Testing
> -------
> 
> Added a test for MESOS-1444. Will add more for MESOS-1445
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Re: Review Request 22427: Implemented framework API rate limiting.

Posted by Jiang Yan Xu <ya...@jxu.me>.

> On June 10, 2014, 2:43 p.m., Vinod Kone wrote:
> > src/master/master.hpp, lines 245-246
> > <https://reviews.apache.org/r/22427/diff/1/?file=606520#file606520line245>
> >
> >     can you use casting to make it work? see authorizer for example.

Yes! Thanks.


> On June 10, 2014, 2:43 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 281
> > <https://reviews.apache.org/r/22427/diff/1/?file=606521#file606521line281>
> >
> >     const&

It's now "const Option<string> principal = frameworks.principals.get(event.message->from);"

const ref is unnecessary as the copy should be elided.


> On June 10, 2014, 2:43 p.m., Vinod Kone wrote:
> > src/tests/rate_limiting_tests.cpp, lines 534-535
> > <https://reviews.apache.org/r/22427/diff/1/?file=606522#file606522line534>
> >
> >     why is this on the heap?

This one doesn't need to be, changed it to be on the stack.

Added comments for other cases:
  // Create MesosSchedulerDriver on the heap because of the need to
  // destroy it during the test due to MESOS-1456.


> On June 10, 2014, 2:43 p.m., Vinod Kone wrote:
> > src/tests/rate_limiting_tests.cpp, lines 538-539
> > <https://reviews.apache.org/r/22427/diff/1/?file=606522#file606522line538>
> >
> >     multiple registered messages should be dropped by the driver since the driver is already connected. are you sure the scheduler gets registered "callback" multiple times?

My mistake. Changed to Times(1).


> On June 10, 2014, 2:43 p.m., Vinod Kone wrote:
> > src/tests/rate_limiting_tests.cpp, line 598
> > <https://reviews.apache.org/r/22427/diff/1/?file=606522#file606522line598>
> >
> >     i'm assuming you ran this test a few thousand times?

Yes.


- Jiang Yan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22427/#review45286
-----------------------------------------------------------


On June 11, 2014, 10:39 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22427/
> -----------------------------------------------------------
> 
> (Updated June 11, 2014, 10:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-1444
>     https://issues.apache.org/jira/browse/MESOS-1444
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> To simplify the implementation, RateLimiters are added statically during initialize() and never removed.
> 
> visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59 
>   src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09 
>   src/tests/rate_limiting_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22427/diff/
> 
> 
> Testing
> -------
> 
> Added a test for MESOS-1444. Will add more for MESOS-1445
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Re: Review Request 22427: Implemented framework API rate limiting.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22427/#review45286
-----------------------------------------------------------



src/master/master.hpp
<https://reviews.apache.org/r/22427/#comment80047>

    can you use casting to make it work? see authorizer for example.



src/master/master.hpp
<https://reviews.apache.org/r/22427/#comment80050>

    A comment on what the keys are?



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80053>

    const&



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80055>

    s/Duplicate principals/Duplicate principal << principal <</



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80056>

    Expand on the comment.



src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22427/#comment80072>

    why is this on the heap?



src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22427/#comment80075>

    multiple registered messages should be dropped by the driver since the driver is already connected. are you sure the scheduler gets registered "callback" multiple times?



src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22427/#comment80078>

    i'm assuming you ran this test a few thousand times?


- Vinod Kone


On June 10, 2014, 6:17 p.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22427/
> -----------------------------------------------------------
> 
> (Updated June 10, 2014, 6:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> To simplify the implementation, RateLimiters are added statically during initialize() and never removed.
> 
> visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f 
>   src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613 
>   src/tests/rate_limiting_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22427/diff/
> 
> 
> Testing
> -------
> 
> Added a test for MESOS-1444. Will add more for MESOS-1445
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Re: Review Request 22427: Implemented framework API rate limiting.

Posted by Jiang Yan Xu <ya...@jxu.me>.

> On June 10, 2014, 2:06 p.m., Benjamin Hindman wrote:
> > src/master/master.hpp, lines 245-246
> > <https://reviews.apache.org/r/22427/diff/1/?file=606520#file606520line245>
> >
> >     Likely can just call these '_visit' since the signatures are different due to the argument types.

Casted the function pointers and it's working now.


> On June 10, 2014, 2:06 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, line 817
> > <https://reviews.apache.org/r/22427/diff/1/?file=606521#file606521line817>
> >
> >     Can you please add more comments here about what happens when there is a new PID but the same principal? Same for below please.

Changed the comments:
  // Throttle the message if it's a framework message and a
  // RateLimiter is configured for the framework's principal.

This along with the new comments above should be sufficient:
  // 'principal' is Some when the message is from a framework with a
  // principal specified. PID -> principal mapping is added when a
  // framework (re)-registers. Multiple PIDs can share a principal.


- Jiang Yan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22427/#review45288
-----------------------------------------------------------


On June 11, 2014, 10:39 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22427/
> -----------------------------------------------------------
> 
> (Updated June 11, 2014, 10:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-1444
>     https://issues.apache.org/jira/browse/MESOS-1444
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> To simplify the implementation, RateLimiters are added statically during initialize() and never removed.
> 
> visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59 
>   src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09 
>   src/tests/rate_limiting_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22427/diff/
> 
> 
> Testing
> -------
> 
> Added a test for MESOS-1444. Will add more for MESOS-1445
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Re: Review Request 22427: Implemented framework API rate limiting.

Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22427/#review45288
-----------------------------------------------------------


Just a quick fly-by review so not giving a 'Ship It' but deferring to Vinod.


src/master/master.hpp
<https://reviews.apache.org/r/22427/#comment80051>

    Likely can just call these '_visit' since the signatures are different due to the argument types.



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80052>

    Can you please add more comments here about what happens when there is a new PID but the same principal? Same for below please.


- Benjamin Hindman


On June 10, 2014, 6:17 p.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22427/
> -----------------------------------------------------------
> 
> (Updated June 10, 2014, 6:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> To simplify the implementation, RateLimiters are added statically during initialize() and never removed.
> 
> visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f 
>   src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613 
>   src/tests/rate_limiting_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22427/diff/
> 
> 
> Testing
> -------
> 
> Added a test for MESOS-1444. Will add more for MESOS-1445
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Re: Review Request 22427: Implemented framework API 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/22427/#review45334
-----------------------------------------------------------



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80148>

    Changed the comment to:
    
      // Throttle the message if it's a framework message and a
      // RateLimiter is configured for the framework's principal.
    
    This along with the new comment about the counters above should be sufficient.
    


- Jiang Yan Xu


On June 11, 2014, 10:39 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22427/
> -----------------------------------------------------------
> 
> (Updated June 11, 2014, 10:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-1444
>     https://issues.apache.org/jira/browse/MESOS-1444
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> To simplify the implementation, RateLimiters are added statically during initialize() and never removed.
> 
> visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59 
>   src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09 
>   src/tests/rate_limiting_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22427/diff/
> 
> 
> Testing
> -------
> 
> Added a test for MESOS-1444. Will add more for MESOS-1445
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Re: Review Request 22427: Implemented framework API 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/22427/#review45442
-----------------------------------------------------------


>From review meeting with Vinod and BenM.


src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80307>

    EXIT(1) <<



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80308>

    Validate max_qps() use EXIT(1)



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80306>

    Use limiters.empty()


- Jiang Yan Xu


On June 11, 2014, 10:39 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22427/
> -----------------------------------------------------------
> 
> (Updated June 11, 2014, 10:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-1444
>     https://issues.apache.org/jira/browse/MESOS-1444
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> To simplify the implementation, RateLimiters are added statically during initialize() and never removed.
> 
> visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59 
>   src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09 
>   src/tests/rate_limiting_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22427/diff/
> 
> 
> Testing
> -------
> 
> Added a test for MESOS-1444. Will add more for MESOS-1445
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Re: Review Request 22427: Implemented framework API 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/22427/
-----------------------------------------------------------

(Updated June 17, 2014, 2:03 p.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
-------

Vinod's. NNRF.


Bugs: MESOS-1444
    https://issues.apache.org/jira/browse/MESOS-1444


Repository: mesos-git


Description
-------

To simplify the implementation, RateLimiters are added statically during initialize() and never removed.

visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.


Diffs (updated)
-----

  src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59 
  src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09 
  src/tests/rate_limiting_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/22427/diff/


Testing
-------

Added a test for MESOS-1444. Will add more for MESOS-1445


Thanks,

Jiang Yan Xu


Re: Review Request 22427: Implemented framework API rate limiting.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22427/#review45884
-----------------------------------------------------------

Ship it!



src/master/master.hpp
<https://reviews.apache.org/r/22427/#comment80960>

    s/Note:/NOTE:/



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80961>

    s/max_qps/qps/.
    
    also print the value?



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80962>

    kill the 'if'. If the flag is specified, log that rate limiting is enabled.



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80963>

    How about,
    
    The framework is not throttled if:
    1) the principal is not specified by the framework (or)
    2) the principal doesn't exist in rate limits (or)
    3) the principal exists in rate limits but 'qps' is not set.



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80964>

    You can just point to the above comment for details.



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80965>

    single quotes.



src/master/master.cpp
<https://reviews.apache.org/r/22427/#comment80966>

    singe quotes.


- Vinod Kone


On June 17, 2014, 12:48 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22427/
> -----------------------------------------------------------
> 
> (Updated June 17, 2014, 12:48 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1444
>     https://issues.apache.org/jira/browse/MESOS-1444
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> To simplify the implementation, RateLimiters are added statically during initialize() and never removed.
> 
> visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59 
>   src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09 
>   src/tests/rate_limiting_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22427/diff/
> 
> 
> Testing
> -------
> 
> Added a test for MESOS-1444. Will add more for MESOS-1445
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Re: Review Request 22427: Implemented framework API 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/22427/
-----------------------------------------------------------

(Updated June 16, 2014, 5:48 p.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
-------

Changes to handle RateLimits.max_qps being renamed to 'qps' and becomes optional.


Bugs: MESOS-1444
    https://issues.apache.org/jira/browse/MESOS-1444


Repository: mesos-git


Description
-------

To simplify the implementation, RateLimiters are added statically during initialize() and never removed.

visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.


Diffs (updated)
-----

  src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59 
  src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09 
  src/tests/rate_limiting_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/22427/diff/


Testing
-------

Added a test for MESOS-1444. Will add more for MESOS-1445


Thanks,

Jiang Yan Xu


Re: Review Request 22427: Implemented framework API rate limiting.

Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22427/#review45873
-----------------------------------------------------------


Bad patch!

Reviews applied: [22427]

Failed command: git apply --index 22427.patch

Error:
 error: patch failed: src/master/master.hpp:510
error: src/master/master.hpp: patch does not apply
error: patch failed: src/master/master.cpp:352
error: src/master/master.cpp: patch does not apply
error: src/tests/rate_limiting_tests.cpp: does not exist in index


- Mesos ReviewBot


On June 13, 2014, 11:55 p.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22427/
> -----------------------------------------------------------
> 
> (Updated June 13, 2014, 11:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-1444
>     https://issues.apache.org/jira/browse/MESOS-1444
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> To simplify the implementation, RateLimiters are added statically during initialize() and never removed.
> 
> visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59 
>   src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09 
>   src/tests/rate_limiting_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22427/diff/
> 
> 
> Testing
> -------
> 
> Added a test for MESOS-1444. Will add more for MESOS-1445
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Re: Review Request 22427: Implemented framework API 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/22427/
-----------------------------------------------------------

(Updated June 13, 2014, 4:55 p.m.)


Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.


Changes
-------

Comments.


Bugs: MESOS-1444
    https://issues.apache.org/jira/browse/MESOS-1444


Repository: mesos-git


Description
-------

To simplify the implementation, RateLimiters are added statically during initialize() and never removed.

visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.


Diffs (updated)
-----

  src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59 
  src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09 
  src/tests/rate_limiting_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/22427/diff/


Testing
-------

Added a test for MESOS-1444. Will add more for MESOS-1445


Thanks,

Jiang Yan Xu


Re: Review Request 22427: Implemented framework API rate limiting.

Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22427/#review45504
-----------------------------------------------------------


Bad patch!

Reviews applied: [22424, 22426, 22425, 22427]

Failed command: git apply --index 22427.patch

Error:
 error: patch failed: src/master/master.cpp:797
error: src/master/master.cpp: patch does not apply
error: src/tests/rate_limiting_tests.cpp: does not exist in index


- Mesos ReviewBot


On June 11, 2014, 5:39 p.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22427/
> -----------------------------------------------------------
> 
> (Updated June 11, 2014, 5:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-1444
>     https://issues.apache.org/jira/browse/MESOS-1444
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> To simplify the implementation, RateLimiters are added statically during initialize() and never removed.
> 
> visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59 
>   src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09 
>   src/tests/rate_limiting_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22427/diff/
> 
> 
> Testing
> -------
> 
> Added a test for MESOS-1444. Will add more for MESOS-1445
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Re: Review Request 22427: Implemented framework API 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/22427/
-----------------------------------------------------------

(Updated June 11, 2014, 10:39 a.m.)


Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.


Changes
-------

Comments.


Bugs: MESOS-1444
    https://issues.apache.org/jira/browse/MESOS-1444


Repository: mesos-git


Description
-------

To simplify the implementation, RateLimiters are added statically during initialize() and never removed.

visit(MessageEvent) and visit(ExitedEvent) are throttled together to keep their order intact.


Diffs (updated)
-----

  src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59 
  src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09 
  src/tests/rate_limiting_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/22427/diff/


Testing
-------

Added a test for MESOS-1444. Will add more for MESOS-1445


Thanks,

Jiang Yan Xu