mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anand Mazumdar <mazumdar.an...@gmail.com>
Subject Re: Review Request 44278: Added HTTP scheduler stream IDs.
Date Wed, 02 Mar 2016 22:03:21 GMT

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



Looks good. Mostly comments around having explicit `CHECK`'s if the stream id header is missing.


src/master/http.cpp (line 464)
<https://reviews.apache.org/r/44278/#comment183528>

    Remove period at the end.



src/master/http.cpp (line 468)
<https://reviews.apache.org/r/44278/#comment183527>

    hmm.. the `framework->http` access can crash the master.
    
    Consider this scenario, if a driver based framework, now tries to send a non-subscribe
call. In that case `framework->http` would be `None()`.
    
    We should do the following after L459.
    
    ```cpp
    if (framework->http.isNone()) {
      return Forbidden("Framework not connected via HTTP");
    }
    ```



src/scheduler/scheduler.cpp (lines 253 - 256)
<https://reviews.apache.org/r/44278/#comment183529>

    Let's make this an explicit check since we are sure that the master would set it:
    
    ```
    CHECK_SOME(stream);
    ```



src/scheduler/scheduler.cpp (line 518)
<https://reviews.apache.org/r/44278/#comment183531>

    Let's have this as an explicit `CHECK` along with the other checks on L504 and remove
this line.


- Anand Mazumdar


On March 2, 2016, 8:58 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44278/
> -----------------------------------------------------------
> 
> (Updated March 2, 2016, 8:58 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3583
>     https://issues.apache.org/jira/browse/MESOS-3583
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added HTTP scheduler stream IDs.
> 
> In some failure scenarios involving highly-available HTTP schedulers with multiple instances,
it's possible for a non-leading instance to successfully make HTTP calls to the master. This
patch enables the master to use HTTP scheduler stream IDs to uniquely identify each HTTP subscription
stream, preventing any non-leading scheduler instance from making calls to the master. The
patch also adds stream ID support to the HTTP scheduler library.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 5e9e28e904ba0045ee27eb828f47231632a91d74 
>   src/master/master.hpp 13c6ff153e77c527822309e787942eb463d59e7d 
>   src/scheduler/scheduler.cpp 7ea1c2567f37a73160bca346a25bb2f0c54e71a0 
> 
> Diff: https://reviews.apache.org/r/44278/diff/
> 
> 
> Testing
> -------
> 
> `make check` was used to test on both OSX and CentOS 7.1.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


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