> On Aug. 5, 2015, 5:46 a.m., Jie Yu wrote:
> > src/master/http.cpp, line 475
> > <https://reviews.apache.org/r/35702/diff/12/?file=1026443#file1026443line475>
> >
> > 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.
Removed temporary variable and used `values.get().at(...)` instead.
The reason why I did it this way is because I've been following the general
pattern of:
```cpp
Try<Obj> _obj = getObj(...);
if (_obj.isError()) {
return SomeError(_obj.error());
}
const Obj& obj = _obj.get();
// proceed with 'obj'.
```
This is a very common pattern for us that I would like to eventually explore
for a cleaner solution.
> On Aug. 5, 2015, 5:46 a.m., Jie Yu wrote:
> > src/master/http.cpp, line 498
> > <https://reviews.apache.org/r/35702/diff/12/?file=1026443#file1026443line498>
> >
> > Do you need this temp variable. Looks like you can just do
> > ```
> > foreach (.. value, parse.get().values) {...
> > ```
Fixed this to use your suggestion. This was another instance of the pattern I
described above.
> On Aug. 5, 2015, 5:46 a.m., Jie Yu wrote:
> > src/master/http.cpp, line 534
> > <https://reviews.apache.org/r/35702/diff/12/?file=1026443#file1026443line534>
> >
> > 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('*');
> > ```
I don't like it either, but we currently have 9 instances of `flatten()` but no
instances of `flatten("*")`. Do you think it's worth breaking consistency here?
As far as I know, we seem to favor consistency.
> On Aug. 5, 2015, 5:46 a.m., Jie Yu wrote:
> > src/master/http.cpp, line 573
> > <https://reviews.apache.org/r/35702/diff/12/?file=1026443#file1026443line573>
> >
> > What is 'Nothing' here?
The `Nothing` here comes from the result of `master->apply` which returns a
`Future<Nothing>`. But I feel like you're not actually asking for an answer
here?
What would you like to see?
What I have currently is a comment above the code which reads:
```cpp
// Propogate the 'Future<Nothing>' as 'Future<Response>' where
// 'Nothing' -> 'OK' and Failed -> 'Conflict'.
```
> On Aug. 5, 2015, 5:46 a.m., Jie Yu wrote:
> > src/master/master.cpp, line 5482
> > <https://reviews.apache.org/r/35702/diff/12/?file=1026445#file1026445line5482>
> >
> > 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);
> > ```
I've introduced the overloaded `Master::apply` as suggested. I renamed the
original `Master::apply` to `Master::_apply` since I wanted to use it as the
continuation for `Master::apply(Slave*, const Offer::Operation&)`, then
realized I could also use it at the end of `Master::apply(Framework*, Slave*,
const Offer::Operation&)` (the same way it was before). So in the end,
functions were renamed.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35702/#review94154
-----------------------------------------------------------
On Aug. 5, 2015, 10:44 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35702/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2015, 10:44 a.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.
>
>
> Diffs
> -----
>
> src/master/http.cpp 76e70801925041f08bc94f0ca18c86f1a573b2b3
> src/master/master.hpp e44174976aa64176916827bec4c911333c9a91db
> src/master/master.cpp 5aa0a5410804fe16abd50b6953f1ffe46a019ecf
> src/master/validation.hpp 43b8d84556e7f0a891dddf6185bbce7ca50b360a
> src/master/validation.cpp ffb7bf07b8a40d6e14f922eabcf46045462498b5
>
> Diff: https://reviews.apache.org/r/35702/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
