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 57336: Updated `MultiRoleSchedulerUpgrade` to test framework updates.
Date Mon, 06 Mar 2017 22:18:31 GMT

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


Fix it, then Ship it!




Looks good, wonder if we can just loop over the master and agent pids and use the same code
to check the contents since they should be the same?


src/tests/upgrade_tests.cpp
Lines 475-543 (patched)
<https://reviews.apache.org/r/57336/#comment240120>

    Hm.. have you considered having checking both the agent and master endpoint result with
the same code?
    
    E.g.
    
    ```
    // Check that the master and agent endpoints reflect the
    // update to use the `roles` field.
    foreach (const UPID& upid, { master.get()->pid, agent.get()->pid }) {
      // ...
    }
    ```



src/tests/upgrade_tests.cpp
Lines 489-505 (patched)
<https://reviews.apache.org/r/57336/#comment240100>

    Have you tried using the contains member function to clean this up?
    
    ```
    // We check that the following is contained within
    // the result:
    //   {
    //     "frameworks":
    //     [
    //       {
    //         "roles": ["role"]
    //       }
    //     ]
    //   }
    ```
    
    But since we can't do the following:
    
    ```
    JSON::Object expected = {
      {"frameworks", { {{"roles", {"foo"} }} }}
    };
    ```
    
    It ends up being pretty tedious.



src/tests/upgrade_tests.cpp
Lines 489-505 (patched)
<https://reviews.apache.org/r/57336/#comment240111>

    Do you want to guard the .as calls with some .is ASSERTs?



src/tests/upgrade_tests.cpp
Lines 508-509 (patched)
<https://reviews.apache.org/r/57336/#comment240110>

    You might want a Clock::settle here to ensure it's processed should the handling of updateFrameworkMessage
become asynchronous in the agent.



src/tests/upgrade_tests.cpp
Lines 519-541 (patched)
<https://reviews.apache.org/r/57336/#comment240113>

    Ditto here.


- Benjamin Mahler


On March 6, 2017, 1:19 p.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57336/
> -----------------------------------------------------------
> 
> (Updated March 6, 2017, 1:19 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Updated `MultiRoleSchedulerUpgrade` to test framework updates.
> 
> 
> Diffs
> -----
> 
>   src/tests/upgrade_tests.cpp 6cdd6d989df14af5f17b41af0bf631471feae00e 
> 
> 
> Diff: https://reviews.apache.org/r/57336/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Michael Park
> 
>


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