> 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 > >
