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 56941: Add a test for a MULTI_ROLE master re-registering an old agent.
Date Thu, 23 Feb 2017 01:21:49 GMT

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



Looks good overall, left some suggestions below. Main thing is I think we can simplify the
test a bit if we don't need the mock executor and test containerizer. Also, seems these types
of tests should be collected in a file called upgrade_tests.cpp so that we can more easily
migrate them towards an approach that actually spins up different versions of components.


src/tests/master_tests.cpp (lines 6690 - 6695)
<https://reviews.apache.org/r/56941/#comment238406>

    I would suggest moving this into a file called upgrade_tests.cpp with a comment at the
top like the following:
    
    ```
    // This file contains upgrade integration tests. Note that tests
    // in this file can only "spoof" an old version of a component,
    // since these run against the current version of the repository.
    // The long term plan for integration tests is to checkout
    // different releases of the repository and run components from
    // different releases against each other.
    ```
    
    That way we can hopefully collect all of the upgrade tests together, and ensure that these
tests are migrated to tests that actually spin up different versions.



src/tests/master_tests.cpp (lines 6690 - 6694)
<https://reviews.apache.org/r/56941/#comment238413>

    We should clarify here that we start a regular agent to get a task launched, then we restart
the master and when the agent re-registers we strip the allocation info in order to spoof
an old (non-MULTI_ROLE agent).



src/tests/master_tests.cpp (lines 6704 - 6705)
<https://reviews.apache.org/r/56941/#comment238407>

    Why did you need a mock executor and test containerizer? Seems to me this can be simplified
by just running a sleep task with no mocking?
    
    E.g.
    
    ```
    TaskInfo task = createTask(offers1.get()[0], "sleep 1000");
    ```
    
    You can find examples with:
    
    ```
    $ grep -R -i sleep src/test
    ```



src/tests/master_tests.cpp (line 6783)
<https://reviews.apache.org/r/56941/#comment238410>

    Is this flaky? Looking at the agent source, I believe this has to be `slaveFlags.registration_backoff_factor
* 2`, no?



src/tests/master_tests.cpp (lines 6796 - 6798)
<https://reviews.apache.org/r/56941/#comment238412>

    You can wrap your loops like this if it fits in 80 characters:
    
    ```
      foreach (ExecutorInfo& executorInfo,
               *strippedReregisterSlaveMessage.mutable_executor_infos()) {
    ```



src/tests/master_tests.cpp (line 6834)
<https://reviews.apache.org/r/56941/#comment238408>

    Can you do a sweep for `s/.get()./->/` ?



src/tests/master_tests.cpp (line 6838)
<https://reviews.apache.org/r/56941/#comment238409>

    Feel free to use `.at("frameworks")` to avoid mutation here, ditto below for "tasks" and
"executors"


- Benjamin Mahler


On Feb. 22, 2017, 5:43 p.m., Jay Guo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56941/
> -----------------------------------------------------------
> 
> (Updated Feb. 22, 2017, 5:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael
Park.
> 
> 
> Bugs: MESOS-7063
>     https://issues.apache.org/jira/browse/MESOS-7063
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> When a non-MULTI_ROLE agent with running tasks re-registers, master
> should inject allocation roles for tasks/executors sent during
> re-registration.
> 
> 
> Diffs
> -----
> 
>   src/tests/master_tests.cpp 357a9a47ef87be6cbc4256d8afabe63cd41b1ea1 
> 
> Diff: https://reviews.apache.org/r/56941/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>


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