----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37993/#review100628 -----------------------------------------------------------
Per AlexR, I'll review as a native speaker. General note: There are quite a few run-on sentences (i.e. using commas instead of periods). This is fairly common in chinese, but most english word processors will point this out automatically :P include/mesos/master/allocator.hpp (line 45) <https://reviews.apache.org/r/37993/#comment157847> Add a newline after `/**` (Inconsistent style.) include/mesos/master/allocator.hpp (line 51) <https://reviews.apache.org/r/37993/#comment157848> s/a/an/ include/mesos/master/allocator.hpp (lines 75 - 78) <https://reviews.apache.org/r/37993/#comment157854> Suggestion: Any errors in initialization should fail fast and result in an `ABORT`. The master expects the allocator to be successfully initialized if this call returns. include/mesos/master/allocator.hpp (lines 86 - 88) <https://reviews.apache.org/r/37993/#comment157856> The roles are actually checked by the master (see `Master::subscribe`). Instead, you could replace the second sentence with: All frameworks that are added to the allocator will fall into one of these roles. include/mesos/master/allocator.hpp (line 104) <https://reviews.apache.org/r/37993/#comment157858> Re-adding a framework will not call this. See: https://github.com/apache/mesos/blob/master/src/master/master.cpp#L5370-L5371 include/mesos/master/allocator.hpp (lines 108 - 110) <https://reviews.apache.org/r/37993/#comment157861> Note that this field is non-zero when the master fails-over. The frameworks report that they are using resources to the recovered master and that gets passed to the allocator. Suggestion: Resources used this framework. The allocator should account for these resources when updating the allocation of this framework. include/mesos/master/allocator.hpp (lines 130 - 131) <https://reviews.apache.org/r/37993/#comment157862> Suggestion: Activates a framework in the Mesos cluster. Offers are only sent to active frameworks. include/mesos/master/allocator.hpp (lines 139 - 140) <https://reviews.apache.org/r/37993/#comment157863> Suggestion: Deactivates a framework in the Mesos cluster. Resource offers are not sent to deactivated frameworks. include/mesos/master/allocator.hpp (line 150) <https://reviews.apache.org/r/37993/#comment157866> s/can/should/ include/mesos/master/allocator.hpp (line 152) <https://reviews.apache.org/r/37993/#comment157865> I think this doc is a better reference than the JIRA: https://cwiki.apache.org/confluence/display/MESOS/Design+doc:+Updating+Framework+Info (If it's too long for one line, add ` // NOLINT` at the end.) include/mesos/master/allocator.hpp (lines 154 - 156) <https://reviews.apache.org/r/37993/#comment157867> `@param` not really necessary. include/mesos/master/allocator.hpp (line 168) <https://reviews.apache.org/r/37993/#comment157869> Remove "going". include/mesos/master/allocator.hpp (line 169) <https://reviews.apache.org/r/37993/#comment157870> Period after "Detailed info of the agent". include/mesos/master/allocator.hpp (line 171) <https://reviews.apache.org/r/37993/#comment157872> s/'total'/`total`/ include/mesos/master/allocator.hpp (lines 186 - 187) <https://reviews.apache.org/r/37993/#comment157875> Suggestion: Removes an agent from the Mesos cluster. All resources belonging to this agent should be released by the allocator. include/mesos/master/allocator.hpp (lines 188 - 189) <https://reviews.apache.org/r/37993/#comment157874> Not really necessary. include/mesos/master/allocator.hpp (line 202) <https://reviews.apache.org/r/37993/#comment157876> Not really necessary. include/mesos/master/allocator.hpp (lines 203 - 205) <https://reviews.apache.org/r/37993/#comment157877> Split the comment into two sentences: s/,/./ include/mesos/master/allocator.hpp (lines 214 - 215) <https://reviews.apache.org/r/37993/#comment157878> Suggestion: Activates an agent. This is invoked when an agent reregisters. Offers are only sent for activated agents. include/mesos/master/allocator.hpp (lines 216 - 217) <https://reviews.apache.org/r/37993/#comment157879> Not necessary. include/mesos/master/allocator.hpp (lines 225 - 227) <https://reviews.apache.org/r/37993/#comment157887> The second sentence is incorrect: * The allocator should treat all offers from the deactivated agent as rescinded. (There is no separate call to the allocator to handle this). * Resources aren't "recovered" when an agent deactivates (because the resources are lost). include/mesos/master/allocator.hpp (lines 228 - 229) <https://reviews.apache.org/r/37993/#comment157881> Not necessary. include/mesos/master/allocator.hpp (line 237) <https://reviews.apache.org/r/37993/#comment157889> s/It/This/ include/mesos/master/allocator.hpp (lines 238 - 242) <https://reviews.apache.org/r/37993/#comment157891> These two comments pretty much say the same thing. I suggest moving merging them. i.e. @param whitelist A set of agents that are allowed to contribute their resources to the resource pool. include/mesos/master/allocator.hpp (line 250) <https://reviews.apache.org/r/37993/#comment157892> s/an/the/ include/mesos/master/allocator.hpp (line 252) <https://reviews.apache.org/r/37993/#comment157893> s/an/the/ include/mesos/master/allocator.hpp (lines 253 - 255) <https://reviews.apache.org/r/37993/#comment157894> Not necessary. include/mesos/master/allocator.hpp (line 265) <https://reviews.apache.org/r/37993/#comment157896> s/An/The/ include/mesos/master/allocator.hpp (line 266) <https://reviews.apache.org/r/37993/#comment157897> s/operations, it may/operations. The allocator should/ include/mesos/master/allocator.hpp (lines 269 - 272) <https://reviews.apache.org/r/37993/#comment157899> Not necessary. include/mesos/master/allocator.hpp (line 290) <https://reviews.apache.org/r/37993/#comment157901> Suggestion: Updates unavailability for an agent. include/mesos/master/allocator.hpp (lines 296 - 299) <https://reviews.apache.org/r/37993/#comment157900> Not necessary. include/mesos/master/allocator.hpp (lines 311 - 312) <https://reviews.apache.org/r/37993/#comment157903> Not necessary. include/mesos/master/allocator.hpp (line 332) <https://reviews.apache.org/r/37993/#comment157904> Be careful with your rebase :) include/mesos/master/allocator.hpp (line 338) <https://reviews.apache.org/r/37993/#comment157906> s/Recover/Recovers/ include/mesos/master/allocator.hpp (lines 340 - 344) <https://reviews.apache.org/r/37993/#comment157910> Suggestion: Used to update the set of available resources for a specific agent. This method is invoked to inform the allocator about allocated resources that have been refused or are no longer in use. include/mesos/master/allocator.hpp (lines 345 - 351) <https://reviews.apache.org/r/37993/#comment157907> Not necessary. include/mesos/master/allocator.hpp (lines 362 - 363) <https://reviews.apache.org/r/37993/#comment157913> Suggestion: Revives offers for a framework. This is invoked when by a framework when it wishes to receive filtered resources or offers immediately. include/mesos/master/allocator.hpp (lines 364 - 365) <https://reviews.apache.org/r/37993/#comment157914> Not necessary. include/mesos/master/allocator.hpp (line 373) <https://reviews.apache.org/r/37993/#comment157915> s/resources/offers/ s/for/to/ - Joseph Wu On Sept. 25, 2015, 10:22 a.m., Guangya Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37993/ > ----------------------------------------------------------- > > (Updated Sept. 25, 2015, 10:22 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 8100f14220599247a340c919a03f29755b5349d8 > > Diff: https://reviews.apache.org/r/37993/diff/ > > > Testing > ------- > > > Thanks, > > Guangya Liu > >
