> 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 > > <https://reviews.apache.org/r/24343/diff/6/?file=654370#file654370line778> > > > > 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 > > <https://reviews.apache.org/r/24343/diff/6/?file=654370#file654370line762> > > > > 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 > > <https://reviews.apache.org/r/24343/diff/6/?file=654371#file654371line944> > > > > 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 > > <https://reviews.apache.org/r/24343/diff/6/?file=654371#file654371line940> > > > > 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 with the naming scheme but I think we started to do this when it was clear that we were going to start using lambdas soon to replace these continuation whenever appropriate. And all these methods groups are sequential that they "would have been written as one if it weren't because of asynchronous" I'd love to revisit this pattern later when we refactor. In my case there are three alternative flows so it's not super clear prepending underscores helps with clarity but at least in the current implementation both _visit(MessageEvent) and _visit(ExitedEvent) are the 'real' message "visit"s and "throttled" merely does accounting so there is some consistency there. See my comments above about EventLimiter. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24343/#review49979 ----------------------------------------------------------- On Aug. 6, 2014, 11:26 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, 11:26 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 > >
