Re: Review Request 45202: Add test for rescinding offer trriggered by updating weights.

[Adam B]

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



Great test for the rescind patch! Just a few cleanup suggestions


src/tests/master_allocator_tests.cpp (lines 1559 - 1561)
<https://reviews.apache.org/r/45202/#comment188183>

    You could just check
    ```
    EXPECT_EQ(Resources(framework1offers1.get()[i].resources()),
              Resources::parse(slaveFlags.resources.get()).get());
    ```
    Then you're not planting magic numbers (2, 1024) all over the code.



src/tests/master_allocator_tests.cpp (line 1591)
<https://reviews.apache.org/r/45202/#comment188187>

    Why "AtMost" instead of just `3`?



src/tests/master_allocator_tests.cpp (lines 1616 - 1618)
<https://reviews.apache.org/r/45202/#comment188188>

    same as above



src/tests/master_allocator_tests.cpp (lines 1623 - 1625)
<https://reviews.apache.org/r/45202/#comment188189>

    same as above



src/tests/mesos.hpp (lines 669 - 673)
<https://reviews.apache.org/r/45202/#comment188182>

    Couldn't this just be:
    `return strings::format("%s", JSON::protobuf(infos)).get();`?
    
    And for that matter, is
    `createUpdateRequestBody(infos)`
    so much shorter than 
    `strings::format("%s", JSON::protobuf(infos)).get()`
    that it really needs to be its own function? I don't think think so. Just 
inline this everywhere. Example:
    
    ```
      RepeatedPtrField<WeightInfo>& infos = createWeightInfos("role1=0");
      Future<Response> response = process::http::request(
          process::http::createRequest(
              master.get()->pid,
              "PUT",
              false,
              "weights",
              createBasicAuthHeaders(DEFAULT_CREDENTIAL),
              strings::format("%s", JSON::protobuf(infos)).get()));
    ```


- Adam B


On March 23, 2016, 1:32 a.m., Yongqiao Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45202/
> -----------------------------------------------------------
> 
> (Updated March 23, 2016, 1:32 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-4881
>     https://issues.apache.org/jira/browse/MESOS-4881
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add test for rescinding offer trriggered by updating weights.
> 
> 
> Diffs
> -----
> 
>   src/tests/dynamic_weights_tests.cpp 
> ee0c4b1cc20e76a35a8e4e445f6827a1fc33e6c6 
>   src/tests/master_allocator_tests.cpp 
> b41ba2bda4d680f6fc42f525719973d56c11fe31 
>   src/tests/mesos.hpp aaef158e5784ce077ef60996ebbeb77b356b7c57 
> 
> Diff: https://reviews.apache.org/r/45202/diff/
> 
> 
> Testing
> -------
> 
> make && make check.
> 
> $ ./src/mesos-tests 
> --gtest_filter=MasterAllocatorTest/0.RebalancedForUpdatedWeights
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from MasterAllocatorTest/0, where TypeParam = 
> mesos::internal::master::allocator::MesosAllocator<mesos::internal::master::allocator::HierarchicalAllocatorProcess<mesos::internal::master::allocator::DRFSorter,
>  mesos::internal::master::allocator::DRFSorter> >
> [ RUN      ] MasterAllocatorTest/0.RebalancedForUpdatedWeights
> [       OK ] MasterAllocatorTest/0.RebalancedForUpdatedWeights (1059 ms)
> [----------] 1 test from MasterAllocatorTest/0 (1059 ms total)
> 
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (1070 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>