mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vinod Kone <vinodk...@gmail.com>
Subject Re: Review Request 41283: Introduced an Executor Library based on the new executor HTTP API.
Date Mon, 25 Jan 2016 21:58:48 GMT

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




src/executor/executor.cpp (line 274)
<https://reviews.apache.org/r/41283/#comment177180>

    Can you add a comment here that you are making 2 persistent connections here, one for
subscribe call (and its streaming response) and another for nonSubscribe calls.



src/executor/executor.cpp (lines 286 - 288)
<https://reviews.apache.org/r/41283/#comment177181>

    formatting:
    
    disconnected(subscribe,
                 connection1.isFailed()
                   ? connection1.failure()
                   : "....");
                   
    Why 'Subscribe' in quotes and not Non-subscribe below?



src/executor/executor.cpp (line 290)
<https://reviews.apache.org/r/41283/#comment177183>

    no need for `else`. just make this a `if` block.



src/executor/executor.cpp (lines 291 - 294)
<https://reviews.apache.org/r/41283/#comment177182>

    see formatting above.



src/executor/executor.cpp (line 299)
<https://reviews.apache.org/r/41283/#comment177184>

    Maybe mention when state transitions from/to CONNECTED/DISCONNECTED?



src/executor/executor.cpp (line 308)
<https://reviews.apache.org/r/41283/#comment177185>

    ditto. why quotes?



src/executor/executor.cpp (line 310)
<https://reviews.apache.org/r/41283/#comment177186>

    s/Subscribe/subscribe/
    
    Also have this comment up at #291?
    
    More importantly, didn't we decide to close both connections if any of them fails/disconnects?
I don't see that code in this patch?



src/executor/executor.cpp (line 318)
<https://reviews.apache.org/r/41283/#comment177187>

    no need for quotes around Subscribe.
    
    s/Subscribe/subscribe/



src/executor/executor.cpp (line 342)
<https://reviews.apache.org/r/41283/#comment177191>

    shouldn't this be done inside `close()` ?



src/executor/executor.cpp (line 361)
<https://reviews.apache.org/r/41283/#comment177193>

    dont you want to set `subscribe` and `nonSubscribe` to None()?



src/executor/executor.cpp (line 368)
<https://reviews.apache.org/r/41283/#comment177190>

    Why is this outside the `if (state == CONNECTED)` block? I'm guessing you don't want to
fire multiple delays one for subscribe disconnection and nonSubscribe disconnection for example?
    
    If yes, you might just want to do the below at the top of this method.
    
    ```
    if (state == DISCONNECTED) {
      return;
    }
    ```
    
    and merge this if block with the one at #352.


- Vinod Kone


On Jan. 22, 2016, 1:36 a.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> -----------------------------------------------------------
> 
> (Updated Jan. 22, 2016, 1:36 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-3550
>     https://issues.apache.org/jira/browse/MESOS-3550
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This change introduces the implementation for the executor library. 
> 
> This uses the new HTTP Connection interface to ensure calls are properly pipelined.
> 
> `connected` -> Callback invoked when a TCP connection is established with the agent
for the `Subscribe` call.
> `disconnected` -> When the `Subscribe` TCP connection is interrupted possibly due
to an agent restart.
> `received` -> When the executor receives events from the agent upon subscribing.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am d23e35001078a86775bd9b76baa207ecb9dab7e1 
>   src/executor/executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41283/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>


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