Re: Review Request 44278: Added HTTP scheduler stream IDs.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 3, 2016, 5:13 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44278/
> ---
> 
> (Updated March 3, 2016, 5:13 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 52b2ceab21bc8cccea1d57385c3f1e14e31f3bce 
>   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
> 
>

Re: Review Request 44278: Added HTTP scheduler stream IDs.

[Greg Mann]

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

(Updated March 3, 2016, 5:13 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Addressed comments.


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 (updated)
-

  src/master/http.cpp 5e9e28e904ba0045ee27eb828f47231632a91d74 
  src/master/master.hpp 52b2ceab21bc8cccea1d57385c3f1e14e31f3bce 
  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

Re: Review Request 44278: Added HTTP scheduler stream IDs.

[Anand Mazumdar]

> On March 3, 2016, 1:37 a.m., Vinod Kone wrote:
> > src/master/http.cpp, lines 458-462
> > 
> >
> > should these be BadRequest as well?

I think `Forbidden` is more apt here. _There is nothing wrong with the request 
object per se i.e. being malformed etc._

In both the cases, the framework is trying to perform an operation that it is 
not allowed to do i.e. trying to send a non-subscribe call without 
subscribing/trying to send a non-subscribe call when previously subscribed as a 
driver based framework.


- Anand


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


On March 2, 2016, 10:36 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, 10:36 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
> 
>

Re: Review Request 44278: Added HTTP scheduler stream IDs.

[Vinod Kone]

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




src/master/http.cpp (line 440)


s/stream/streamId/



src/master/http.cpp (lines 458 - 462)


should these be BadRequest as well?



src/master/master.hpp (line 1605)


s/_stream/_streamId/



src/master/master.hpp (line 1608)


s/stream/streamId/



src/scheduler/scheduler.cpp (lines 501 - 502)


pull this down to #516



src/scheduler/scheduler.cpp (line 678)


s/stream/streamId/


- Vinod Kone


On March 2, 2016, 10:36 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, 10:36 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
> 
>

Re: Review Request 44278: Added HTTP scheduler stream IDs.

[Greg Mann]

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

(Updated March 2, 2016, 10:36 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Addressed comments.


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 (updated)
-

  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

Re: Review Request 44278: Added HTTP scheduler stream IDs.

[Anand Mazumdar]

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


Remove period at the end.



src/master/http.cpp (line 468)


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)


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)


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

Re: Review Request 44278: Added HTTP scheduler stream IDs.

[Greg Mann]

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


Changes
---

Moved new tests into a different patch.


Bugs: MESOS-3583
https://issues.apache.org/jira/browse/MESOS-3583


Repository: mesos


Description (updated)
---

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 (updated)
-

  src/master/http.cpp 5e9e28e904ba0045ee27eb828f47231632a91d74 
  src/master/master.hpp 13c6ff153e77c527822309e787942eb463d59e7d 
  src/scheduler/scheduler.cpp 7ea1c2567f37a73160bca346a25bb2f0c54e71a0 

Diff: https://reviews.apache.org/r/44278/diff/


Testing (updated)
---

`make check` was used to test on both OSX and CentOS 7.1.


Thanks,

Greg Mann

Re: Review Request 44278: Added HTTP scheduler stream IDs.

[Greg Mann]

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

(Updated March 2, 2016, 7:14 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.

Three new tests have been added in this patch: 
SchedulerHttpApiTest.TeardownWithoutStreamId, 
SchedulerHttpApiTest.TeardownWrongStreamId, 
SchedulerHttpApiTest.SubscribeWithStreamId


Diffs
-

  src/master/http.cpp 5e9e28e904ba0045ee27eb828f47231632a91d74 
  src/master/master.hpp 13c6ff153e77c527822309e787942eb463d59e7d 
  src/scheduler/scheduler.cpp 7ea1c2567f37a73160bca346a25bb2f0c54e71a0 
  src/tests/scheduler_http_api_tests.cpp 
428e12646d80b45daec30cfe607b97f36170fdf5 

Diff: https://reviews.apache.org/r/44278/diff/


Testing (updated)
---

New tests were added, and `make check` was used to test on both OSX and CentOS 
7.1.

The new tests were run 1000 times to check for flakiness; no failures were 
observed.


Thanks,

Greg Mann

Review Request 44278: Added HTTP scheduler stream IDs.

[Greg Mann]

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

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.

Three new tests have been added in this patch: 
SchedulerHttpApiTest.TeardownWithoutStreamId, 
SchedulerHttpApiTest.TeardownWrongStreamId, 
SchedulerHttpApiTest.SubscribeWithStreamId


Diffs
-

  src/master/http.cpp 5e9e28e904ba0045ee27eb828f47231632a91d74 
  src/master/master.hpp 13c6ff153e77c527822309e787942eb463d59e7d 
  src/scheduler/scheduler.cpp 7ea1c2567f37a73160bca346a25bb2f0c54e71a0 
  src/tests/scheduler_http_api_tests.cpp 
428e12646d80b45daec30cfe607b97f36170fdf5 

Diff: https://reviews.apache.org/r/44278/diff/


Testing
---

New tests were added, and `make check` was used to test on OSX.

The new tests were run 1000 times to check for flakiness; no failures were 
observed.


Thanks,

Greg Mann