----------------------------------------------------------- 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 > >
