-----------------------------------------------------------
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
> 
>

Reply via email to