This is an automatically generated e-mail. To reply, visit:

Ship it!

LGTM overall. Please address the remaining issues and commit yourself!

src/master/http.cpp (line 475)

    We typically use leading undescore for temp variables. The tailing 
underscore is for class members (following google style).
    In fact, I think the temp variable here is not necessary. There are only 
two places where this temp variable is used. I would rather use 
'values.get().at("...")', but this is up to you.

src/master/http.cpp (line 498)

    Do you need this temp variable. Looks like you can just do
    foreach (.. value, parse.get().values) {...

src/master/http.cpp (line 511)

    Kill this line.

src/master/http.cpp (line 533)

    What does 'recovered' mean and what does remaining mean? It'll be great if 
you comment on that to improve readability.
    For example,
    // The resources recovered by recinding outstanding offers.
    // The unreserved resources needed to satify the RESERVE operation.
    IIUC, the variable 'remaining' is only used for optimization, right? Could 
you please mention that in the comments that keep this variable is for 
optimization (i.e., avoid rescinding unnecessary offers).

src/master/http.cpp (line 534)

    I don't like the name 'flatten' :(
    Could you at least be more explicit about it (i.e., emphasize that 
'remaining' only has unreserved resources). 
    Resources remaining = resources.flatten('*');

src/master/http.cpp (line 553)

    you win the race because the default filter refuse_sec is 5 seconds? Worth 
mentioning that in the comment.

src/master/http.cpp (line 573)

    What is 'Nothing' here?

src/master/master.cpp (line 5472)

    The name sounds weired. You are passing in an offer operation but the 
function name is called 'applyResourceOperation'.
    I would suggest we create two 'Master::apply' overloads and don't worry 
about the code duplication.
    void apply(framework, slave, opeartion);
    Future<Nothing> apply(slave, operation);

- Jie Yu

On July 31, 2015, 9:56 p.m., Michael Park wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35702/
> -----------------------------------------------------------
> (Updated July 31, 2015, 9:56 p.m.)
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, Joris 
> Van Remoortere, and Vinod Kone.
> Bugs: MESOS-2600
>     https://issues.apache.org/jira/browse/MESOS-2600
> Repository: mesos
> Description
> -------
> This involved a lot more challenges than I anticipated, I've captured the 
> various approaches and limitations and deal-breakers of those approaches 
> here: [Master Endpoint Implementation 
> Challenges](https://docs.google.com/document/d/1cwVz4aKiCYP9Y4MOwHYZkyaiuEv7fArCye-vPvB2lAI/edit#)
> Key points:
> * This is a stop-gap solution until we shift the offer creation/management 
> logic from the master to the allocator.
> * `updateAvailable` and `updateSlave` are kept separate because
>   (1) `updateAvailable` is allowed to fail whereas `updateSlave` must not.
>   (2) `updateAvailable` returns a `Future` whereas `updateSlave` does not.
>   (3) `updateAvailable` never leaves the allocator in an over-allocated state 
> and must not, whereas `updateSlave` does, and can.
> * The algorithm:
>     * Initially, the master pessimistically assume that what seems like 
> "available" resources will be gone.
>       This is due to the race between the allocator scheduling an `allocate` 
> call to itself vs master's `allocator->updateAvailable` invocation.
>       As such, we first try to satisfy the request only with the offered 
> resources.
>     * We greedily rescind one offer at a time until we've rescinded 
> sufficiently many offers.
>       IMPORTANT: We perform `recoverResources(..., Filters())` rather than 
> `recoverResources(..., None())` so that we can pretty much always win the 
> race against `allocate`.
>                  In the case that we lose, no disaster occurs. We simply fail 
> to satisfy the request.
>     * If we still don't have enough resources after resciding all offers, be 
> optimistic and forward the request to the allocator since there may be 
> available resources to satisfy the request.
>     * If the allocator returns a failure, report the error to the user with 
> `PreconditionFailed`. This could be updated to be `Forbidden`, or `Conflict` 
> maybe as well. We'll pick one eventually.
> This approach is clearly not ideal, since we would prefer to rescind as 
> little offers as possible.
> The challenges of implementing the ideal solution in the current state is 
> described in the document above.
> TODO(mpark): Add more comments and test cases.
> Diffs
> -----
>   src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f 
>   src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc 
>   src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544 
>   src/master/validation.hpp 43b8d84556e7f0a891dddf6185bbce7ca50b360a 
>   src/master/validation.cpp ffb7bf07b8a40d6e14f922eabcf46045462498b5 
> Diff: https://reviews.apache.org/r/35702/diff/
> Testing
> -------
> `make check`
> Thanks,
> Michael Park

Reply via email to