Mailing-List: contact dev-help@mesos.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@mesos.apache.org
Content-Type: multipart/alternative;
 boundary="===============8299163176408446140=="
MIME-Version: 1.0
Subject: Re: Review Request 30911: Updated the filter abstraction for
 Resources.
From: "Michael Park" <mcypark@gmail.com>
To: "Benjamin Hindman" <benh@berkeley.edu>, "Adam B" <adam@mesosphere.io>,
 "Jie Yu" <yujie.jay@gmail.com>, "Ben Mahler" <benjamin.mahler@gmail.com>,
 "Vinod Kone" <vinodkone@gmail.com>
Cc: "mesos" <dev@mesos.apache.org>, "Michael Park" <mcypark@gmail.com>
Date: Tue, 24 Feb 2015 08:31:58 -0000
Message-ID: <20150224083158.4174.48110@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Michael Park" <noreply@reviews.apache.org>
References: <20150224030810.4175.70130@reviews.apache.org>
In-Reply-To: <20150224030810.4175.70130@reviews.apache.org>
Reply-To: "Michael Park" <mcypark@gmail.com>

--===============8299163176408446140==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> 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() != "*";
> >     ```

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?


- Michael


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30911/#review73743
-----------------------------------------------------------


On Feb. 24, 2015, 7:48 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, 7:48 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
> 
>


--===============8299163176408446140==--