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