----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37993/#review100393 -----------------------------------------------------------
Looking almost perfect! Really appreciate that you diligently address all the comments I throw into you and don't give up. While you're touching this file, I would say it makes sense to clean up everything we can in one patch. Here are some suggestions: - Wrap all types and variables in comments in backticks; - As I've mentioned in one of my previous reviews, let's use the same tense everywhere in the commens for consistency; - Not a native speaker, but I think we should consolidate the use of articles when referencing allocator in comments. I think "an allocator" makes more sense since it is explicitly indicates that it's implementation dependent and not tied to a particular implementation; - Again, not a native speaker, but let's refer to a framework as "which" or "that", not "who". include/mesos/master/allocator.hpp (lines 45 - 51) <https://reviews.apache.org/r/37993/#comment157571> While you're touching this file, let's doxygenify this one as well. include/mesos/master/allocator.hpp (line 54) <https://reviews.apache.org/r/37993/#comment157570> I think you killed that line in previous versions of the review, why do you want to bring it back? include/mesos/master/allocator.hpp (lines 60 - 61) <https://reviews.apache.org/r/37993/#comment157569> Not yours, but let's wrap all types and variable names in backticks "`" include/mesos/master/allocator.hpp (lines 74 - 75) <https://reviews.apache.org/r/37993/#comment157583> Mind switching to Present Simple here for consistency? s/will be/is include/mesos/master/allocator.hpp (line 75) <https://reviews.apache.org/r/37993/#comment157572> I'm not sure `initialize()` can fail: it returns nothing and we do not throw exceptions. I see two possibilities here: - an allocator uses an exception, which is not caught in the master => master fails over; - an allocator uses `CHECK*` macros for error situations => master fails over. Could you please rephrase that in order not to mislead the reader? include/mesos/master/allocator.hpp (line 78) <https://reviews.apache.org/r/37993/#comment157584> s/perform the allocation/perform the batch allocation/ An allocator may also perform allocation based on events (a framework is added and so on). However, it's up to the implementation. include/mesos/master/allocator.hpp (lines 151 - 152) <https://reviews.apache.org/r/37993/#comment157591> I'm a bit hesitant to put a reference to the built-in allocator here. The behaviour you describe is correct, but 1) It's how the built-in allocator works 2) It's how the built-in allocator works now: when it the behaviour changes, my bet is that this comment won't be updated, hence it will become misleading. What do you think? include/mesos/master/allocator.hpp (line 159) <https://reviews.apache.org/r/37993/#comment157594> s/a/an include/mesos/master/allocator.hpp (line 161) <https://reviews.apache.org/r/37993/#comment157595> s/will be/is include/mesos/master/allocator.hpp (line 162) <https://reviews.apache.org/r/37993/#comment157596> s/new agent joins the Mesos cluster or in the case of a agent recovery./new agent joins the cluster or in case of agent recovery. not a native speaker though include/mesos/master/allocator.hpp (line 180) <https://reviews.apache.org/r/37993/#comment157597> s/a/an here and below. include/mesos/master/allocator.hpp (line 183) <https://reviews.apache.org/r/37993/#comment157598> s/should/are include/mesos/master/allocator.hpp (line 191) <https://reviews.apache.org/r/37993/#comment157599> s/a/an include/mesos/master/allocator.hpp (line 193) <https://reviews.apache.org/r/37993/#comment157600> What do you mean by "new" here? New feature or new update? include/mesos/master/allocator.hpp (line 208) <https://reviews.apache.org/r/37993/#comment157601> s/a/an here and below include/mesos/master/allocator.hpp (line 210) <https://reviews.apache.org/r/37993/#comment157602> s/will be/is s/when re-register a agent/when an agent reregisters include/mesos/master/allocator.hpp (line 213) <https://reviews.apache.org/r/37993/#comment157604> s/going to be/being include/mesos/master/allocator.hpp (line 219) <https://reviews.apache.org/r/37993/#comment157605> s/a/an here and below include/mesos/master/allocator.hpp (lines 221 - 223) <https://reviews.apache.org/r/37993/#comment157609> How about this: This is triggered if an agent disconnects from the master. Outstanding offers on a deactivated agent are removed and resources are recovered in a separate call. include/mesos/master/allocator.hpp (line 232) <https://reviews.apache.org/r/37993/#comment157610> How about this one, should be more readable: Updates a list of trusted agents include/mesos/master/allocator.hpp (lines 234 - 237) <https://reviews.apache.org/r/37993/#comment157611> How about this: It is invokved when the master starts up with a whitelist flag. In this case an allocator must allocate resources only to the hosts in the whitelist. include/mesos/master/allocator.hpp (line 239) <https://reviews.apache.org/r/37993/#comment157614> Or: A whitelist of agents that are allowed to contribute their resources to the allocation pool. (agents do not offer resources to frameworks directly, let's avoid misleading comments) include/mesos/master/allocator.hpp (line 245) <https://reviews.apache.org/r/37993/#comment157615> Suggestion: Requests resources for a framework. include/mesos/master/allocator.hpp (lines 247 - 250) <https://reviews.apache.org/r/37993/#comment157616> Suggestion: A framework may request resources via this call. It is up to an allocator how to react to this request. For example, a request may be ignored, or may influence internal priorities an allocator may keep for frameworks. include/mesos/master/allocator.hpp (line 260) <https://reviews.apache.org/r/37993/#comment157617> s/Update/Updates include/mesos/master/allocator.hpp (lines 260 - 265) <https://reviews.apache.org/r/37993/#comment157618> Suggestion: Updates allocation by applying offer operations. This call is mainly intended to support persistence-related features (dynamic reservationa and persistent volumes). An allocator may react differently for certain offer operations, it may use this call to update bookkeeping information related to the framework. include/mesos/master/allocator.hpp (line 267) <https://reviews.apache.org/r/37993/#comment157621> s/who has just got resources./which reacted to an offer. include/mesos/master/allocator.hpp (lines 270 - 271) <https://reviews.apache.org/r/37993/#comment157623> Again, I'm afraid the next engineer to update the set of offer operations will forget to update this list. Do you think it's valuable to keep this list here explicitly? include/mesos/master/allocator.hpp (line 279) <https://reviews.apache.org/r/37993/#comment157629> Let's kill this for now, it doesn't really add extra information. include/mesos/master/allocator.hpp (line 291) <https://reviews.apache.org/r/37993/#comment157630> s/time/interval include/mesos/master/allocator.hpp (line 299) <https://reviews.apache.org/r/37993/#comment157631> Unfinished sentence? include/mesos/master/allocator.hpp (lines 355 - 361) <https://reviews.apache.org/r/37993/#comment157633> Suggestion: Revives offers for a framework. It is invoked when a framework that has filtered resources before wants to revive offers for those resources. @param frameworkId ID of the framework which wants to revive offers. include/mesos/master/allocator.hpp (lines 366 - 369) <https://reviews.apache.org/r/37993/#comment157632> It's resolved, isn't it? - Alexander Rukletsov On Sept. 21, 2015, 7:28 a.m., Guangya Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37993/ > ----------------------------------------------------------- > > (Updated Sept. 21, 2015, 7:28 a.m.) > > > Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park. > > > Bugs: MESOS-2224 > https://issues.apache.org/jira/browse/MESOS-2224 > > > Repository: mesos > > > Description > ------- > > Add explanatory comments for Allocator interface > > > Diffs > ----- > > include/mesos/master/allocator.hpp 3fea47ffcc69531308068e2701502e481605b912 > > Diff: https://reviews.apache.org/r/37993/diff/ > > > Testing > ------- > > > Thanks, > > Guangya Liu > >
