You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2016/12/08 14:21:48 UTC
Review Request 54529: Exposed framework roles in master and agent
endpoints.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54529/
-----------------------------------------------------------
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 d410c2f725206a9879a3026adff305c7aed2e98f
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
Re: Review Request 54529: Exposed framework roles in master and agent
endpoints.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54529/#review158544
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54529]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 8, 2016, 2:21 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54529/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 2:21 p.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 d410c2f725206a9879a3026adff305c7aed2e98f
> 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
>
>
Re: Review Request 54529: Exposed framework roles in master and agent
endpoints.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54529/#review158744
-----------------------------------------------------------
Ship it!
Modulo discussion in the comments.
- Benjamin Mahler
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
>
>
Re: Review Request 54529: Exposed framework roles in master and agent
endpoints.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54529/#review158674
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54529]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
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
>
>
Re: Review Request 54529: Exposed framework roles in master and agent
endpoints.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54529/
-----------------------------------------------------------
(Updated Dec. 9, 2016, 11: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 (updated)
-----
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
Re: Review Request 54529: Exposed framework roles in master and agent
endpoints.
Posted by Guangya Liu <gy...@gmail.com>.
> On \u5341\u4e8c\u6708 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.
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"
}
```
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54529/#review158557
-----------------------------------------------------------
On \u5341\u4e8c\u6708 8, 2016, 2:21 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54529/
> -----------------------------------------------------------
>
> (Updated \u5341\u4e8c\u6708 8, 2016, 2:21 p.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 d410c2f725206a9879a3026adff305c7aed2e98f
> 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
>
>
Re: Review Request 54529: Exposed framework roles in master and agent
endpoints.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Dec. 8, 2016, 8: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"
> }
> ```
@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.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54529/#review158557
-----------------------------------------------------------
On Dec. 9, 2016, 11: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, 11: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
>
>
Re: Review Request 54529: Exposed framework roles in master and agent
endpoints.
Posted by Benjamin Mahler <bm...@apache.org>.
> 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
>
>
Re: Review Request 54529: Exposed framework roles in master and agent
endpoints.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54529/#review158557
-----------------------------------------------------------
src/master/http.cpp (lines 210 - 211)
<https://reviews.apache.org/r/54529/#comment229339>
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.
- Benjamin Mahler
On Dec. 8, 2016, 2:21 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54529/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 2:21 p.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 d410c2f725206a9879a3026adff305c7aed2e98f
> 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
>
>