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