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 43662: Added support for pipelining calls to the scheduler library.
Date Wed, 24 Feb 2016 03:03:27 GMT

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




src/scheduler/scheduler.cpp (lines 117 - 119)
<https://reviews.apache.org/r/43662/#comment181864>

    This comment is old. Can you update?
    
    s/receiving messages (eventually events/HTTP messages/
    
    s/sending messages (via calls)/HTTP messages/



src/scheduler/scheduler.cpp (line 207)
<https://reviews.apache.org/r/43662/#comment181865>

    I think for both scheduler::Mesos and executor::Mesos we need to add more comments around
send() semantics. 
    
    For example, we should mention that clients should send() only after connected() callback
has been called.



src/scheduler/scheduler.cpp (lines 237 - 242)
<https://reviews.apache.org/r/43662/#comment181866>

    Shouldn't we do the same for executor library?
    
    More importnatly, what if a previous subscribe is in progress but `subscribed` is not
set?



src/scheduler/scheduler.cpp (line 264)
<https://reviews.apache.org/r/43662/#comment181872>

    CHECK(state == DISCONNECTED) ?



src/scheduler/scheduler.cpp (lines 278 - 280)
<https://reviews.apache.org/r/43662/#comment181873>

    s/i.e./e.g.,/ ?
    
    s/we failed over to a new master/master failed over/
    
    Also, not sure what you wanted to say in this comment? Are you saying it is OK even if
a new master is enqueued because....?



src/scheduler/scheduler.cpp (line 297)
<https://reviews.apache.org/r/43662/#comment181876>

    This is the current master as detected by the detector. What is the guarantee that the
connection was made with this master and not an old master? IOW, should this method take `master`
as an argument?
    
    Can we guarantee that the master hasn't changed because a new master detected causes these
futures to be discarded synchronously? Looking at detected() that doesn't seem to be the case.
    
    Do we want to enforce the invariant that there is ever only one connection attempt in
progress?



src/scheduler/scheduler.cpp (line 370)
<https://reviews.apache.org/r/43662/#comment181886>

    is it possible for this callback to be invoked on the scheduler after they get a connected
callback from this new connection attempt?



src/scheduler/scheduler.cpp (line 400)
<https://reviews.apache.org/r/43662/#comment181891>

    What happens if a previous connection attempt is in progress?



src/scheduler/scheduler.cpp (line 457)
<https://reviews.apache.org/r/43662/#comment181887>

    For executor library, we did a close() here of the previous reader. Why was it required
there and not here?


- Vinod Kone


On Feb. 22, 2016, 8:19 p.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43662/
> -----------------------------------------------------------
> 
> (Updated Feb. 22, 2016, 8:19 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3570
>     https://issues.apache.org/jira/browse/MESOS-3570
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously, the scheduler library used to chain calls on previous call responses. This
was inherently slow. This change adds support for pipelining all calls to the master on a
single connection via the `http::Connection` abstraction in libprocess.
> 
> This change also adds support for handling various error scenarios when we notice a disconnection
instead of just relying on the master detector for invoking the `disconnected` callback.
> 
> 
> Diffs
> -----
> 
>   src/scheduler/scheduler.cpp 99a7d0dfff7b0c61decc9ff6d9e6d46ef13a7e75 
> 
> Diff: https://reviews.apache.org/r/43662/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>


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