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

Reply via email to