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/05 12:19:54 UTC
Re: Review Request 55021: Added a framework capabilities struct.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55021/
-----------------------------------------------------------
(Updated Jan. 5, 2017, 8:19 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
Changes
-------
rebase
Repository: mesos
Description
-------
Master, agent and allocator structs could use this stuct to easily
deal with Framework Capabilities.
Diffs (updated)
-----
src/common/protobuf_utils.hpp f225bb0f5196f7f4b12be25828361a38e1d42e60
src/tests/protobuf_utils_tests.cpp 4ce952a7d3901a4362aa69f199b135b53d14a723
Diff: https://reviews.apache.org/r/55021/diff/
Testing
-------
make check
Thanks,
Jay Guo
Re: Review Request 55021: Added a framework capabilities struct.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Jan. 5, 2017, 4:52 p.m., Benjamin Bannier wrote:
> > While looking at https://reviews.apache.org/r/55068/, should we also update `mesos::internal::protobuf::frameworkHasCapability` here?
Please disregard this comment, I got confused about the use value types.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55021/#review160599
-----------------------------------------------------------
On Jan. 5, 2017, 1:19 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55021/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2017, 1:19 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Master, agent and allocator structs could use this stuct to easily
> deal with Framework Capabilities.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp f225bb0f5196f7f4b12be25828361a38e1d42e60
> src/tests/protobuf_utils_tests.cpp 4ce952a7d3901a4362aa69f199b135b53d14a723
>
> Diff: https://reviews.apache.org/r/55021/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55021: Added a framework capabilities struct.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55021/#review160599
-----------------------------------------------------------
While looking at https://reviews.apache.org/r/55068/, should we also update `mesos::internal::protobuf::frameworkHasCapability` here?
- Benjamin Bannier
On Jan. 5, 2017, 1:19 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55021/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2017, 1:19 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Master, agent and allocator structs could use this stuct to easily
> deal with Framework Capabilities.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp f225bb0f5196f7f4b12be25828361a38e1d42e60
> src/tests/protobuf_utils_tests.cpp 4ce952a7d3901a4362aa69f199b135b53d14a723
>
> Diff: https://reviews.apache.org/r/55021/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55021: Added a framework capabilities struct.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55021/#review160591
-----------------------------------------------------------
src/common/protobuf_utils.hpp (line 44)
<https://reviews.apache.org/r/55021/#comment231723>
Do not use `using` declarations in header files. Instead use the fully qualified name.
src/common/protobuf_utils.hpp (line 210)
<https://reviews.apache.org/r/55021/#comment231724>
`Capabilities() = default;`
src/common/protobuf_utils.hpp (line 214)
<https://reviews.apache.org/r/55021/#comment231726>
Nit: `Framework::Capability::Type` is probably smaller than the size of a ref, so you could just copy the loop variable here.
- Benjamin Bannier
On Jan. 5, 2017, 1:19 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55021/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2017, 1:19 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Master, agent and allocator structs could use this stuct to easily
> deal with Framework Capabilities.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp f225bb0f5196f7f4b12be25828361a38e1d42e60
> src/tests/protobuf_utils_tests.cpp 4ce952a7d3901a4362aa69f199b135b53d14a723
>
> Diff: https://reviews.apache.org/r/55021/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55021: Added a framework capabilities struct.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55021/#review161604
-----------------------------------------------------------
Fix it, then Ship it!
Looks good, just a few minor suggestions and we're good to go. I don't think the test here was critical given that any code relying on the capability will fail, but I left a comment about how we could test this more comprehensively without making the test much longer.
src/common/protobuf_utils.hpp (line 207)
<https://reviews.apache.org/r/55021/#comment232863>
Brace on a newline.
src/common/protobuf_utils.hpp (lines 210 - 212)
<https://reviews.apache.org/r/55021/#comment232865>
Is it possible for this to be templated to allow any iterable? As we did here:
https://github.com/apache/mesos/blob/104532d867214b6d3c6fe96d6ebb144aa8803f7c/3rdparty/stout/include/stout/strings.hpp#L332-L334
```
template <typename Capabilities>
Capabilities(const Capabilities& capabilities)
{
foreach (const FrameworkInfo::Capability& capability, capabilities) {
...
}
}
```
This lets us test this a bit more easily (see below).
src/common/protobuf_utils.hpp (line 214)
<https://reviews.apache.org/r/55021/#comment232864>
Since FramworkInfo::Capability is not a POD type, let's leave the reference here, so:
```
foreach (const FrameworkInfo::Capability& capability, capabilities) {
```
Note that while FrameworkInfo::Capability currently only has an enum currently, it can grow to be more expensive to copy in the future.
src/tests/protobuf_utils_tests.cpp (lines 63 - 85)
<https://reviews.apache.org/r/55021/#comment232866>
The current test seems a bit arbitrary, perhaps we could test this a bit more comprehensively using a reverse operation and equality? Something like:
```
TEST(ProtobufUtilTest, Capabilities)
{
auto toSet = [](const protobuf::framework::Capabilities& capabilities) {
set<framework::Capability> result;
if (capabilities.revocableResources) {
result.insert(FrameworkInfo::Capability::REVOCABLE_RESOURCES);
}
if (capabilities.taskKillingState) {
result.insert(FrameworkInfo::Capability::TASK_KILLING_STATE);
}
if (capabilities.gpuResources) {
result.insert(FrameworkInfo::Capability::GPU_RESOURCES);
}
if (capabilities.sharedResources) {
result.insert(FrameworkInfo::Capability::SHARED_RESOURCES);
}
if (capabilities.partitionAware) {
result.insert(FrameworkInfo::Capability::PARTITION_AWARE);
}
if (capabilities.multiRole) {
result.insert(FrameworkInfo::Capability::MULTI_ROLE);
}
return result;
};
set<FrameworkInfo::Capability> expected = { REVOCABLE_RESOURCES };
EXPECT_EQ(expected, toSet(protobuf::framework::Capabilities(expected));
expected = { TASK_KILLING_STATE };
EXPECT_EQ(expected, toSet(protobuf::framework::Capabilities(expected));
expected = { GPU_RESOURCES };
EXPECT_EQ(expected, toSet(protobuf::framework::Capabilities(expected));
expected = { SHARED_RESOURCES };
EXPECT_EQ(expected, toSet(protobuf::framework::Capabilities(expected));
expected = { PARTITION_AWARE };
EXPECT_EQ(expected, toSet(protobuf::framework::Capabilities(expected));
expected = { MULTI_ROLE };
EXPECT_EQ(expected, toSet(protobuf::framework::Capabilities(expected));
}
```
This isn't a lot more code but it tests it more comprehensively.
- Benjamin Mahler
On Jan. 6, 2017, 7:21 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55021/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2017, 7:21 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Master, agent and allocator structs could use this stuct to easily
> deal with Framework Capabilities.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp f225bb0f5196f7f4b12be25828361a38e1d42e60
> src/tests/protobuf_utils_tests.cpp 4ce952a7d3901a4362aa69f199b135b53d14a723
>
> Diff: https://reviews.apache.org/r/55021/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55021: Added a framework capabilities struct.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55021/#review161610
-----------------------------------------------------------
src/tests/protobuf_utils_tests.cpp (line 65)
<https://reviews.apache.org/r/55021/#comment232869>
Also, let's call this 'FrameworkCapabilties' since we're planning to introduce capabilities for different components.
- Benjamin Mahler
On Jan. 6, 2017, 7:21 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55021/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2017, 7:21 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Master, agent and allocator structs could use this stuct to easily
> deal with Framework Capabilities.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp f225bb0f5196f7f4b12be25828361a38e1d42e60
> src/tests/protobuf_utils_tests.cpp 4ce952a7d3901a4362aa69f199b135b53d14a723
>
> Diff: https://reviews.apache.org/r/55021/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 55021: Added a framework capabilities struct.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55021/
-----------------------------------------------------------
(Updated Jan. 6, 2017, 3:21 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
Changes
-------
addressed guangya's and benb's comments
Repository: mesos
Description
-------
Master, agent and allocator structs could use this stuct to easily
deal with Framework Capabilities.
Diffs (updated)
-----
src/common/protobuf_utils.hpp f225bb0f5196f7f4b12be25828361a38e1d42e60
src/tests/protobuf_utils_tests.cpp 4ce952a7d3901a4362aa69f199b135b53d14a723
Diff: https://reviews.apache.org/r/55021/diff/
Testing
-------
make check
Thanks,
Jay Guo