subject:"Re\: Review Request 24343\: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal."

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-07 Thread Jiang Yan Xu

> On Aug. 7, 2014, 5:33 p.m., Ben Mahler wrote:
> > I realize this is already committed, had a few questions around doing this 
> > more cleanly. Let me know your thoughts!

I appreciate these comments and let's chat about what could be done to improve 
this!

> On Aug. 7, 2014, 5:33 p.m., Ben Mahler wrote:
> > src/master/master.hpp, lines 778-781
> > 
> >
> > What is the difference between None() and no entry in the map? If 
> > there's no difference should we eliminate the Option? Otherwise, maybe a 
> > comment is necessary here?

None is "not throttled", no entry is "use the default throttler" (if one is 
configured). If no default throttler is configured, the two cases are same.

Therefore there is a difference and the Option cannot be eliminated.

> On Aug. 7, 2014, 5:33 p.m., Ben Mahler wrote:
> > src/master/master.hpp, lines 762-776
> > 
> >
> > Have you considered making this an 'EventLimiter'? Right now the Master 
> > has to do the capacity accounting correctly whenever it throttles something.

Yes, see earlier versions of the review: 
https://reviews.apache.org/r/24343/diff/2/#index_header
The reason the logic is put back into Master is that Master still needs to call 
a nextEvent() method to get the event (as in version 2 of the review) or at 
least to do the capacity accounting. Then there was this question of "what if 
the user of the nested class (i.e., Master) doesn't call the methods in 
EventThrottler according to the prescribed order". With a data/dumb struct the 
caller still needs to know not to manipulate it incorrectly but at least with a 
dumb struct it's obvious that the burden is on the caller.

I basically followed the convention in other structs nested in Master.

I am all for modularization given the ever increasing complexity in Master but 
the challenge here is that the EventThrottler calls a method in RateLimiter 
that returns a Future but it cannot defer the Future to itself because it's not 
a Process. Only master can provide the callback and thus inevitably needs to 
call into the class again. 

We can definitely iteration on this (e.g., can EventThrottler be itself a 
Process and be responsible for throttling all frameworks?) and let's chat if 
you have good ideas.

> On Aug. 7, 2014, 5:33 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 944-950
> > 
> >
> > What is the invariant here that ensures that the principal is still 
> > present in the 'limiters' map? Is there a way we can better enforce the 
> > 'const'ness of the limiters map? A clear comment would be nice. Maybe some 
> > CHECKs here as well?
> > 
> > Per my comment above, if we moved both the
> >  (1) message accounting, and
> >  (2) ExitedEvent vs. MessageEvent throttling decision
> > into the 'BoundedRateLimiter', then no accounting logic would be needed 
> > here.
> > 
> > We could use failed Futures to represent being over capacity as well, 
> > simplifying the logic you have above.
> > 
> > Any reason that you didn't chose to do it this way?

You are right! The CHECKs unfortunately weren't added in this review but they 
were in a subsequent one: 
https://github.com/apache/mesos/commit/8c4f45d67be22cfe252ad6ed27a79ad4a1f972c6

The limiters are static (never modified after initialization) so I thought it 
was clear. Of course I could add a comment here.

> On Aug. 7, 2014, 5:33 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 940
> > 
> >
> > Any reason to call this throttled? This looks like _visit and _visit 
> > looks like __visit:
> > 
> > Exited path:  visit(ExitedEvent) -> _visit(ExitedEvent)
> > Message path: visit(MessageEvent) -> throttled(MessageEvent) -> 
> > _visit(MessageEvent)
> > 
> > A more typical continuation naming scheme here would be:
> > 
> > Exited path:  visit(ExitedEvent)  -> _visit(ExitedEvent)
> > Message path: visit(MessageEvent) -> _visit(MessageEvent) -> 
> > __visit(MessageEvent)
> > 
> > This exposes the flow more clearly, no? But! Per my comment below, I 
> > think we can eliminate the need for 'throttled' entirely if we encapsulate 
> > the queue accounting inside the 'EventLimiter' abstraction. Anything I'm 
> > missing?

There is also a "capacity exceeded" path.
I was using the scheme you suggested but if you see Vinod's and Tim's comment, 
there were a few issues raised:
1. What constitutes a "continuation" and should use the underscore prefix 
scheme (e.g., whether it needs to be a deferred callback).
2. If a method clearly serves a single purpose that can be clearly summarized 
by one word, should we should a more explicit name?

We haven't been super consistent wi

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-07 Thread Ben Mahler

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

I realize this is already committed, had a few questions around doing this more 
cleanly. Let me know your thoughts!

src/master/master.hpp

Have you considered making this an 'EventLimiter'? Right now the Master has 
to do the capacity accounting correctly whenever it throttles something.

src/master/master.hpp

What is the difference between None() and no entry in the map? If there's 
no difference should we eliminate the Option? Otherwise, maybe a comment is 
necessary here?

src/master/master.cpp

Any reason to call this throttled? This looks like _visit and _visit looks 
like __visit:

Exited path:  visit(ExitedEvent) -> _visit(ExitedEvent)
Message path: visit(MessageEvent) -> throttled(MessageEvent) -> 
_visit(MessageEvent)

A more typical continuation naming scheme here would be:

Exited path:  visit(ExitedEvent)  -> _visit(ExitedEvent)
Message path: visit(MessageEvent) -> _visit(MessageEvent) -> 
__visit(MessageEvent)

This exposes the flow more clearly, no? But! Per my comment below, I think 
we can eliminate the need for 'throttled' entirely if we encapsulate the queue 
accounting inside the 'EventLimiter' abstraction. Anything I'm missing?

src/master/master.cpp

What is the invariant here that ensures that the principal is still present 
in the 'limiters' map? Is there a way we can better enforce the 'const'ness of 
the limiters map? A clear comment would be nice. Maybe some CHECKs here as well?

Per my comment above, if we moved both the
 (1) message accounting, and
 (2) ExitedEvent vs. MessageEvent throttling decision
into the 'BoundedRateLimiter', then no accounting logic would be needed 
here.

We could use failed Futures to represent being over capacity as well, 
simplifying the logic you have above.

Any reason that you didn't chose to do it this way?

- Ben Mahler

On Aug. 7, 2014, 6:26 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24343/
> ---
> 
> (Updated Aug. 7, 2014, 6:26 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1578
> https://issues.apache.org/jira/browse/MESOS-1578
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 628cce12d2fae645d2ef55e4809631ca03a56207 
>   src/examples/load_generator_framework.cpp 
> 7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
>   src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
>   src/master/master.cpp 97e4340f7949a261558f09ea533aac0bbb0e40f6 
>   src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 
> 
> Diff: https://reviews.apache.org/r/24343/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> ./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-06 Thread Jiang Yan Xu


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

(Updated Aug. 6, 2014, 11:26 p.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
---

Vinod's comments. No need for review.


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


Repository: mesos-git


Description
---

See summary.


Diffs (updated)
-

  include/mesos/mesos.proto 628cce12d2fae645d2ef55e4809631ca03a56207 
  src/examples/load_generator_framework.cpp 
7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
  src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
  src/master/master.cpp 97e4340f7949a261558f09ea533aac0bbb0e40f6 
  src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 

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


Testing
---

make check

./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000


Thanks,

Jiang Yan Xu

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-06 Thread Vinod Kone


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

Ship it!



src/master/master.hpp


s/be/being/



src/master/master.cpp


Would this be simpler if you assign the correct rate limiter (specific or 
default) first?

Option > limiter;

if (use specificLimiter) {
  limiter = specific limiter;
} else if (use defualtLimiter) {
  limiter = defaultLimiter;
}

if (limiter.isSome()) {
  ...
} else {
   _visit(event);
}



- Vinod Kone


On Aug. 7, 2014, 12:25 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24343/
> ---
> 
> (Updated Aug. 7, 2014, 12:25 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1578
> https://issues.apache.org/jira/browse/MESOS-1578
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 628cce12d2fae645d2ef55e4809631ca03a56207 
>   src/examples/load_generator_framework.cpp 
> 7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
>   src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
>   src/master/master.cpp 97e4340f7949a261558f09ea533aac0bbb0e40f6 
>   src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 
> 
> Diff: https://reviews.apache.org/r/24343/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> ./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-06 Thread Jiang Yan Xu


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

(Updated Aug. 6, 2014, 5:25 p.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
---

Now use uint64_t instead of size_t to keep track of capacity and number of 
outstanding messages.


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


Repository: mesos-git


Description
---

See summary.


Diffs (updated)
-

  include/mesos/mesos.proto 628cce12d2fae645d2ef55e4809631ca03a56207 
  src/examples/load_generator_framework.cpp 
7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
  src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
  src/master/master.cpp 97e4340f7949a261558f09ea533aac0bbb0e40f6 
  src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 

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


Testing
---

make check

./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000


Thanks,

Jiang Yan Xu

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-06 Thread Jiang Yan Xu



> On Aug. 6, 2014, 4:53 p.m., Dominic Hamon wrote:
> > src/master/master.hpp, line 770
> > 
> >
> > this should either be a uint64 (to match the proto) or the proto should 
> > be a uint32 (to be reasonable). There's no real benefit to storing this as 
> > a size_t.
> > 
> > i'll note that we claim to follow the google c++ style guide which 
> > states that this should really just be an int unless we know we want to 
> > support more than 2^31 messages (unlikely): 
> > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types
> > 
> >

thanks. the code used to check this against the queue.size() so size_t was 
used. not anymore. so I can just use uint64_t. 

I think uint64 is more future proof and the style guide says "When in doubt, 
choose a larger type" :)


- Jiang Yan


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


On Aug. 6, 2014, 4:37 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24343/
> ---
> 
> (Updated Aug. 6, 2014, 4:37 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1578
> https://issues.apache.org/jira/browse/MESOS-1578
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 628cce12d2fae645d2ef55e4809631ca03a56207 
>   src/examples/load_generator_framework.cpp 
> 7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
>   src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
>   src/master/master.cpp 97e4340f7949a261558f09ea533aac0bbb0e40f6 
>   src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 
> 
> Diff: https://reviews.apache.org/r/24343/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> ./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-06 Thread Dominic Hamon


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



src/master/master.hpp


this should either be a uint64 (to match the proto) or the proto should be 
a uint32 (to be reasonable). There's no real benefit to storing this as a 
size_t.

i'll note that we claim to follow the google c++ style guide which states 
that this should really just be an int unless we know we want to support more 
than 2^31 messages (unlikely): 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types




- Dominic Hamon


On Aug. 6, 2014, 4:37 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24343/
> ---
> 
> (Updated Aug. 6, 2014, 4:37 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1578
> https://issues.apache.org/jira/browse/MESOS-1578
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 628cce12d2fae645d2ef55e4809631ca03a56207 
>   src/examples/load_generator_framework.cpp 
> 7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
>   src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
>   src/master/master.cpp 97e4340f7949a261558f09ea533aac0bbb0e40f6 
>   src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 
> 
> Diff: https://reviews.apache.org/r/24343/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> ./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-06 Thread Jiang Yan Xu


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

(Updated Aug. 6, 2014, 4:37 p.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
---

Rebased and removed some changes not belonged in this review.


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


Repository: mesos-git


Description
---

See summary.


Diffs (updated)
-

  include/mesos/mesos.proto 628cce12d2fae645d2ef55e4809631ca03a56207 
  src/examples/load_generator_framework.cpp 
7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
  src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
  src/master/master.cpp 97e4340f7949a261558f09ea533aac0bbb0e40f6 
  src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 

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


Testing
---

make check

./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000


Thanks,

Jiang Yan Xu

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-06 Thread Jiang Yan Xu



> On Aug. 6, 2014, 9:38 a.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 375
> > 
> >
> > Option capacity;
> > if (limit_.has_capacity()) {
> >   capacity = limit_.capacity();
> > }
> > 
> > should work due to implicit conversion and is much clearer.

ok I agree. thanks.
I didn't remove the static_cast because it's more explicit and it might be a 
narrowing conversion. I added check for this case. 


> On Aug. 6, 2014, 9:38 a.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 394
> > 
> >
> > see above

done


> On Aug. 6, 2014, 9:38 a.m., Dominic Hamon wrote:
> > src/master/master.hpp, line 756
> > 
> >
> > it seems to me this could have some unit tests created outside of 
> > master tests. I'd really like to see that.

It's a good idea but for this review it's no longer applicable because the 
logic has been moved to Master::visit().
I agree in the future we should strive to test individual methods and in fact 
refactor long methods in Master. But in this case it proved to be difficult.


- Jiang Yan


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


On Aug. 6, 2014, 4:13 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24343/
> ---
> 
> (Updated Aug. 6, 2014, 4:13 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1578
> https://issues.apache.org/jira/browse/MESOS-1578
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 628cce12d2fae645d2ef55e4809631ca03a56207 
>   src/examples/load_generator_framework.cpp 
> 7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
>   src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
>   src/master/master.cpp a925a936bc1cd878f1c9ff63a9dcc4ee808ec7a5 
>   src/tests/cluster.hpp d857fc604f5b47cf9805b37c13051093f486ba31 
>   src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 
> 
> Diff: https://reviews.apache.org/r/24343/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> ./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-06 Thread Jiang Yan Xu



> On Aug. 6, 2014, 11:30 a.m., Vinod Kone wrote:
> > src/tests/rate_limiting_tests.cpp, lines 973-975
> > 
> >
> > Isn't this already done by StartMaster()?

Yes. I lost track of the change :)


> On Aug. 6, 2014, 11:30 a.m., Vinod Kone wrote:
> > src/master/master.hpp, line 792
> > 
> >
> > seems weird for this to be a CHECK, since the users of this class has 
> > no idea about the restriction. How about returning an Error instead?
> > 
> > Try nextPermittedEvent() {}
> > 
> > Also, s/nextPermittedEvent/nextEvent/ ?

Removed EventThrottler and made it a dumb struct. Put the logic inside visit() 
per discussion with Vinod.


> On Aug. 6, 2014, 11:30 a.m., Vinod Kone wrote:
> > src/master/master.cpp, line 984
> > 
> >
> > +1. This is not really a continuation i.e., never called asynchronously.

In master we already use continuations that are not called asynchronously e.g. 
__reregisterSlave(), or maybe I don't need to say it's a "continuation".
Anyway, __visit() is gone and the behavior the other _visit()s has changed in 
this new revision, please take a look.


> On Aug. 6, 2014, 11:30 a.m., Vinod Kone wrote:
> > src/tests/rate_limiting_tests.cpp, line 1048
> > 
> >
> > s/reach/processed/ ?

I meant to comment on the capacity with "reach".
I rephrased it to be:

// Send two messages which will be queued up. This will reach but not
// exceed the capacity.


> On Aug. 6, 2014, 11:30 a.m., Vinod Kone wrote:
> > src/tests/rate_limiting_tests.cpp, line 1071
> > 
> >
> > how are you making sure the deactivate message is received by the 
> > master?

DeactivateFrameworkMessage is sent by the driver upon receiving the error 
message so with Clock::settle() we can be sure the master has received it (not 
necessarily processed).
Should there be more comment to explain this?


- Jiang Yan


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


On Aug. 6, 2014, 4:13 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24343/
> ---
> 
> (Updated Aug. 6, 2014, 4:13 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1578
> https://issues.apache.org/jira/browse/MESOS-1578
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 628cce12d2fae645d2ef55e4809631ca03a56207 
>   src/examples/load_generator_framework.cpp 
> 7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
>   src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
>   src/master/master.cpp a925a936bc1cd878f1c9ff63a9dcc4ee808ec7a5 
>   src/tests/cluster.hpp d857fc604f5b47cf9805b37c13051093f486ba31 
>   src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 
> 
> Diff: https://reviews.apache.org/r/24343/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> ./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-06 Thread Jiang Yan Xu



> On Aug. 5, 2014, 11:25 p.m., Timothy Chen wrote:
> > src/master/master.hpp, line 766
> > 
> >
> > It also returns error when the rate limiter reachs its capacity too 
> > right?

the rate limiter doesn't have a capacity.


- Jiang Yan


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


On Aug. 6, 2014, 4:13 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24343/
> ---
> 
> (Updated Aug. 6, 2014, 4:13 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1578
> https://issues.apache.org/jira/browse/MESOS-1578
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 628cce12d2fae645d2ef55e4809631ca03a56207 
>   src/examples/load_generator_framework.cpp 
> 7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
>   src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
>   src/master/master.cpp a925a936bc1cd878f1c9ff63a9dcc4ee808ec7a5 
>   src/tests/cluster.hpp d857fc604f5b47cf9805b37c13051093f486ba31 
>   src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 
> 
> Diff: https://reviews.apache.org/r/24343/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> ./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-06 Thread Jiang Yan Xu


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

(Updated Aug. 6, 2014, 4:13 p.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
---

Addressed comments.

Pulled the logic in EventThrottler into Master::visit() after discussion with 
Vinod because it is unable to enforce that the caller (i.e., Master) use its 
methods correctly (in the correct order).
Changed it to a dumb struct BoundedRateLimiter the same way other structs 
nested in Master are used.


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


Repository: mesos-git


Description
---

See summary.


Diffs (updated)
-

  include/mesos/mesos.proto 628cce12d2fae645d2ef55e4809631ca03a56207 
  src/examples/load_generator_framework.cpp 
7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
  src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
  src/master/master.cpp a925a936bc1cd878f1c9ff63a9dcc4ee808ec7a5 
  src/tests/cluster.hpp d857fc604f5b47cf9805b37c13051093f486ba31 
  src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 

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


Testing
---

make check

./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000


Thanks,

Jiang Yan Xu

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-06 Thread Vinod Kone


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



include/mesos/mesos.proto


s/are//



src/master/master.hpp


s/head/the head/?



src/master/master.hpp


seems weird for this to be a CHECK, since the users of this class has no 
idea about the restriction. How about returning an Error instead?

Try nextPermittedEvent() {}

Also, s/nextPermittedEvent/nextEvent/ ?



src/master/master.cpp


s/throttle_/throttle/ ?



src/master/master.cpp


+1. This is not really a continuation i.e., never called asynchronously.



src/master/master.cpp


s/EventThrottler returns error://



src/tests/rate_limiting_tests.cpp


Isn't this already done by StartMaster()?



src/tests/rate_limiting_tests.cpp


s/reach/processed/ ?



src/tests/rate_limiting_tests.cpp


s/it//



src/tests/rate_limiting_tests.cpp


how are you making sure the deactivate message is received by the master?


- Vinod Kone


On Aug. 6, 2014, 5:53 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24343/
> ---
> 
> (Updated Aug. 6, 2014, 5:53 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1578
> https://issues.apache.org/jira/browse/MESOS-1578
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 6d4fd145004a14514e27ba65b2eae19b945aeb83 
>   src/examples/load_generator_framework.cpp 
> 7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
>   src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
>   src/master/master.cpp 9bede96083f56768a5d42baf051af97de4c48d48 
>   src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 
> 
> Diff: https://reviews.apache.org/r/24343/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> ./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-06 Thread Dominic Hamon


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



src/master/master.hpp


it seems to me this could have some unit tests created outside of master 
tests. I'd really like to see that.



src/master/master.cpp


Option capacity;
if (limit_.has_capacity()) {
  capacity = limit_.capacity();
}

should work due to implicit conversion and is much clearer.



src/master/master.cpp


see above


- Dominic Hamon


On Aug. 5, 2014, 10:53 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24343/
> ---
> 
> (Updated Aug. 5, 2014, 10:53 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1578
> https://issues.apache.org/jira/browse/MESOS-1578
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 6d4fd145004a14514e27ba65b2eae19b945aeb83 
>   src/examples/load_generator_framework.cpp 
> 7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
>   src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
>   src/master/master.cpp 9bede96083f56768a5d42baf051af97de4c48d48 
>   src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 
> 
> Diff: https://reviews.apache.org/r/24343/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> ./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-05 Thread Timothy Chen


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



src/master/master.hpp


It also returns error when the rate limiter reachs its capacity too right?



src/master/master.hpp


Nit: Seems like it's used just as a queue, why not just use queue?



src/master/master.cpp


I wonder if we should rename __visit to something like dropMessage or 
failed that it's obvious it's the failed throttle/limiter path.



src/tests/rate_limiting_tests.cpp


I think you can bring it back one line:
error(driver,
  "Message dropped: Reached capacity: 2")
  


- Timothy Chen


On Aug. 6, 2014, 5:53 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24343/
> ---
> 
> (Updated Aug. 6, 2014, 5:53 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1578
> https://issues.apache.org/jira/browse/MESOS-1578
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 6d4fd145004a14514e27ba65b2eae19b945aeb83 
>   src/examples/load_generator_framework.cpp 
> 7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
>   src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
>   src/master/master.cpp 9bede96083f56768a5d42baf051af97de4c48d48 
>   src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 
> 
> Diff: https://reviews.apache.org/r/24343/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> ./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-05 Thread Jiang Yan Xu


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

(Updated Aug. 5, 2014, 10:53 p.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
---

Dominic's comments.


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


Repository: mesos-git


Description
---

See summary.


Diffs (updated)
-

  include/mesos/mesos.proto 6d4fd145004a14514e27ba65b2eae19b945aeb83 
  src/examples/load_generator_framework.cpp 
7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
  src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
  src/master/master.cpp 9bede96083f56768a5d42baf051af97de4c48d48 
  src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 

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


Testing
---

make check

./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000


Thanks,

Jiang Yan Xu

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-05 Thread Jiang Yan Xu



> On Aug. 5, 2014, 4:50 p.m., Dominic Hamon wrote:
> > src/master/master.hpp, line 753
> > 
> >
> > is there a reason this is a nested class in Master?

It only intended to be used in Master but I guess it doesn't need to be defined 
in Master.
Existing "internal" classes and structs defined in master.hpp are defined both 
inside and outside but the ones outside are accessed from http.cpp as well so I 
figured that's the reason.
In this case I just thought it logically belongs "inside". Any reason not to be 
put inside as nested?


> On Aug. 5, 2014, 4:50 p.m., Dominic Hamon wrote:
> > src/master/master.hpp, line 766
> > 
> >
> > might be worth adding the capacity to the error message to make it 
> > easier to debug issues.

Done.


> On Aug. 5, 2014, 4:50 p.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 939
> > 
> >
> > maybe a const Option& here?

Thanks!


> On Aug. 5, 2014, 4:50 p.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 377
> > 
> >
> > please pull these out into more traditional if statements above the 
> > 'put' call. this is completely unreadable.

Done.


> On Aug. 5, 2014, 4:50 p.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 884
> > 
> >
> > assuming there might be more errors in the future, this comment might 
> > be misleading.

Removed the comment and now pass the returned error to __visit() for better 
logging and error message sent to framework.


> On Aug. 5, 2014, 4:50 p.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 997
> > 
> >
> > consider including the number of messages and principal.

Included the "number of messages", i.e., the capacity. Did not include the 
principal as I think it should be very clear to the framework what its own 
principal is (if it sets it in FrameworkInfo) and it could be empty (if it 
doesn't set it). "capacity" on the other hand is configured on the master so I 
think it's useful information.


- Jiang Yan


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


On Aug. 5, 2014, 10:53 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24343/
> ---
> 
> (Updated Aug. 5, 2014, 10:53 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1578
> https://issues.apache.org/jira/browse/MESOS-1578
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 6d4fd145004a14514e27ba65b2eae19b945aeb83 
>   src/examples/load_generator_framework.cpp 
> 7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
>   src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
>   src/master/master.cpp 9bede96083f56768a5d42baf051af97de4c48d48 
>   src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 
> 
> Diff: https://reviews.apache.org/r/24343/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> ./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

2014-08-05 Thread Dominic Hamon


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



src/master/master.hpp


is there a reason this is a nested class in Master?



src/master/master.hpp


might be worth adding the capacity to the error message to make it easier 
to debug issues.



src/master/master.cpp


please pull these out into more traditional if statements above the 'put' 
call. this is completely unreadable.



src/master/master.cpp


assuming there might be more errors in the future, this comment might be 
misleading.



src/master/master.cpp


maybe a const Option& here?



src/master/master.cpp


consider including the number of messages and principal.


- Dominic Hamon


On Aug. 5, 2014, 4:41 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24343/
> ---
> 
> (Updated Aug. 5, 2014, 4:41 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1578
> https://issues.apache.org/jira/browse/MESOS-1578
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 6d4fd145004a14514e27ba65b2eae19b945aeb83 
>   src/examples/load_generator_framework.cpp 
> 7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 
>   src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
>   src/master/master.cpp 9bede96083f56768a5d42baf051af97de4c48d48 
>   src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 
> 
> Diff: https://reviews.apache.org/r/24343/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> ./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

Re: Review Request 24343: Improved framework rate limiting by imposing the max number of outstanding messages per framework principal.

18 matches

Site Navigation

Mail list logo

Footer information