mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 54529: Exposed framework roles in master and agent endpoints.
Date Sat, 10 Dec 2016 00:42:01 GMT


> On Dec. 8, 2016, 7:59 p.m., Benjamin Mahler wrote:
> > src/master/http.cpp, lines 210-211
> > <https://reviews.apache.org/r/54529/diff/1/?file=1579754#file1579754line210>
> >
> >     In the case of a multi-role framework with two roles ("role1", "role2") the
json here would display:
> >     
> >     ```
> >     {
> >       "role": "*",
> >       "roles": ["role1", "role2"]
> >     }
> >     ```
> >     
> >     For multi-role frameworks we must show:
> >     
> >     ```
> >     {
> >       // No "role"
> >       "roles": ["role1", "role2"]
> >     }
> >     ```
> >     
> >     For single-role frameworks, we could show either of these:
> >     
> >     ```
> >     {
> >       "role": "role",
> >       "roles": ["role"]
> >     }
> >     
> >     {
> >       "role: "role"
> >     }
> >     ```
> >     
> >     The first option would allow tooling to unconditionally look for "roles", but
the second mirrors what's happening in the protobufs. I'm inclined to just mirror the protobufs.
> 
> Guangya Liu wrote:
>     For single-role frameworks, why want to show both `role` and `roles` as an option?
I think only showing `role` is good enough, as showing `roles` may make someone confused:
I did not use multi-role framework, why `roles` here?
>     
>     ```
>     {
>       "role: "role"
>     }
>     ```
> 
> Benjamin Bannier wrote:
>     @bbmahler: I removed the `role` field for multi-role capable frameworks, and went
with the first option for non-multi-role capable frameworks.
>     
>     For clusters non running any multi-role capable frameworks this means that existing
tooling relying on `role` keeps working.
>     
>     For clusters with multi-role capable frameworks consumers already need to be updated
in order to understand `roles`; by mirroring information from `role` into `roles` we allow
them to just use `roles`. This will make writing consumer code less complicated, and also
give them a clear path forward when the `role` field is finally removed.

I should have clarified a bit further, my inclination for mirroring the protobufs is because
we tend to avoid having any kind of special logic in the endpoint handlers as it leads to
a lot of maintenance overhead (note here that we have to update the endpoint because of multi-role
support, when ideally a generic protobuf -> json mapping could pick up the new field).

When we tried moving certain pieces of the protobuf->json mapping to be done using the
generic protobuf -> json conversion (via `JSON::protobuf`), we were bit by the fact that
a lot of our "schema" has slight differences with the protobuf, and so we were only able to
use this approach in a few places:

```
src/common/http.cpp:  return JSON::protobuf(labels.labels());
src/common/http.cpp:      array.values.push_back(JSON::protobuf(ipAddress));
src/common/http.cpp:    object.values["container_id"] = JSON::protobuf(status.container_id());
src/common/http.cpp:    object.values["cgroup_info"] = JSON::protobuf(status.cgroup_info());
src/common/http.cpp:    object.values["discovery"] = JSON::protobuf(task.discovery());
src/common/http.cpp:    object.values["container"] = JSON::protobuf(task.container());
```

This was the motivation for the V1 `Call::GetState` to just provide the protobuf directly,
and translate into JSON generically if the user wants JSON. Because of this, I'm thinking
we'll just mirror the JSON here and require that tools handle the presence of both. We can
improve this later if folks feel the need (but keep in mind that the goal is to move away
from these old-style endpoints towards V1 `master::Call`s).


- Benjamin


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


On Dec. 9, 2016, 10:09 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54529/
> -----------------------------------------------------------
> 
> (Updated Dec. 9, 2016, 10:09 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6749
>     https://issues.apache.org/jira/browse/MESOS-6749
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Exposed framework roles in master and agent endpoints.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp d8cff310ecadf5bb301af9f2aa47acdf2dcd42d2 
>   src/slave/http.cpp 580a90b5547dd26ac5d26e0fd2fc1471a382f62d 
> 
> Diff: https://reviews.apache.org/r/54529/diff/
> 
> 
> Testing
> -------
> 
> `make check` (OS X), checked on various Linux flavors in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message