mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ben Mahler <benjamin.mah...@gmail.com>
Subject Re: Review Request 43488: Adding framework capability for TASK_KILLING.
Date Sat, 20 Feb 2016 12:44:23 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43488/#review120039
-----------------------------------------------------------


Fix it, then Ship it!




Thanks! I'll make the adjustments here since they're minor.


include/mesos/mesos.proto (line 261)
<https://reviews.apache.org/r/43488/#comment181413>

    Newline here. How about the following:
    
    ```
          // Receive the TASK_KILLING TaskState when a task is being
          // killed by an executor. The executor will examine this
          // capability to determine whether it can send TASK_KILLING.
          TASK_KILLING_STATE = 2;
    ```



src/tests/master_tests.cpp (lines 3019 - 3024)
<https://reviews.apache.org/r/43488/#comment181415>

    We would typically stick a newline in front of the 'capabilityType' variable since there
is an expression above it.
    
    How about 'capabilities' as a name? Note that this is a plural name which indicates to
the reader that this is a container rather than a single capability.
    
    How about using an initializer list here? Altogether:
    
    ```
      FrameworkInfo framework = DEFAULT_FRAMEWORK_INFO;
      framework.set_webui_url("http://localhost:8080/");
    
      vector<FrameworkInfo::Capability::Type> capabilities = {
        FrameworkInfo::Capability::REVOCABLE_RESOURCES,
        FrameworkInfo::Capability::TASK_KILLING_STATE
      };
    
      foreach (FrameworkInfo::Capability::Type capability, capabilities) {
        framework.add_capabilities()->set_type(capability);
      }
    ```



src/tests/master_tests.cpp (lines 3060 - 3069)
<https://reviews.apache.org/r/43488/#comment181416>

    There are a few places where you're using 'as' here directly without first asserting that
it 'is' the right type, so the test can crash if the assumptions don't hold. We'd rather have
assertions fail than have the entire test suite crash so it would be great to check 'is' before
using 'as':
    
    ```
      EXPECT_EQ(1u, framework_.values.count("capabilities"));
      ASSERT_TRUE(framework_.values["capabilities"].is<JSON::Array>());
    
      vector<FrameworkInfo::Capability::Type> actual;
    
      foreach (const JSON::Value& capability,
               framework_.values["capabilities"].as<JSON::Array>().values) {
        FrameworkInfo::Capability::Type type;
    
        ASSERT_TRUE(capability.is<JSON::String>());
        ASSERT_TRUE(
            FrameworkInfo::Capability::Type_Parse(
                capability.as<JSON::String>().value,
                &type));
    
        actual.push_back(type);
      }
    
      EXPECT_EQ(capabilities, actual);
    ```
    
    I'll make sure to update this test so that there isn't 'as' usage without an assertion
on 'is'.


- Ben Mahler


On Feb. 16, 2016, 3:30 p.m., Abhishek Dasgupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43488/
> -----------------------------------------------------------
> 
> (Updated Feb. 16, 2016, 3:30 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
>     https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adding framework capability for TASK_KILLING.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 5d1a20b4ff647f43701a353472b56a4b74b6bbc3 
>   include/mesos/v1/mesos.proto 415a20d9f3385e9627635dd89d78c58d03e3c116 
>   src/tests/master_tests.cpp 393a6f5fe3744d6ba743f362b7e309d1ee75a303 
> 
> Diff: https://reviews.apache.org/r/43488/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>


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