> On Feb. 24, 2015, 3:08 a.m., Ben Mahler wrote:
> > src/common/resources.cpp, lines 416-417
> > <https://reviews.apache.org/r/30911/diff/5/?file=873147#file873147line416>
> >
> > This is a little tricky to think through, how about:
> >
> > ```
> > if (role.isSome()) {
> > return resource.role() == role.get();
> > }
> >
> > return resource.role() != "*";
> > ```
>
> Michael Park wrote:
> That snippet has different semantics. `isReserved` needs to return
> `false` given `"*"`.
> I went through roughly the following steps starting from what you
> suggested:
>
> ```cpp
> if (role.isSome()) {
> return !isUnreserved(resource) && resource.role() == role.get();
> }
>
> return !isUnreserved(resource);
> ```
>
> mm... pull out the `isUnreserved`,
>
> ```cpp
> if (isUnreserved(resource)) {
> return false;
> }
>
> return role.isSome() ? resource.role() == role.get() : true;
> ```
>
> mm... don't like the `true`,
>
> ```cpp
> if (isUnreserved(resource)) {
> return false;
> }
>
> return role.isNone() || resource.role() == role.get();
> ```
>
> mm... don't like the `false`,
>
> ```cpp
> return !isUnreserved(resource) &&
> (role.isNone() || resource.role() == role.get());
> ```
>
> Later on when I introduced `ReserverType`:
>
> ```cpp
> return !isUnreserved(resource) &&
> (role.isNone() || resource.role() == role.get()) &&
> (reserverType.isNone() || resource.reserver_type() ==
> reserverType.get());
> ```
>
> We're no longer introducing `ReserverType` so that part is irrelevant, I
> was just pointing out that the pattern seems to extend nicely.
>
> Which alternative (if any) do you like?
Chatted with BenM, we think the following is more easy to read:
```
if (role.isSome()) {
return resource.role() != "*" && role.get() == resource.role();
} else {
return resource.role() != "*";
}
```
> On Feb. 24, 2015, 3:08 a.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp, lines 795-799
> > <https://reviews.apache.org/r/30911/diff/5/?file=873148#file873148line795>
> >
> > Whoops!
> >
> > What happens when role is `"*"` now? The role sorter can wind up with
> > `"*"` as a role, since FrameworkInfo uses that as the default and we don't
> > reject frameworks with role `"*"`. It looks like with your change we're
> > over-counting in this case.
> >
> > We should really add a NOTE about this gotcha.
>
> Michael Park wrote:
> This does work correctly, because of the way you introduced
> `reserved(role)` in https://reviews.apache.org/r/28985:
>
> ```cpp
> // Returns the reserved resources for the role. Note that the "*"
> // role represents unreserved resources, and will be ignored.
> Resources reserved(const std::string& role) const;
> ```
>
> I could comment here explaining this, but if that's the case I wonder if
> the API isn't intuitive enough? I think part of the problem though is that we
> know that the concept of reservations are implemented through the state of
> `role`. Anyway, I'm open to suggestions here.
Added a NOTE in the allocator.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30911/#review73743
-----------------------------------------------------------
On Feb. 24, 2015, 9:03 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30911/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2015, 9:03 a.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and
> Vinod Kone.
>
>
> Bugs: MESOS-2348
> https://issues.apache.org/jira/browse/MESOS-2348
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See [JIRA Ticket](https://issues.apache.org/jira/browse/MESOS-2348).
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp c242bcc29c490841354d6fdc8d0de16eeea602ed
> src/common/resources.cpp a45bbaf696a6cc61a06daaa52a84f0014e7fe8cb
> src/master/allocator/mesos/hierarchical.hpp
> 9b7ded34ad7546f36dd41f64fe2e3cf41c1cf702
> src/master/master.hpp 8c44d6ed57ad1b94a17bef8142a5e6a15889a810
> src/master/validation.cpp e0936515d763a2cd3631a78062f66a304c0c0944
> src/slave/slave.cpp d44910838fc479b62200cdfd342ad13524504838
> src/tests/resources_tests.cpp 4298d108e32758599a28097028cc03b6bd7febfc
>
> Diff: https://reviews.apache.org/r/30911/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
