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 2016/12/24 00:12:10 UTC
Review Request 55021: Added a framework capabilities struct.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55021/
-----------------------------------------------------------
Review request for mesos, 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
Diff: https://reviews.apache.org/r/55021/diff/
Testing
-------
Thanks,
Jay Guo
Re: Review Request 55021: Added a framework capabilities struct.
Posted by Guangya Liu <gy...@gmail.com>.
> On \u5341\u4e8c\u6708 28, 2016, 9:24 a.m., Guangya Liu wrote:
> > src/common/protobuf_utils.hpp, lines 238-243
> > <https://reviews.apache.org/r/55021/diff/2/?file=1592796#file1592796line238>
> >
> > I'd like we keep a comment for each of the capability here and also keep the variable same as before.
> > ```
> > // Whether the framework desires revocable resources.
> > bool revocable = false;
> >
> > // Whether the framework can receive TASK_KILLING TaskState
> > // when a task is being killed.
> > bool taskKillingAware = false;
> >
> > // Whether the framework is aware of GPU resources. See
> > // the documentation for the GPU_RESOURCES Capability.
> > bool gpuAware = false;
> >
> > // Whether the framework desires shared resources.
> > bool shared = false;
> >
> > // Whether the framework is prepared to handle the following
> > // TaskStates: TASK_UNREACHABLE, TASK_DROPPED, TASK_GONE,
> > // TASK_GONE_BY_OPERATOR, and TASK_UNKNOWN, and whether the
> > // framework will assume responsibility for managing
> > // partitioned tasks that reregister with the master.
> > bool partitionAware = false;
> >
> > // Whether the framework support `multi-tenancy`.
> > bool multiRole = false;
> > ```
>
> Jay Guo wrote:
> previous naming seems more reasonable from literal perspective but as the # of caps grows, it looks more straightforward to me to keep the consistency. what do you think?
I have no comments for this but it is better leave some comments for each capability as before.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55021/#review160217
-----------------------------------------------------------
On \u5341\u4e8c\u6708 29, 2016, 1:37 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55021/
> -----------------------------------------------------------
>
> (Updated \u5341\u4e8c\u6708 29, 2016, 1:37 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>.
> On Dec. 28, 2016, 5:24 p.m., Guangya Liu wrote:
> > src/common/protobuf_utils.hpp, line 209
> > <https://reviews.apache.org/r/55021/diff/2/?file=1592796#file1592796line209>
> >
> > You may need a default constructor for `Capabilities` here, otherwise, the `hierarhical.cpp` will be failed when construct the `Framework`.
yes that's right.. I forgot to push the updated patch...
> On Dec. 28, 2016, 5:24 p.m., Guangya Liu wrote:
> > src/common/protobuf_utils.hpp, lines 238-243
> > <https://reviews.apache.org/r/55021/diff/2/?file=1592796#file1592796line238>
> >
> > I'd like we keep a comment for each of the capability here and also keep the variable same as before.
> > ```
> > // Whether the framework desires revocable resources.
> > bool revocable = false;
> >
> > // Whether the framework can receive TASK_KILLING TaskState
> > // when a task is being killed.
> > bool taskKillingAware = false;
> >
> > // Whether the framework is aware of GPU resources. See
> > // the documentation for the GPU_RESOURCES Capability.
> > bool gpuAware = false;
> >
> > // Whether the framework desires shared resources.
> > bool shared = false;
> >
> > // Whether the framework is prepared to handle the following
> > // TaskStates: TASK_UNREACHABLE, TASK_DROPPED, TASK_GONE,
> > // TASK_GONE_BY_OPERATOR, and TASK_UNKNOWN, and whether the
> > // framework will assume responsibility for managing
> > // partitioned tasks that reregister with the master.
> > bool partitionAware = false;
> >
> > // Whether the framework support `multi-tenancy`.
> > bool multiRole = false;
> > ```
previous naming seems more reasonable from literal perspective but as the # of caps grows, it looks more straightforward to me to keep the consistency. what do you think?
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55021/#review160217
-----------------------------------------------------------
On Dec. 28, 2016, 4:32 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55021/
> -----------------------------------------------------------
>
> (Updated Dec. 28, 2016, 4:32 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 Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55021/#review160217
-----------------------------------------------------------
src/common/protobuf_utils.hpp (line 209)
<https://reviews.apache.org/r/55021/#comment231313>
You may need a default constructor for `Capabilities` here, otherwise, the `hierarhical.cpp` will be failed when construct the `Framework`.
src/common/protobuf_utils.hpp (line 212)
<https://reviews.apache.org/r/55021/#comment231311>
s/cap/capability
Mesos perfer using long name which is easy to understand.
src/common/protobuf_utils.hpp (lines 238 - 243)
<https://reviews.apache.org/r/55021/#comment231314>
I'd like we keep a comment for each of the capability here and also keep the variable same as before.
```
// Whether the framework desires revocable resources.
bool revocable = false;
// Whether the framework can receive TASK_KILLING TaskState
// when a task is being killed.
bool taskKillingAware = false;
// Whether the framework is aware of GPU resources. See
// the documentation for the GPU_RESOURCES Capability.
bool gpuAware = false;
// Whether the framework desires shared resources.
bool shared = false;
// Whether the framework is prepared to handle the following
// TaskStates: TASK_UNREACHABLE, TASK_DROPPED, TASK_GONE,
// TASK_GONE_BY_OPERATOR, and TASK_UNKNOWN, and whether the
// framework will assume responsibility for managing
// partitioned tasks that reregister with the master.
bool partitionAware = false;
// Whether the framework support `multi-tenancy`.
bool multiRole = false;
```
src/tests/protobuf_utils_tests.cpp (lines 78 - 83)
<https://reviews.apache.org/r/55021/#comment231315>
Group the TRUE and FALSE cases, also the variable names may need some update accoding to my above comments.
```
ASSERT_TRUE(capabilities.revocableResources);
ASSERT_TRUE(capabilities.partitionAware);
ASSERT_TRUE(capabilities.gpuResources);
ASSERT_FALSE(capabilities.sharedResources);
ASSERT_FALSE(capabilities.taskKillingState);
ASSERT_FALSE(capabilities.multiRole);
```
- Guangya Liu
On \u5341\u4e8c\u6708 28, 2016, 8:32 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55021/
> -----------------------------------------------------------
>
> (Updated \u5341\u4e8c\u6708 28, 2016, 8:32 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 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
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. 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 Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55021/
-----------------------------------------------------------
(Updated Dec. 29, 2016, 9:37 a.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
Changes
-------
addressed some of guangya'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
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 Dec. 28, 2016, 4:32 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 (updated)
-----
src/common/protobuf_utils.hpp f225bb0f5196f7f4b12be25828361a38e1d42e60
src/tests/protobuf_utils_tests.cpp 4ce952a7d3901a4362aa69f199b135b53d14a723
Diff: https://reviews.apache.org/r/55021/diff/
Testing (updated)
-------
make check
Thanks,
Jay Guo