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 59320: Added test case for agent re-registration.
Date Wed, 31 May 2017 00:09:51 GMT

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




src/master/master.cpp
Lines 6018-6025 (original), 6018-6025 (patched)
<https://reviews.apache.org/r/59320/#comment249773>

    Hm.. what does slaveWasRemoved mean? Is it equivalen to the agent being marked as unreachable?
Or is being removed something beyond being marked as unreachable (what it sounds like)? I
had a hard time grasping the code below when it was referring to "unreachable" but the variable
is called "removed".



src/master/master.cpp
Lines 6027-6043 (original), 6027-6043 (patched)
<https://reviews.apache.org/r/59320/#comment249772>

    Hm.. it seems like we could maybe make this code easier to read. Currently, the loop for
dealing with frameworks is down below and the loop for dealing with tasks is up here. However,
it seems to me that these are highly related, and it would be easier to reason about if it
were consolidated into a loop over the framework and then a loop over the tasks belonging
to that framework. That would also eliminate the need for `partitionAwareFrameworks`.
    
    E.g.
    
    ```
      foreach (const FrameworkInfo& framework, frameworks):
        if completed:
          shut down the framework, drop the tasks
          continue
          
        if agent was unreachable/removed and framework is not partition aware:
          shut down the framework, drop the tasks
          continue
        
        for each task in the framework:
          recoveredTasks.push_back(task)
          remove from unreachable tasks
    ```



src/master/master.cpp
Lines 6058 (patched)
<https://reviews.apache.org/r/59320/#comment249766>

    "it" is referring to the task? Threw me off initially, since it sounds like "remove the
framework" given the previous setence.
    
    s/it/the task/ ?



src/tests/partition_tests.cpp
Lines 2387 (patched)
<https://reviews.apache.org/r/59320/#comment249765>

    Hm.. why disabled on windows? Do you know why or is this just copied? For example, it's
not clear to me why the above partition test isn't disabled but the below one is.


- Benjamin Mahler


On May 16, 2017, 8:19 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59320/
> -----------------------------------------------------------
> 
> (Updated May 16, 2017, 8:19 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add an explanatory comment and a test case for a particular case in the
> master's logic for handling agent re-registration.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 4e7a161f431624bd78d3e9032eb8587687149cad 
>   src/tests/partition_tests.cpp 4ff428564d1fa6cb96e6f8ec8edc331da88a3eb6 
> 
> 
> Diff: https://reviews.apache.org/r/59320/diff/2/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> Checked that if you add `CHECK_NOTNULL(framework)` to the master code (circa the newly
added comment), the incorrect `CHECK` is triggered by this test case (but not by any existing
test cases).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


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