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/06 22:17:10 UTC
Review Request 22316: Added "per-framework-principal" counters for messages
from a scheduler to the master.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/
-----------------------------------------------------------
Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
Bugs: MESOS-1339
https://issues.apache.org/jira/browse/MESOS-1339
Repository: mesos-git
Description
-------
- Multiple frameworks use the same principal use the same counter.
- The design is shared by rate limiters
- For more on the design see https://cwiki.apache.org/confluence/display/MESOS/Framework+API+Rate+Limiting+Design+Document#FrameworkAPIRateLimitingDesignDocument-RateLimitinginMaster which I am updating right now.
Diffs
-----
src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
src/tests/fault_tolerance_tests.cpp 4c6a5c4ecd42a5d0bb4b4d69ab2cd38f842edf7a
src/tests/master_tests.cpp 34df12195d7bbdf77fb6079aa3ad644264b8cfa5
Diff: https://reviews.apache.org/r/22316/diff/
Testing
-------
make check
Thanks,
Jiang Yan Xu
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review44972
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment79582>
There is a bug here: process::metric::remove() should be called only when this framework is the last one that uses with this principal (not when removing each framework)
I want to hold off submitting a new patch and until we agree on the use of these data structures in Master
- Jiang Yan Xu
On June 6, 2014, 1:17 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 6, 2014, 1:17 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
> - The design is shared by rate limiters
> - For more on the design see https://cwiki.apache.org/confluence/display/MESOS/Framework+API+Rate+Limiting+Design+Document#FrameworkAPIRateLimitingDesignDocument-RateLimitinginMaster which I am updating right now.
>
>
> Diffs
> -----
>
> src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
> src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
> src/tests/fault_tolerance_tests.cpp 4c6a5c4ecd42a5d0bb4b4d69ab2cd38f842edf7a
> src/tests/master_tests.cpp 34df12195d7bbdf77fb6079aa3ad644264b8cfa5
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 6, 2014, 2:21 p.m., Dominic Hamon wrote:
> > src/master/master.cpp, lines 3278-3296
> > <https://reviews.apache.org/r/22316/diff/1/?file=604889#file604889line3278>
> >
> > ownership is very confusing here.
> >
> > In the first stanza you set the metrics.framework_messages entry to reference the frameworks.messageCounters Counter.
> >
> > In the second stanza, the frameworks.messageCounters entry references the metrics.framework_messages Counter.
> >
> > Instead, consider having a mapping from PID to principal, and a mapping from principal to counter.
That's the alternative approach I have considered. It has the following problems:
1) For every message two lookups are required from UPID -> principal -> counter. But the performance overhead is admittedly small.
2) When to remove the entry from principal -> counter map? (i.e. when do we know the framework to be removed is the last framework that uses this principal, thus the counter needs to be deleted?)
I do realize that I can avoid using an auxiliary map and just scan the values of (UPID, counter*) map and use counter->name() to check
1) if this framework is the first one with this principal (during addFramework)
2) if this framework is the last one with this principal (during removeFramework)
This way it becomes a O(n) operation but the 'n' here is small in most cases.
Since the same logic is used to keep track of RateLimiters and they don't have names, I will need to use pointer equality for them (and probably for counters too).
How do these approach sound?
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review44965
-----------------------------------------------------------
On June 6, 2014, 1:17 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 6, 2014, 1:17 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
> - The design is shared by rate limiters
> - For more on the design see https://cwiki.apache.org/confluence/display/MESOS/Framework+API+Rate+Limiting+Design+Document#FrameworkAPIRateLimitingDesignDocument-RateLimitinginMaster which I am updating right now.
>
>
> Diffs
> -----
>
> src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
> src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
> src/tests/fault_tolerance_tests.cpp 4c6a5c4ecd42a5d0bb4b4d69ab2cd38f842edf7a
> src/tests/master_tests.cpp 34df12195d7bbdf77fb6079aa3ad644264b8cfa5
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On June 6, 2014, 2:21 p.m., Dominic Hamon wrote:
> > src/master/master.cpp, lines 3278-3296
> > <https://reviews.apache.org/r/22316/diff/1/?file=604889#file604889line3278>
> >
> > ownership is very confusing here.
> >
> > In the first stanza you set the metrics.framework_messages entry to reference the frameworks.messageCounters Counter.
> >
> > In the second stanza, the frameworks.messageCounters entry references the metrics.framework_messages Counter.
> >
> > Instead, consider having a mapping from PID to principal, and a mapping from principal to counter.
>
> Jiang Yan Xu wrote:
> That's the alternative approach I have considered. It has the following problems:
>
> 1) For every message two lookups are required from UPID -> principal -> counter. But the performance overhead is admittedly small.
> 2) When to remove the entry from principal -> counter map? (i.e. when do we know the framework to be removed is the last framework that uses this principal, thus the counter needs to be deleted?)
>
> I do realize that I can avoid using an auxiliary map and just scan the values of (UPID, counter*) map and use counter->name() to check
> 1) if this framework is the first one with this principal (during addFramework)
> 2) if this framework is the last one with this principal (during removeFramework)
>
> This way it becomes a O(n) operation but the 'n' here is small in most cases.
>
> Since the same logic is used to keep track of RateLimiters and they don't have names, I will need to use pointer equality for them (and probably for counters too).
>
> How do these approach sound?
>
>
>
Given N pids per principal and 1 counter per principal, I think the following should work:
map[pid] -> principal
map[principal] -> pair(pid_count, counter)
on add framework, if principal is new, create and add the counter and set pid_count to 1.
on remove framework, decrement the pid_count and if 0 remove and destroy the counter.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review44965
-----------------------------------------------------------
On June 6, 2014, 1:17 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 6, 2014, 1:17 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
> - The design is shared by rate limiters
> - For more on the design see https://cwiki.apache.org/confluence/display/MESOS/Framework+API+Rate+Limiting+Design+Document#FrameworkAPIRateLimitingDesignDocument-RateLimitinginMaster which I am updating right now.
>
>
> Diffs
> -----
>
> src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
> src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
> src/tests/fault_tolerance_tests.cpp 4c6a5c4ecd42a5d0bb4b4d69ab2cd38f842edf7a
> src/tests/master_tests.cpp 34df12195d7bbdf77fb6079aa3ad644264b8cfa5
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 6, 2014, 2:21 p.m., Dominic Hamon wrote:
> > src/master/master.cpp, lines 3278-3296
> > <https://reviews.apache.org/r/22316/diff/1/?file=604889#file604889line3278>
> >
> > ownership is very confusing here.
> >
> > In the first stanza you set the metrics.framework_messages entry to reference the frameworks.messageCounters Counter.
> >
> > In the second stanza, the frameworks.messageCounters entry references the metrics.framework_messages Counter.
> >
> > Instead, consider having a mapping from PID to principal, and a mapping from principal to counter.
>
> Jiang Yan Xu wrote:
> That's the alternative approach I have considered. It has the following problems:
>
> 1) For every message two lookups are required from UPID -> principal -> counter. But the performance overhead is admittedly small.
> 2) When to remove the entry from principal -> counter map? (i.e. when do we know the framework to be removed is the last framework that uses this principal, thus the counter needs to be deleted?)
>
> I do realize that I can avoid using an auxiliary map and just scan the values of (UPID, counter*) map and use counter->name() to check
> 1) if this framework is the first one with this principal (during addFramework)
> 2) if this framework is the last one with this principal (during removeFramework)
>
> This way it becomes a O(n) operation but the 'n' here is small in most cases.
>
> Since the same logic is used to keep track of RateLimiters and they don't have names, I will need to use pointer equality for them (and probably for counters too).
>
> How do these approach sound?
>
>
>
>
> Dominic Hamon wrote:
> Given N pids per principal and 1 counter per principal, I think the following should work:
>
> map[pid] -> principal
> map[principal] -> pair(pid_count, counter)
>
> on add framework, if principal is new, create and add the counter and set pid_count to 1.
> on remove framework, decrement the pid_count and if 0 remove and destroy the counter.
Thanks. I originally thought these smart pointer basically do this "pid_count" counting for us. I found it nifty, probably needs some explanation but not counterintuitive. (weak_ptrs by definition don't claim ownership and share_ptr indicates these counters are "shared" and their shared ownership stays within metrics.framework_messages).
I agree that to optimize for simplicity we can maintain "pid_count" ourselves. If no objections from other reviews I can submit a new review per Dominic's suggestion.
P.S. I communicated with Dominic and BenM offline but to clarify:
1) These maps don't store "copies" of the same counter because maps require Counter to have a default constructor which doesn't make sense here.
2) If we increment the counter in specific framework message handlers instead of Master::visit(), we only need a map[principal] -> counter. But since it is used with the framework rate limiter (which are yet to be added) and they have similar logic, their implementation should be symmetric. Rate limiters are checked in Master::visit() so I think the counters should be manipulated in visit() too.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review44965
-----------------------------------------------------------
On June 6, 2014, 1:17 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 6, 2014, 1:17 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
> - The design is shared by rate limiters
> - For more on the design see https://cwiki.apache.org/confluence/display/MESOS/Framework+API+Rate+Limiting+Design+Document#FrameworkAPIRateLimitingDesignDocument-RateLimitinginMaster which I am updating right now.
>
>
> Diffs
> -----
>
> src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
> src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
> src/tests/fault_tolerance_tests.cpp 4c6a5c4ecd42a5d0bb4b4d69ab2cd38f842edf7a
> src/tests/master_tests.cpp 34df12195d7bbdf77fb6079aa3ad644264b8cfa5
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review44965
-----------------------------------------------------------
src/master/master.hpp
<https://reviews.apache.org/r/22316/#comment79571>
See below for more on this, but the use of weak_ptr here and shared_ptr below hints at confused ownership semantics. There should be a single owner for a Counter and other sources of information should be directed to that owner.
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment79574>
Consider commenting here about the case when we receive something from an inactive framework/PID.
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment79575>
ownership is very confusing here.
In the first stanza you set the metrics.framework_messages entry to reference the frameworks.messageCounters Counter.
In the second stanza, the frameworks.messageCounters entry references the metrics.framework_messages Counter.
Instead, consider having a mapping from PID to principal, and a mapping from principal to counter.
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment79577>
consider checking the return of add
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment79576>
consider checking the return of remove
- Dominic Hamon
On June 6, 2014, 1:17 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 6, 2014, 1:17 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
> - The design is shared by rate limiters
> - For more on the design see https://cwiki.apache.org/confluence/display/MESOS/Framework+API+Rate+Limiting+Design+Document#FrameworkAPIRateLimitingDesignDocument-RateLimitinginMaster which I am updating right now.
>
>
> Diffs
> -----
>
> src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
> src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
> src/tests/fault_tolerance_tests.cpp 4c6a5c4ecd42a5d0bb4b4d69ab2cd38f842edf7a
> src/tests/master_tests.cpp 34df12195d7bbdf77fb6079aa3ad644264b8cfa5
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review45034
-----------------------------------------------------------
Patch looks great!
Reviews applied: [22250, 22316]
All tests passed.
- Mesos ReviewBot
On June 6, 2014, 8:17 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 6, 2014, 8:17 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
> - The design is shared by rate limiters
> - For more on the design see https://cwiki.apache.org/confluence/display/MESOS/Framework+API+Rate+Limiting+Design+Document#FrameworkAPIRateLimitingDesignDocument-RateLimitinginMaster which I am updating right now.
>
>
> Diffs
> -----
>
> src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
> src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
> src/tests/fault_tolerance_tests.cpp 4c6a5c4ecd42a5d0bb4b4d69ab2cd38f842edf7a
> src/tests/master_tests.cpp 34df12195d7bbdf77fb6079aa3ad644264b8cfa5
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review44994
-----------------------------------------------------------
This patch is implicitly designed on the fact that rate limiting is what motivates this. In that, if we were to implement the counters on their own, we could just have a single map<principal, Counter> and count in each message handler.
Since the complexity here arises from rate limiting, I would suggest we either:
(1) Implement the limiting first (solve the issues being discussed in this review)
(2) Or we should do the counting first, ignoring the fact that rate limiting is coming soon. This means a much simpler patch here and then circling back to update the counters.
Up to you Yan!
- Ben Mahler
On June 6, 2014, 8:17 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 6, 2014, 8:17 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
> - The design is shared by rate limiters
> - For more on the design see https://cwiki.apache.org/confluence/display/MESOS/Framework+API+Rate+Limiting+Design+Document#FrameworkAPIRateLimitingDesignDocument-RateLimitinginMaster which I am updating right now.
>
>
> Diffs
> -----
>
> src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
> src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
> src/tests/fault_tolerance_tests.cpp 4c6a5c4ecd42a5d0bb4b4d69ab2cd38f842edf7a
> src/tests/master_tests.cpp 34df12195d7bbdf77fb6079aa3ad644264b8cfa5
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 11, 2014, 10:55 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 3316-3317
> > <https://reviews.apache.org/r/22316/diff/2/?file=606512#file606512line3316>
> >
> > Why is this Owned and on a heap?
It can't be an plain object because Counter doesn't have a default constructor thus cannot be put into a map.
It could be a raw pointer but I wrapped it with a Owned because these Counters do only live in the map and not copied anywhere.
With Owned I don't need to delete it explicitly and I don't think we need to worry about lifecycle issues here because it's not passed around.
> On June 11, 2014, 10:55 a.m., Vinod Kone wrote:
> > src/master/master.cpp, line 3343
> > <https://reviews.apache.org/r/22316/diff/2/?file=606512#file606512line3343>
> >
> > why the change?
It was a actually a bug to use a reference because we modify the value of framework->pid below ("framework->pid = newPid;"). We just never used the oldPid after the value change.
So I wanted to make a copy. I changed it now to be "const UPID oldPid = framework->pid;"
> On June 11, 2014, 10:55 a.m., Vinod Kone wrote:
> > src/master/master.cpp, line 3399
> > <https://reviews.apache.org/r/22316/diff/2/?file=606512#file606512line3399>
> >
> > const&?
Added const but not ref. I erase this key from the map below.
> On June 11, 2014, 10:55 a.m., Vinod Kone wrote:
> > src/tests/rate_limiting_tests.cpp, lines 81-93
> > <https://reviews.apache.org/r/22316/diff/2/?file=606513#file606513line81>
> >
> > I've seen this block in various tests. We should probably have a helper for this. Maybe a TODO for now.
Added a TODO but it's inconvenient to do because there are AWAIT_* and ASSERT_* that should live inside a TEST. Maybe a helper MACRO.
> On June 11, 2014, 10:55 a.m., Vinod Kone wrote:
> > src/tests/rate_limiting_tests.cpp, line 371
> > <https://reviews.apache.org/r/22316/diff/2/?file=606513#file606513line371>
> >
> > why will there be subsequent registered callbacks?
You are right. They are ignored.
> On June 11, 2014, 10:55 a.m., Vinod Kone wrote:
> > src/tests/rate_limiting_tests.cpp, line 419
> > <https://reviews.apache.org/r/22316/diff/2/?file=606513#file606513line419>
> >
> > const&
Fixed these but I didn't use const or & because they are short-lived (goes out of scope 8 lines below) and there is actually no copying. But in general I agree having a const is nice.
> On June 11, 2014, 10:55 a.m., Vinod Kone wrote:
> > src/tests/rate_limiting_tests.cpp, line 469
> > <https://reviews.apache.org/r/22316/diff/2/?file=606513#file606513line469>
> >
> > This comment should be pulled down. Here you are just sending a dup.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review45320
-----------------------------------------------------------
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/22316/
> -----------------------------------------------------------
>
> (Updated June 11, 2014, 10:39 a.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438c3401ff815116d6faab4f0a34bc0fe3fd
> src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
> src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
> src/tests/rate_limiting_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review45320
-----------------------------------------------------------
src/master/master.hpp
<https://reviews.apache.org/r/22316/#comment80131>
we allow non-authenticated but registered with principal right?
src/master/master.hpp
<https://reviews.apache.org/r/22316/#comment80132>
s/an//
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80137>
pull this outside to the top of the if block?
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80207>
Why is this Owned and on a heap?
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80138>
why the change?
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80139>
const&?
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80140>
s/must/must be/
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80221>
To simplify this can we just do
if (frameworks.principals.contains(framework->pid)) {
}
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22316/#comment80225>
s/is/are/
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22316/#comment80226>
I've seen this block in various tests. We should probably have a helper for this. Maybe a TODO for now.
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22316/#comment80227>
s/multiple/different/
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22316/#comment80229>
s/the framework/the first framework/
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22316/#comment80230>
why will there be subsequent registered callbacks?
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22316/#comment80232>
kill "and to make sure..." because you AWAIT on frameworkId for that.
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22316/#comment80234>
const&
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22316/#comment80235>
const &
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22316/#comment80237>
This comment should be pulled down. Here you are just sending a dup.
- Vinod Kone
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/22316/
> -----------------------------------------------------------
>
> (Updated June 11, 2014, 5:39 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438c3401ff815116d6faab4f0a34bc0fe3fd
> src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
> src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
> src/tests/rate_limiting_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review45487
-----------------------------------------------------------
Patch looks great!
Reviews applied: [22316]
All tests passed.
- Mesos ReviewBot
On June 11, 2014, 8:49 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 11, 2014, 8:49 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438c3401ff815116d6faab4f0a34bc0fe3fd
> src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
> src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
> src/tests/rate_limiting_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review45428
-----------------------------------------------------------
>From the review meeting with Vinod and BenM.
src/master/master.hpp
<https://reviews.apache.org/r/22316/#comment80288>
delete 'that'
src/master/master.hpp
<https://reviews.apache.org/r/22316/#comment80292>
Create a struct to group theses and have a comment about how they are exported differently
Also no need to use Owned
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80301>
Instead of checking UPID -> principal mapping, should check if the counter still exists. If yes, increment it.
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80293>
Explain why checking two sources for the principal.
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80295>
Use Option<string> principal.
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80294>
If framework principal is not specified, we don't export its message counters.
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80296>
Expand on this.
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80297>
(i.e. explain 'failover')
- Jiang Yan Xu
On June 11, 2014, 1:49 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 11, 2014, 1:49 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438c3401ff815116d6faab4f0a34bc0fe3fd
> src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
> src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
> src/tests/rate_limiting_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review45879
-----------------------------------------------------------
Ship it!
src/master/master.hpp
<https://reviews.apache.org/r/22316/#comment80939>
s/Note/NOTE/
src/master/master.hpp
<https://reviews.apache.org/r/22316/#comment80938>
s/doesn't require it/allows them/
src/master/master.hpp
<https://reviews.apache.org/r/22316/#comment80942>
s/Messages/messages/
src/master/master.hpp
<https://reviews.apache.org/r/22316/#comment80940>
s/Note/NOTE/
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80946>
quotes around Master::Metrics::Frameworks?
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80947>
single quotes?
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80948>
s/. i.e. Change/, i.e., change/
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80949>
why did you need to extract this into temp?
- Vinod Kone
On June 17, 2014, 12:17 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 17, 2014, 12:17 a.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
>
>
> Diffs
> -----
>
> src/Makefile.am 3e623cc0137ea4ae9113f9b47a4fc88281ee96c1
> src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
> src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
> src/tests/rate_limiting_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review45992
-----------------------------------------------------------
Patch looks great!
Reviews applied: [22316]
All tests passed.
- Mesos ReviewBot
On June 17, 2014, 8:55 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 17, 2014, 8:55 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks with the same principal use the same counter.
>
>
> Diffs
> -----
>
> src/Makefile.am 7ae67604ea1a54f846cb24cbf7735078183bb4b6
> src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
> src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
> src/tests/rate_limiting_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/
-----------------------------------------------------------
(Updated June 17, 2014, 1:55 p.m.)
Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
Changes
-------
Vinods'. NNFR.
Bugs: MESOS-1339
https://issues.apache.org/jira/browse/MESOS-1339
Repository: mesos-git
Description (updated)
-------
- Multiple frameworks with the same principal use the same counter.
Diffs (updated)
-----
src/Makefile.am 7ae67604ea1a54f846cb24cbf7735078183bb4b6
src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
src/tests/rate_limiting_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22316/diff/
Testing
-------
make check
Thanks,
Jiang Yan Xu
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review45872
-----------------------------------------------------------
Ship it!
Ship It!
- Dominic Hamon
On June 16, 2014, 5:17 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 16, 2014, 5:17 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
>
>
> Diffs
> -----
>
> src/Makefile.am 3e623cc0137ea4ae9113f9b47a4fc88281ee96c1
> src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
> src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
> src/tests/rate_limiting_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/
-----------------------------------------------------------
(Updated June 16, 2014, 5:17 p.m.)
Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
Changes
-------
Dominic's comments and rebase.
Bugs: MESOS-1339
https://issues.apache.org/jira/browse/MESOS-1339
Repository: mesos-git
Description
-------
- Multiple frameworks use the same principal use the same counter.
Diffs (updated)
-----
src/Makefile.am 3e623cc0137ea4ae9113f9b47a4fc88281ee96c1
src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
src/tests/rate_limiting_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22316/diff/
Testing
-------
make check
Thanks,
Jiang Yan Xu
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review45668
-----------------------------------------------------------
src/master/master.hpp
<https://reviews.apache.org/r/22316/#comment80568>
I ended up still using Owned<Frameworks> because with metrics addition and removal being handled in ctor/dtor of this "struct Frameworks", copying and destructing of temporary Frameworks object causes unwanted behavior. Using a pointer makes sure they are created and registered once.
- Jiang Yan Xu
On June 13, 2014, 3:48 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 13, 2014, 3:48 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438c3401ff815116d6faab4f0a34bc0fe3fd
> src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
> src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
> src/tests/rate_limiting_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 16, 2014, 10:39 a.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 763
> > <https://reviews.apache.org/r/22316/diff/5/?file=609679#file609679line763>
> >
> > is this simpler?
> >
> > metrics.frameworks.get(principal.get()).get()->messages_received++;
> >
> >
I made a copy here because otherwise I would be incrementing a member of a readonly object. frameworks.get() returns an Option, whose get() returns a "const T&".
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review45779
-----------------------------------------------------------
On June 13, 2014, 3:48 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 13, 2014, 3:48 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438c3401ff815116d6faab4f0a34bc0fe3fd
> src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
> src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
> src/tests/rate_limiting_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review45779
-----------------------------------------------------------
src/master/master.hpp
<https://reviews.apache.org/r/22316/#comment80757>
const std::string& ?
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80758>
is this simpler?
metrics.frameworks.get(principal.get()).get()->messages_received++;
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80759>
ditto above re not creating a copy.
- Dominic Hamon
On June 13, 2014, 3:48 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 13, 2014, 3:48 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438c3401ff815116d6faab4f0a34bc0fe3fd
> src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
> src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
> src/tests/rate_limiting_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/
-----------------------------------------------------------
(Updated June 13, 2014, 3:48 p.m.)
Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
Changes
-------
Comments from review meeting.
Bugs: MESOS-1339
https://issues.apache.org/jira/browse/MESOS-1339
Repository: mesos-git
Description
-------
- Multiple frameworks use the same principal use the same counter.
Diffs (updated)
-----
src/Makefile.am c91b438c3401ff815116d6faab4f0a34bc0fe3fd
src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
src/tests/rate_limiting_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22316/diff/
Testing
-------
make check
Thanks,
Jiang Yan Xu
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/
-----------------------------------------------------------
(Updated June 11, 2014, 1:49 p.m.)
Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
Changes
-------
Vinod's comments.
Bugs: MESOS-1339
https://issues.apache.org/jira/browse/MESOS-1339
Repository: mesos-git
Description
-------
- Multiple frameworks use the same principal use the same counter.
Diffs (updated)
-----
src/Makefile.am c91b438c3401ff815116d6faab4f0a34bc0fe3fd
src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
src/tests/rate_limiting_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22316/diff/
Testing
-------
make check
Thanks,
Jiang Yan Xu
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 11, 2014, 10:57 a.m., Vinod Kone wrote:
> > src/tests/rate_limiting_tests.cpp, lines 512-513
> > <https://reviews.apache.org/r/22316/diff/3/?file=607141#file607141line512>
> >
> > there is no throttling because rate limiting is not setup correct?
Yeah I meant to add it to the framework throttling case. Killed it.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review45400
-----------------------------------------------------------
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/22316/
> -----------------------------------------------------------
>
> (Updated June 11, 2014, 10:39 a.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438c3401ff815116d6faab4f0a34bc0fe3fd
> src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
> src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
> src/tests/rate_limiting_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review45400
-----------------------------------------------------------
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22316/#comment80239>
there is no throttling because rate limiting is not setup correct?
- Vinod Kone
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/22316/
> -----------------------------------------------------------
>
> (Updated June 11, 2014, 5:39 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438c3401ff815116d6faab4f0a34bc0fe3fd
> src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
> src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
> src/tests/rate_limiting_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/
-----------------------------------------------------------
(Updated June 11, 2014, 10:39 a.m.)
Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
Changes
-------
Comments.
Bugs: MESOS-1339
https://issues.apache.org/jira/browse/MESOS-1339
Repository: mesos-git
Description
-------
- Multiple frameworks use the same principal use the same counter.
Diffs (updated)
-----
src/Makefile.am c91b438c3401ff815116d6faab4f0a34bc0fe3fd
src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
src/tests/rate_limiting_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22316/diff/
Testing
-------
make check
Thanks,
Jiang Yan Xu
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 10, 2014, 11:30 a.m., Dominic Hamon wrote:
> > src/master/master.hpp, line 530
> > <https://reviews.apache.org/r/22316/diff/2/?file=606511#file606511line530>
> >
> > Are we going to add more of these? It might make sense to combine them:
> >
> > struct PerFrameworkPrincipalMetrics
> > {
> > process::metrics::Counter received;
> > process::metrics::Counter processed;
> > };
> >
> > hashmap<std::string, process::Owned<PerFrameworkPrincipalMetrics> > perFrameworkPrincipalMetrics;
> >
> > what do you think?
None that I can think of.
This feels nice but with only two of them it's a little odd for it to look much different from everything else, especially
std::vector<process::metrics::Gauge> resources_total;
std::vector<process::metrics::Gauge> resources_used;
std::vector<process::metrics::Gauge> resources_percent;
Maybe refactor later when there are more?
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review45261
-----------------------------------------------------------
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/22316/
> -----------------------------------------------------------
>
> (Updated June 11, 2014, 10:39 a.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438c3401ff815116d6faab4f0a34bc0fe3fd
> src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
> src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
> src/tests/rate_limiting_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On June 10, 2014, 11:30 a.m., Dominic Hamon wrote:
> > src/master/master.hpp, line 530
> > <https://reviews.apache.org/r/22316/diff/2/?file=606511#file606511line530>
> >
> > Are we going to add more of these? It might make sense to combine them:
> >
> > struct PerFrameworkPrincipalMetrics
> > {
> > process::metrics::Counter received;
> > process::metrics::Counter processed;
> > };
> >
> > hashmap<std::string, process::Owned<PerFrameworkPrincipalMetrics> > perFrameworkPrincipalMetrics;
> >
> > what do you think?
>
> Jiang Yan Xu wrote:
> None that I can think of.
> This feels nice but with only two of them it's a little odd for it to look much different from everything else, especially
>
> std::vector<process::metrics::Gauge> resources_total;
> std::vector<process::metrics::Gauge> resources_used;
> std::vector<process::metrics::Gauge> resources_percent;
>
> Maybe refactor later when there are more?
This is a little different as they're being accessed using the same key and they should be in sync. It simplifies some of the CHECKs in the code too. Up to you though, I won't insist.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review45261
-----------------------------------------------------------
On June 11, 2014, 1:49 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 11, 2014, 1:49 p.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438c3401ff815116d6faab4f0a34bc0fe3fd
> src/master/master.hpp 7a121856806ff2d79661fa31e491ddd6e4512c59
> src/master/master.cpp 4a01b1aacfff83c62aa2ec3b46ad64e631488d09
> src/tests/rate_limiting_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/#review45261
-----------------------------------------------------------
src/Makefile.am
<https://reviews.apache.org/r/22316/#comment79996>
whitespace .. you need a tab.
src/master/master.hpp
<https://reviews.apache.org/r/22316/#comment79997>
Are we going to add more of these? It might make sense to combine them:
struct PerFrameworkPrincipalMetrics
{
process::metrics::Counter received;
process::metrics::Counter processed;
};
hashmap<std::string, process::Owned<PerFrameworkPrincipalMetrics> > perFrameworkPrincipalMetrics;
what do you think?
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment79998>
this does a double look-up. Can you use:
Option<string> principal = frameworks.principals.get(event.message->from);
if (principal.isSome()) {
...
}
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment79999>
see above re: double lookup.
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80000>
const string?
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80001>
s/and but/but/
src/master/master.cpp
<https://reviews.apache.org/r/22316/#comment80002>
s/its the/the/
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22316/#comment80003>
nit: s/is/are/
src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/22316/#comment80004>
to be complete, you should check that the metrics aren't already added before the framework is registered.
- Dominic Hamon
On June 10, 2014, 11:04 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22316/
> -----------------------------------------------------------
>
> (Updated June 10, 2014, 11:04 a.m.)
>
>
> Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1339
> https://issues.apache.org/jira/browse/MESOS-1339
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Multiple frameworks use the same principal use the same counter.
>
>
> Diffs
> -----
>
> src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60
> src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f
> src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613
> src/tests/rate_limiting_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22316/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22316: Added "per-framework-principal" counters for
messages from a scheduler to the master.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22316/
-----------------------------------------------------------
(Updated June 10, 2014, 11:04 a.m.)
Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone.
Changes
-------
- Add one more counter, now we have
frameworks/principal/messages_received
frameworks/principal/messages_processed
- Revised the implemented to use these two maps to locate the counters.
map[UPID] -> principal
map[principal] -> counter
- Moved tests to a separate file where rate limiting tests will also be added (either as separate tests or added to the counters tests)
Bugs: MESOS-1339
https://issues.apache.org/jira/browse/MESOS-1339
Repository: mesos-git
Description (updated)
-------
- Multiple frameworks use the same principal use the same counter.
Diffs (updated)
-----
src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60
src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f
src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613
src/tests/rate_limiting_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22316/diff/
Testing
-------
make check
Thanks,
Jiang Yan Xu