mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 55021: Added a framework capabilities struct.
Date Fri, 13 Jan 2017 22:41:07 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message