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


Fix it, then Ship it!





src/executor/executor.cpp (line 424)
<https://reviews.apache.org/r/44580/#comment185433>

    kill this.



src/executor/executor.cpp (line 444)
<https://reviews.apache.org/r/44580/#comment185432>

    This if statement sounds like all combinations of "checkpoint", 
"connecting" and "recoveryTimeout" are possible? I don't think that's the case?
    
    Should this be?
    
    ```
    if (recoveryTimer.isSome()) {
      CHECK(checkpoint);
      CHECK_EQ(CONNECTING, state);
      
      return;
    }
    ```



src/executor/executor.cpp (line 498)
<https://reviews.apache.org/r/44580/#comment185434>

    kill this?



src/executor/executor.cpp (lines 500 - 502)
<https://reviews.apache.org/r/44580/#comment185435>

    This comment talks about the case when we should return. But the code below 
is when we shouln't return.
    
    How about
    
    ```
    //...
    if (recoveryTimer.isNone() || !recoveryTimer->timeout().expired) {
      return;
    }
    
    ```


- Vinod Kone


On March 9, 2016, 6:03 p.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44580/
> -----------------------------------------------------------
> 
> (Updated March 9, 2016, 6:03 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4858
>     https://issues.apache.org/jira/browse/MESOS-4858
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This change makes the following modifications to the library:
> 
> - Removes passing connection objects to `defer` callbacks as it can
> sometimes lead to deadlocks around destruction in the same execution
> context.
> - Introduced 3 additional states `CONNECTING`, `SUBSCRIBING` and
> `SUBSCRIBED`. The `CONNECTING` state helps us in identifying if a
> connection attempt is in progress while the latter two states allows
> us to drop subscribe calls if one is already is in progress.
> - Creates a random `connectionID` to demarcate a new connection
> instance and  allowing to discard a state connection attempt.
> - Changes around setting the recovery timeout timer only once.
> This allows us to later discard the recoveryTimeout callback
> if we connected with the agent at a later point of time.
> 
> 
> Diffs
> -----
> 
>   src/executor/executor.cpp c3e95ea7e4edf78f2a65ddc15e213aba66e69db2 
> 
> Diff: https://reviews.apache.org/r/44580/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Reply via email to