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/02/07 09:59:27 UTC
Re: Review Request 55710: Added agent capabilities to /state endpoint
of master.
-----------------------------------------------------------
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 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