----------------------------------------------------------- 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 <https://reviews.apache.org/r/24343/#comment87432> 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 <https://reviews.apache.org/r/24343/#comment87436> 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 <https://reviews.apache.org/r/24343/#comment87439> 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 <https://reviews.apache.org/r/24343/#comment87435> 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 > >
