You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jay Guo <gu...@gmail.com> on 2017/01/19 08:01:38 UTC
Review Request 55710: Added Agent capabilities in the response of
/state endpoint.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
Bugs: MESOS-6902
https://issues.apache.org/jira/browse/MESOS-6902
Repository: mesos
Description
-------
We introduced Capabilities to SlaveInfo protobuf message in ec1a326.
It is automatically wrapped into response of GetState v1 API. This
patch added this field to /state v0 API for consistency.
Diffs
-----
src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230
Diff: https://reviews.apache.org/r/55710/diff/
Testing
-------
WIP. We need to figure out how to inject `SlaveInfo` during Agent registration for testing purpose, before we actually implement Agent Capabilities.
Thanks,
Jay Guo
Re: Review Request 55710: Added Agent capabilities in the response of
/state endpoint.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/#review162396
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55710]
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 Jan. 20, 2017, 5:29 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55710/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2017, 5:29 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We introduced Capabilities to SlaveInfo protobuf message in ec1a326.
> It is automatically wrapped into response of GetState v1 API. This
> patch added this field to /state v0 API for consistency.
>
>
> Diffs
> -----
>
> src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230
> src/tests/master_tests.cpp da7094dbbafbb0ab1153a0a4a6fcabd63888d67a
>
> Diff: https://reviews.apache.org/r/55710/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
> [ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
> [----------] 1 test from MasterTest (94 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (122 ms total)
>
> make check on Ubuntu 14.04
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55710: Added Agent capabilities in the response of
/state endpoint.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 23, 2017, 10:33 p.m., Michael Park wrote:
> > Ship It!
>
> Michael Park wrote:
> Do we have a patch to add `capabilities` to Agent `/state` endpoint as well?
FYI
I was looking into setting the MULTI_ROLE capability in the agent code, and I'd like to update what we committed to follow the approach we took for sending the agent's version. The version is communicated through the registration and re-registration messages as a separate field rather than being set inside `SlaveInfo`. This was because we don't handle an updated `SlaveInfo` currently (the agent considers `SlaveInfo` changes during restart to be incompatible, and the master does not update the `SlaveInfo` in the registry if it were to change). So, to avoid tackling these problems now, we can start with just having capabilities follow the same approach as what we did for the agent version. Make sense? Can you send patches for this? Note that it doesn't need to block any of your outstanding patches from going in AFAICT.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/#review162719
-----------------------------------------------------------
On Jan. 20, 2017, 5:29 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55710/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2017, 5:29 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We introduced Capabilities to SlaveInfo protobuf message in ec1a326.
> It is automatically wrapped into response of GetState v1 API. This
> patch added this field to /state v0 API for consistency.
>
>
> Diffs
> -----
>
> src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230
> src/tests/master_tests.cpp da7094dbbafbb0ab1153a0a4a6fcabd63888d67a
>
> Diff: https://reviews.apache.org/r/55710/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
> [ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
> [----------] 1 test from MasterTest (94 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (122 ms total)
>
> make check on Ubuntu 14.04
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55710: Added Agent capabilities in the response of
/state endpoint.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 23, 2017, 2:33 p.m., Michael Park wrote:
> > Ship It!
Do we have a patch to add `capabilities` to Agent `/state` endpoint as well?
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/#review162719
-----------------------------------------------------------
On Jan. 19, 2017, 9:29 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55710/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2017, 9:29 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We introduced Capabilities to SlaveInfo protobuf message in ec1a326.
> It is automatically wrapped into response of GetState v1 API. This
> patch added this field to /state v0 API for consistency.
>
>
> Diffs
> -----
>
> src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230
> src/tests/master_tests.cpp da7094dbbafbb0ab1153a0a4a6fcabd63888d67a
>
> Diff: https://reviews.apache.org/r/55710/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
> [ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
> [----------] 1 test from MasterTest (94 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (122 ms total)
>
> make check on Ubuntu 14.04
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55710: Added Agent capabilities in the response of
/state endpoint.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/#review162719
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On Jan. 19, 2017, 9:29 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55710/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2017, 9:29 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We introduced Capabilities to SlaveInfo protobuf message in ec1a326.
> It is automatically wrapped into response of GetState v1 API. This
> patch added this field to /state v0 API for consistency.
>
>
> Diffs
> -----
>
> src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230
> src/tests/master_tests.cpp da7094dbbafbb0ab1153a0a4a6fcabd63888d67a
>
> Diff: https://reviews.apache.org/r/55710/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
> [ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
> [----------] 1 test from MasterTest (94 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (122 ms total)
>
> make check on Ubuntu 14.04
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55710: Added agent capabilities to /state endpoint
of master.
Posted by Michael Park <mp...@apache.org>.
> On Feb. 8, 2017, 3:46 p.m., Michael Park wrote:
> > src/master/http.cpp, lines 380-384
> > <https://reviews.apache.org/r/55710/diff/3/?file=1626394#file1626394line380>
> >
> > I think it makes sense to add:
> > ```cpp
> > static void json(
> > JSON::StringWriter* writer, const SlaveInfo::Capability& capability)
> > {
> > writer->append(SlaveInfo::Capability::Type_Name(capability.type()));
> > }
> > ```
> >
> > then let `protobuf::slave::Capabilities` keep track of
> > `std::set<SlaveInfo::Capability> capabilities`.
> >
> > and just do:
> > ```cpp
> > writer->field("capabilities", slave.capabilities.capabilities);
> > ```
>
> Jay Guo wrote:
> Instead of `std::set`, I use `RepeatedPtrField` to store agent capabilities to avoid adding more methods to type_utils.hpp
I don't really understand. What do you mean by this?
> avoid adding more methods to type_utils.hpp
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/#review164807
-----------------------------------------------------------
On Feb. 8, 2017, 11:22 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55710/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 11:22 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Master should be able to reflect agent capabilities via `/state`(v0)
> and `getState`(v1) endpoints.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017
> include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd
> src/common/protobuf_utils.hpp 3ba689f30ae080ea9b2a0af4d819dd3e377c7bf5
> src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a
> src/master/http.cpp a598488296d4616c0126aa3bd4d1d7e7a6e439fe
> src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18
>
> Diff: https://reviews.apache.org/r/55710/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
> [ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
> [----------] 1 test from MasterTest (94 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (122 ms total)
>
> make check on Ubuntu 14.04
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55710: Added agent capabilities to /state endpoint
of master.
Posted by Jay Guo <gu...@gmail.com>.
> On Feb. 9, 2017, 7:46 a.m., Michael Park wrote:
> > src/master/http.cpp, lines 380-384
> > <https://reviews.apache.org/r/55710/diff/3/?file=1626394#file1626394line380>
> >
> > I think it makes sense to add:
> > ```cpp
> > static void json(
> > JSON::StringWriter* writer, const SlaveInfo::Capability& capability)
> > {
> > writer->append(SlaveInfo::Capability::Type_Name(capability.type()));
> > }
> > ```
> >
> > then let `protobuf::slave::Capabilities` keep track of
> > `std::set<SlaveInfo::Capability> capabilities`.
> >
> > and just do:
> > ```cpp
> > writer->field("capabilities", slave.capabilities.capabilities);
> > ```
>
> Jay Guo wrote:
> Instead of `std::set`, I use `RepeatedPtrField` to store agent capabilities to avoid adding more methods to type_utils.hpp
>
> Michael Park wrote:
> I don't really understand. What do you mean by this?
> > avoid adding more methods to type_utils.hpp
For `set<SlaveInfo::Capability>`, we need to implement `operator==` and `hash()`, like others in https://github.com/apache/mesos/blob/master/include/mesos/type_utils.hpp
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/#review164807
-----------------------------------------------------------
On Feb. 9, 2017, 3:22 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55710/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2017, 3:22 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Master should be able to reflect agent capabilities via `/state`(v0)
> and `getState`(v1) endpoints.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017
> include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd
> src/common/protobuf_utils.hpp 3ba689f30ae080ea9b2a0af4d819dd3e377c7bf5
> src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a
> src/master/http.cpp a598488296d4616c0126aa3bd4d1d7e7a6e439fe
> src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18
>
> Diff: https://reviews.apache.org/r/55710/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
> [ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
> [----------] 1 test from MasterTest (94 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (122 ms total)
>
> make check on Ubuntu 14.04
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55710: Added agent capabilities to /state endpoint
of master.
Posted by Jay Guo <gu...@gmail.com>.
> On Feb. 9, 2017, 7:46 a.m., Michael Park wrote:
> > include/mesos/master/master.proto, line 310
> > <https://reviews.apache.org/r/55710/diff/3/?file=1626391#file1626391line310>
> >
> > `s/agent_capabilities/capabilities/`?
This name has been used in some of BenM's patches, do you still want to make the terminology change?
> On Feb. 9, 2017, 7:46 a.m., Michael Park wrote:
> > include/mesos/v1/master/master.proto, line 310
> > <https://reviews.apache.org/r/55710/diff/3/?file=1626392#file1626392line310>
> >
> > `s/agent_capabilities/capabilities/`
same as above.
> On Feb. 9, 2017, 7:46 a.m., Michael Park wrote:
> > src/master/http.cpp, lines 380-384
> > <https://reviews.apache.org/r/55710/diff/3/?file=1626394#file1626394line380>
> >
> > I think it makes sense to add:
> > ```cpp
> > static void json(
> > JSON::StringWriter* writer, const SlaveInfo::Capability& capability)
> > {
> > writer->append(SlaveInfo::Capability::Type_Name(capability.type()));
> > }
> > ```
> >
> > then let `protobuf::slave::Capabilities` keep track of
> > `std::set<SlaveInfo::Capability> capabilities`.
> >
> > and just do:
> > ```cpp
> > writer->field("capabilities", slave.capabilities.capabilities);
> > ```
Instead of `std::set`, I use `RepeatedPtrField` to store agent capabilities to avoid adding more methods to type_utils.hpp
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/#review164807
-----------------------------------------------------------
On Feb. 9, 2017, 3:22 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55710/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2017, 3:22 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Master should be able to reflect agent capabilities via `/state`(v0)
> and `getState`(v1) endpoints.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017
> include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd
> src/common/protobuf_utils.hpp 3ba689f30ae080ea9b2a0af4d819dd3e377c7bf5
> src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a
> src/master/http.cpp a598488296d4616c0126aa3bd4d1d7e7a6e439fe
> src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18
>
> Diff: https://reviews.apache.org/r/55710/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
> [ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
> [----------] 1 test from MasterTest (94 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (122 ms total)
>
> make check on Ubuntu 14.04
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55710: Added agent capabilities to /state endpoint
of master.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/#review164807
-----------------------------------------------------------
include/mesos/master/master.proto (line 310)
<https://reviews.apache.org/r/55710/#comment236599>
`s/agent_capabilities/capabilities/`?
include/mesos/v1/master/master.proto (line 310)
<https://reviews.apache.org/r/55710/#comment236600>
`s/agent_capabilities/capabilities/`
src/master/http.cpp (lines 380 - 384)
<https://reviews.apache.org/r/55710/#comment236604>
I think it makes sense to add:
```cpp
static void json(
JSON::StringWriter* writer, const SlaveInfo::Capability& capability)
{
writer->append(SlaveInfo::Capability::Type_Name(capability.type()));
}
```
then let `protobuf::slave::Capabilities` keep track of
`std::set<SlaveInfo::Capability> capabilities`.
and just do:
```cpp
writer->field("capabilities", slave.capabilities.capabilities);
```
- Michael Park
On Feb. 7, 2017, 1:59 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55710/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2017, 1:59 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Master should be able to reflect agent capabilities via `/state`(v0)
> and `getState`(v1) endpoints.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017
> include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd
> src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a
> src/master/http.cpp d881ad6dba9ba96057988db265faf0b3013c9b05
> src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18
>
> Diff: https://reviews.apache.org/r/55710/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
> [ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
> [----------] 1 test from MasterTest (94 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (122 ms total)
>
> make check on Ubuntu 14.04
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55710: Added agent capabilities to /state endpoint
of master.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/#review164549
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55710]
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 Feb. 7, 2017, 9:59 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55710/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2017, 9:59 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Master should be able to reflect agent capabilities via `/state`(v0)
> and `getState`(v1) endpoints.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017
> include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd
> src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a
> src/master/http.cpp d881ad6dba9ba96057988db265faf0b3013c9b05
> src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18
>
> Diff: https://reviews.apache.org/r/55710/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
> [ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
> [----------] 1 test from MasterTest (94 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (122 ms total)
>
> make check on Ubuntu 14.04
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55710: Added agent capabilities to /state endpoint
of master.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/#review165409
-----------------------------------------------------------
Fix it, then Ship it!
src/common/protobuf_utils.hpp (line 163)
<https://reviews.apache.org/r/55710/#comment237252>
This comment is a bit misleading, since this is more general, it's for going back to the protobuf from the struct.
src/master/http.cpp (lines 129 - 142)
<https://reviews.apache.org/r/55710/#comment237258>
Just a note, the existing approach here that we're following in this patch is unfortunate in that it assumes there is only a 'type' within capability. If more fields are added it's difficult to expose them here as we have to introduce a second 'capabilities' key to keep it backwards compatible.
E.g.
```
// current technique:
{
"capabilities": ["MULTI_ROLE"],
}
// vs pure transformation into json:
{
"capabilities": [
{
"type": "MULTI_ROLE"
}
],
}
```
Since this is already how we expose framework capabilities I think we'll just stick with this approach.
src/tests/master_tests.cpp (line 4090)
<https://reviews.apache.org/r/55710/#comment237254>
We should check for a capabilities entry as this will insert one if absent.
- Benjamin Mahler
On Feb. 13, 2017, 9:35 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55710/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2017, 9:35 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Master should be able to reflect agent capabilities via `/state`(v0)
> and `getState`(v1) endpoints.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017
> include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd
> src/common/protobuf_utils.hpp 3ba689f30ae080ea9b2a0af4d819dd3e377c7bf5
> src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a
> src/master/http.cpp c5324abc0db82275fd65d3f7d361ad8ee9e017d1
> src/tests/master_tests.cpp 9a7a953adbbbed0b37082bb0166d03b1388c80ae
>
> Diff: https://reviews.apache.org/r/55710/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
> [ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
> [----------] 1 test from MasterTest (94 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (122 ms total)
>
> make check on Ubuntu 14.04
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55710: Added agent capabilities to /state endpoint
of master.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/#review165305
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55710]
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 Feb. 13, 2017, 9:35 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55710/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2017, 9:35 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Master should be able to reflect agent capabilities via `/state`(v0)
> and `getState`(v1) endpoints.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017
> include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd
> src/common/protobuf_utils.hpp 3ba689f30ae080ea9b2a0af4d819dd3e377c7bf5
> src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a
> src/master/http.cpp c5324abc0db82275fd65d3f7d361ad8ee9e017d1
> src/tests/master_tests.cpp 9a7a953adbbbed0b37082bb0166d03b1388c80ae
>
> Diff: https://reviews.apache.org/r/55710/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
> [ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
> [----------] 1 test from MasterTest (94 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (122 ms total)
>
> make check on Ubuntu 14.04
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55710: Added agent capabilities to /state endpoint
of master.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/
-----------------------------------------------------------
(Updated Feb. 13, 2017, 5:35 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
Changes
-------
address BenM's comments.
Bugs: MESOS-6902
https://issues.apache.org/jira/browse/MESOS-6902
Repository: mesos
Description
-------
Master should be able to reflect agent capabilities via `/state`(v0)
and `getState`(v1) endpoints.
Diffs (updated)
-----
include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017
include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd
src/common/protobuf_utils.hpp 3ba689f30ae080ea9b2a0af4d819dd3e377c7bf5
src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a
src/master/http.cpp c5324abc0db82275fd65d3f7d361ad8ee9e017d1
src/tests/master_tests.cpp 9a7a953adbbbed0b37082bb0166d03b1388c80ae
Diff: https://reviews.apache.org/r/55710/diff/
Testing
-------
make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
[ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
[----------] 1 test from MasterTest (94 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (122 ms total)
make check on Ubuntu 14.04
Thanks,
Jay Guo
Re: Review Request 55710: Added agent capabilities to /state endpoint
of master.
Posted by Jay Guo <gu...@gmail.com>.
> On Feb. 11, 2017, 8:57 a.m., Benjamin Mahler wrote:
> > src/common/protobuf_utils.hpp, lines 164-165
> > <https://reviews.apache.org/r/55710/diff/4/?file=1628277#file1628277line164>
> >
> > This is unfortunate since the bool and the repeated field need to be kept consistent but there's nothing enforcing this.
> >
> > How about a function that generates the repeated field?
> >
> > ```
> > google::protobuf::RepeatedPtrField<SlaveInfo::Capability> toRepeatedField() const
> > {
> > ...;
> > }
> > ```
> >
> > Alternatively, store the capabilities as you did here, but make the booleans functions that loop over the capabilities:
> >
> > ```
> > bool multiRole() const
> > {
> > ...;
> > }
> > ```
I went for the first solution so that we don't need to affect some existing code. However, would this somehow affect performance?
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/#review165217
-----------------------------------------------------------
On Feb. 13, 2017, 5:35 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55710/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2017, 5:35 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Master should be able to reflect agent capabilities via `/state`(v0)
> and `getState`(v1) endpoints.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017
> include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd
> src/common/protobuf_utils.hpp 3ba689f30ae080ea9b2a0af4d819dd3e377c7bf5
> src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a
> src/master/http.cpp c5324abc0db82275fd65d3f7d361ad8ee9e017d1
> src/tests/master_tests.cpp 9a7a953adbbbed0b37082bb0166d03b1388c80ae
>
> Diff: https://reviews.apache.org/r/55710/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
> [ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
> [----------] 1 test from MasterTest (94 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (122 ms total)
>
> make check on Ubuntu 14.04
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55710: Added agent capabilities to /state endpoint
of master.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/#review165217
-----------------------------------------------------------
Looks good, the main issue is that it seems error-prone to store both the booleans and the raw capabilities within the struct in a non-const manner, since they can go out of sync. Left some suggestions below.
include/mesos/master/master.proto (line 310)
<https://reviews.apache.org/r/55710/#comment237042>
I don't think you need the `agent_` prefix, given we're within `Agent` here, any reason you added it?
src/common/protobuf_utils.hpp (lines 164 - 165)
<https://reviews.apache.org/r/55710/#comment237047>
This is unfortunate since the bool and the repeated field need to be kept consistent but there's nothing enforcing this.
How about a function that generates the repeated field?
```
google::protobuf::RepeatedPtrField<SlaveInfo::Capability> toRepeatedField() const
{
...;
}
```
Alternatively, store the capabilities as you did here, but make the booleans functions that loop over the capabilities:
```
bool multiRole() const
{
...;
}
```
src/common/protobuf_utils.cpp (lines 654 - 658)
<https://reviews.apache.org/r/55710/#comment237048>
Can't you just do a single CopyFrom here?
```
agent.mutable_agent_capabilities()->CopyFrom(
slave.capabilities.capabilities);
```
src/master/http.cpp (lines 136 - 137)
<https://reviews.apache.org/r/55710/#comment237045>
Can you put each argument on its own line?
```
static void json(
JSON::StringWriter* writer,
const SlaveInfo::Capability& capability)
```
src/tests/master_tests.cpp (line 4087)
<https://reviews.apache.org/r/55710/#comment237050>
We can just use the `->` operator instead of `.get().`, note that a lot of the existing tests still need to be swept to clean them up.
- Benjamin Mahler
On Feb. 9, 2017, 7:22 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55710/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2017, 7:22 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Master should be able to reflect agent capabilities via `/state`(v0)
> and `getState`(v1) endpoints.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017
> include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd
> src/common/protobuf_utils.hpp 3ba689f30ae080ea9b2a0af4d819dd3e377c7bf5
> src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a
> src/master/http.cpp a598488296d4616c0126aa3bd4d1d7e7a6e439fe
> src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18
>
> Diff: https://reviews.apache.org/r/55710/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
> [ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
> [----------] 1 test from MasterTest (94 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (122 ms total)
>
> make check on Ubuntu 14.04
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55710: Added agent capabilities to /state endpoint
of master.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/#review164931
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55710]
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 Feb. 9, 2017, 7:22 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55710/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2017, 7:22 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Master should be able to reflect agent capabilities via `/state`(v0)
> and `getState`(v1) endpoints.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017
> include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd
> src/common/protobuf_utils.hpp 3ba689f30ae080ea9b2a0af4d819dd3e377c7bf5
> src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a
> src/master/http.cpp a598488296d4616c0126aa3bd4d1d7e7a6e439fe
> src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18
>
> Diff: https://reviews.apache.org/r/55710/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
> [ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
> [----------] 1 test from MasterTest (94 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (122 ms total)
>
> make check on Ubuntu 14.04
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55710: Added agent capabilities to /state endpoint
of master.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/
-----------------------------------------------------------
(Updated Feb. 9, 2017, 3:22 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
Changes
-------
address mcypark's comments.
Bugs: MESOS-6902
https://issues.apache.org/jira/browse/MESOS-6902
Repository: mesos
Description
-------
Master should be able to reflect agent capabilities via `/state`(v0)
and `getState`(v1) endpoints.
Diffs (updated)
-----
include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017
include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd
src/common/protobuf_utils.hpp 3ba689f30ae080ea9b2a0af4d819dd3e377c7bf5
src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a
src/master/http.cpp a598488296d4616c0126aa3bd4d1d7e7a6e439fe
src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18
Diff: https://reviews.apache.org/r/55710/diff/
Testing
-------
make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
[ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
[----------] 1 test from MasterTest (94 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (122 ms total)
make check on Ubuntu 14.04
Thanks,
Jay Guo
Re: Review Request 55710: Added agent capabilities to /state endpoint
of master.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/
-----------------------------------------------------------
(Updated Feb. 7, 2017, 5:59 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
Summary (updated)
-----------------
Added agent capabilities to /state endpoint of master.
Bugs: MESOS-6902
https://issues.apache.org/jira/browse/MESOS-6902
Repository: mesos
Description (updated)
-------
Master should be able to reflect agent capabilities via `/state`(v0)
and `getState`(v1) endpoints.
Diffs (updated)
-----
include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017
include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd
src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a
src/master/http.cpp d881ad6dba9ba96057988db265faf0b3013c9b05
src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18
Diff: https://reviews.apache.org/r/55710/diff/
Testing
-------
make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
[ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
[----------] 1 test from MasterTest (94 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (122 ms total)
make check on Ubuntu 14.04
Thanks,
Jay Guo
Re: Review Request 55710: Added Agent capabilities in the response of
/state endpoint.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55710/
-----------------------------------------------------------
(Updated Jan. 20, 2017, 1:29 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
Changes
-------
Added a test to check `capablities` is included in `/state` response.
Bugs: MESOS-6902
https://issues.apache.org/jira/browse/MESOS-6902
Repository: mesos
Description
-------
We introduced Capabilities to SlaveInfo protobuf message in ec1a326.
It is automatically wrapped into response of GetState v1 API. This
patch added this field to /state v0 API for consistency.
Diffs (updated)
-----
src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230
src/tests/master_tests.cpp da7094dbbafbb0ab1153a0a4a6fcabd63888d67a
Diff: https://reviews.apache.org/r/55710/diff/
Testing (updated)
-------
make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
[ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
[----------] 1 test from MasterTest (94 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (122 ms total)
make check on Ubuntu 14.04
Thanks,
Jay Guo