You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Quinn Leng <qu...@gmail.com> on 2017/07/31 17:31:00 UTC
Review Request 61262: Added 'heartbeat' event for the operator API.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar and Greg Mann.
Repository: mesos
Description
-------
Added the 'HEARTBEAT' event for the operator API, modified other
related test cases to accept heartbeats.
Diffs
-----
include/mesos/master/master.proto 7a722a6deac484f28a85f63fd68ef49cb6a47606
include/mesos/v1/master/master.proto cf88ea64fa08e4bdcf685de4def28d374eb32fdf
src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70
src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043
src/master/master.cpp e12c997dad04f8a4ddb47a993a84b2b05c9e2f32
src/tests/api_tests.cpp f22ca28c819712d8797e0c0c69dc1ebf1fe5ac1f
Diff: https://reviews.apache.org/r/61262/diff/1/
Testing
-------
make check -j48
Thanks,
Quinn Leng
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 6, 2018, 2:44 a.m., Zhitao Li wrote:
> > src/master/http.cpp
> > Lines 845-857 (original), 845-861 (patched)
> > <https://reviews.apache.org/r/61262/diff/8/?file=1800242#file1800242line845>
> >
> > I believe this is a behavior change.
> >
> > Previous, `master->subscribe(...)` call will not trigger additional event being sent, so `SUBSCRIBED` is still guaranteed to be the first event.
> >
> > However, we start the `heartbeater` process immediately without wait for `SUBSCRIBED` to be sent, so this could mean that subscriber can receive the `SUBSCRIBED` event after heartbeater sends something.
> >
> > Should reorder the call so we only call `master->subscribe(http);` after the `http.send` on `SUBSCRIBED`?
Reordering is an option, but I wonder if this will really be an issue in practice? We delay the first heartbeat event sent by the Heartbeater by DEFAULT_HEARTBEAT_INTERVAL: https://github.com/apache/mesos/blob/0d247c3887ea08b6273992218cd5899010d89fed/src/master/master.hpp#L1986
so the Heartbeater sends its first heartbeat event after that interval elapses. So, the heartbeat event could only arrive first in the presence of some extreme CPU contention, it doesn't seem likely/possible to me? WDYT?
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/#review198678
-----------------------------------------------------------
On Aug. 18, 2017, 6:54 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 6:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Greg Mann.
>
>
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b
> include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
>
>
> Diff: https://reviews.apache.org/r/61262/diff/8/
>
>
> Testing
> -------
>
> make check -j48
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Zhitao Li <zh...@gmail.com>.
> On March 6, 2018, 2:44 a.m., Zhitao Li wrote:
> > src/master/http.cpp
> > Lines 845-857 (original), 845-861 (patched)
> > <https://reviews.apache.org/r/61262/diff/8/?file=1800242#file1800242line845>
> >
> > I believe this is a behavior change.
> >
> > Previous, `master->subscribe(...)` call will not trigger additional event being sent, so `SUBSCRIBED` is still guaranteed to be the first event.
> >
> > However, we start the `heartbeater` process immediately without wait for `SUBSCRIBED` to be sent, so this could mean that subscriber can receive the `SUBSCRIBED` event after heartbeater sends something.
> >
> > Should reorder the call so we only call `master->subscribe(http);` after the `http.send` on `SUBSCRIBED`?
>
> Greg Mann wrote:
> Reordering is an option, but I wonder if this will really be an issue in practice? We delay the first heartbeat event sent by the Heartbeater by DEFAULT_HEARTBEAT_INTERVAL: https://github.com/apache/mesos/blob/0d247c3887ea08b6273992218cd5899010d89fed/src/master/master.hpp#L1986
> so the Heartbeater sends its first heartbeat event after that interval elapses. So, the heartbeat event could only arrive first in the presence of some extreme CPU contention, it doesn't seem likely/possible to me? WDYT?
I think in large cluster, the first SUBSCRIBED message could take longer than `DEFAULT_HEARTBEAT_INTERVAL` under extreme cases. In practice this has hit our cluster maybe less than 10% of master failovers.
Still, I would argue that we should ensure `SUBSCRIBED` message is sent before any `HEARTBEAT`, also because subscriber cannot event process the `HEARTBEAT` without the `heartbeat_interval_seconds` value from `SUBSCRIBED` message.
- Zhitao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/#review198678
-----------------------------------------------------------
On Aug. 18, 2017, 6:54 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 6:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Greg Mann.
>
>
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b
> include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
>
>
> Diff: https://reviews.apache.org/r/61262/diff/8/
>
>
> Testing
> -------
>
> make check -j48
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/#review198678
-----------------------------------------------------------
src/master/http.cpp
Lines 845-857 (original), 845-861 (patched)
<https://reviews.apache.org/r/61262/#comment278922>
I believe this is a behavior change.
Previous, `master->subscribe(...)` call will not trigger additional event being sent, so `SUBSCRIBED` is still guaranteed to be the first event.
However, we start the `heartbeater` process immediately without wait for `SUBSCRIBED` to be sent, so this could mean that subscriber can receive the `SUBSCRIBED` event after heartbeater sends something.
Should reorder the call so we only call `master->subscribe(http);` after the `http.send` on `SUBSCRIBED`?
- Zhitao Li
On Aug. 18, 2017, 6:54 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 6:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Greg Mann.
>
>
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b
> include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
>
>
> Diff: https://reviews.apache.org/r/61262/diff/8/
>
>
> Testing
> -------
>
> make check -j48
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/#review183259
-----------------------------------------------------------
Patch looks great!
Reviews applied: [61262]
Logs available here: http://104.210.40.105/logs/master/61262
- Mesos Reviewbot Windows
On Aug. 18, 2017, 6:54 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 6:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Greg Mann.
>
>
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b
> include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
>
>
> Diff: https://reviews.apache.org/r/61262/diff/8/
>
>
> Testing
> -------
>
> make check -j48
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/#review183251
-----------------------------------------------------------
Patch looks great!
Reviews applied: [61262]
Passed command: support\windows-build.bat
- Mesos Reviewbot Windows
On Aug. 18, 2017, 6:54 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 6:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Greg Mann.
>
>
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b
> include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
>
>
> Diff: https://reviews.apache.org/r/61262/diff/8/
>
>
> Testing
> -------
>
> make check -j48
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Quinn Leng <qu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/
-----------------------------------------------------------
(Updated Aug. 18, 2017, 6:54 p.m.)
Review request for mesos, Anand Mazumdar and Greg Mann.
Bugs: MESOS-7695
https://issues.apache.org/jira/browse/MESOS-7695
Repository: mesos
Description
-------
Added the 'HEARTBEAT' event for the operator API, modified other
related test cases to accept heartbeats.
Diffs (updated)
-----
include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b
include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8
src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
Diff: https://reviews.apache.org/r/61262/diff/8/
Changes: https://reviews.apache.org/r/61262/diff/7-8/
Testing
-------
make check -j48
Thanks,
Quinn Leng
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/#review183244
-----------------------------------------------------------
src/master/master.hpp
Lines 309-312 (patched)
<https://reviews.apache.org/r/61262/#comment259266>
Could you add something about the `delay` parameter here?
src/master/master.hpp
Lines 321 (patched)
<https://reviews.apache.org/r/61262/#comment259267>
Let's give this a default value of `None()`.
src/master/master.hpp
Lines 2762 (patched)
<https://reviews.apache.org/r/61262/#comment259268>
If you give this parameter a default value, you could get rid of this.
- Greg Mann
On Aug. 18, 2017, 6:12 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 6:12 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Greg Mann.
>
>
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b
> include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
>
>
> Diff: https://reviews.apache.org/r/61262/diff/7/
>
>
> Testing
> -------
>
> make check -j48
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Quinn Leng <qu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/
-----------------------------------------------------------
(Updated Aug. 18, 2017, 6:12 p.m.)
Review request for mesos, Anand Mazumdar and Greg Mann.
Bugs: MESOS-7695
https://issues.apache.org/jira/browse/MESOS-7695
Repository: mesos
Description
-------
Added the 'HEARTBEAT' event for the operator API, modified other
related test cases to accept heartbeats.
Diffs (updated)
-----
include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b
include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8
src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
Diff: https://reviews.apache.org/r/61262/diff/7/
Changes: https://reviews.apache.org/r/61262/diff/6-7/
Testing
-------
make check -j48
Thanks,
Quinn Leng
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/#review183163
-----------------------------------------------------------
src/master/master.hpp
Lines 309 (patched)
<https://reviews.apache.org/r/61262/#comment259187>
Could you also explain the template parameters here? i.e. the first parameter represents the type of the input message, the second is the type of the event we send on the connection.
Also please explain the purpose of the `delay` parameter to the constructor.
src/master/master.hpp
Lines 310 (patched)
<https://reviews.apache.org/r/61262/#comment259186>
I think you can get rid of the default template parameter ` = v1::scheduler::Event` here, since you specify it explicitly in both cases.
src/master/master.hpp
Lines 329-333 (patched)
<https://reviews.apache.org/r/61262/#comment259200>
I think making `delay` an `Option<Duration>` is more appropriate here. Then this check could be `if (delay.isSome())`
- Greg Mann
On Aug. 17, 2017, 9:20 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2017, 9:20 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Greg Mann.
>
>
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b
> include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
>
>
> Diff: https://reviews.apache.org/r/61262/diff/6/
>
>
> Testing
> -------
>
> make check -j48
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/#review183166
-----------------------------------------------------------
Patch looks great!
Reviews applied: [61262]
Passed command: support\windows-build.bat
- Mesos Reviewbot Windows
On Aug. 17, 2017, 2:20 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2017, 2:20 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Greg Mann.
>
>
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b
> include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
>
>
> Diff: https://reviews.apache.org/r/61262/diff/6/
>
>
> Testing
> -------
>
> make check -j48
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Quinn Leng <qu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/
-----------------------------------------------------------
(Updated Aug. 17, 2017, 9:20 p.m.)
Review request for mesos, Anand Mazumdar and Greg Mann.
Bugs: MESOS-7695
https://issues.apache.org/jira/browse/MESOS-7695
Repository: mesos
Description
-------
Added the 'HEARTBEAT' event for the operator API, modified other
related test cases to accept heartbeats.
Diffs (updated)
-----
include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b
include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8
src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
Diff: https://reviews.apache.org/r/61262/diff/6/
Changes: https://reviews.apache.org/r/61262/diff/5-6/
Testing
-------
make check -j48
Thanks,
Quinn Leng
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/#review182915
-----------------------------------------------------------
Bad patch!
Reviews applied: [61262]
Failed command: python support/apply-reviews.py -n -r 61262
Error:
error: patch failed: src/tests/api_tests.cpp:1590
error: src/tests/api_tests.cpp: patch does not apply
Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/223/console
- Mesos Reviewbot Windows
On Aug. 14, 2017, 9:51 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2017, 9:51 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Greg Mann.
>
>
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b
> include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/tests/api_tests.cpp 75f7a585ec75f1d8b78453dd203bb78822ee7bc9
>
>
> Diff: https://reviews.apache.org/r/61262/diff/4/
>
>
> Testing
> -------
>
> make check -j48
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/#review182953
-----------------------------------------------------------
I think this needs a rebase?
- Greg Mann
On Aug. 14, 2017, 9:51 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2017, 9:51 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Greg Mann.
>
>
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b
> include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/tests/api_tests.cpp 75f7a585ec75f1d8b78453dd203bb78822ee7bc9
>
>
> Diff: https://reviews.apache.org/r/61262/diff/4/
>
>
> Testing
> -------
>
> make check -j48
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Quinn Leng <qu...@gmail.com>.
> On Aug. 15, 2017, 6:44 p.m., Greg Mann wrote:
> > src/master/master.hpp
> > Lines 309-311 (patched)
> > <https://reviews.apache.org/r/61262/diff/5/?file=1797997#file1797997line309>
> >
> > Now that we're sending heartbeats for multiple purposes, I'm concerned that the logging will be ambiguous. Instead of templating this class on the ID type, perhaps it makes more sense to initialize it with a string that it can use for logging purposes. For example:
> >
> > ```
> > class Heartbeater : public process::Process<Heartbeater>
> > {
> > public:
> > Heartbeater(const string& _message,
> > const HttpConnection& _http,
> > const Duration& _interval,
> > const Duration& _delay = Seconds(0))
> > : process::ProcessBase(process::ID::generate("heartbeater")),
> > message(_message),
> > http(_http),
> > interval(_interval),
> > delay(_delay) {}
> >
> > ...
> >
> > private:
> > void heartbeat()
> > {
> > // Only send a heartbeat if the connection is not closed.
> > if (http.closed().isPending()) {
> > VLOG(1) << "Sending heartbeat to " << message;
> >
> > etc...
> > ```
> >
> > and then for the framework case we would do something like:
> > ```
> > heartbeater = new Heartbeater<FrameworkID>(
> > "framework " + stringify(info.id()),
> > http.get(),
> > DEFAULT_HEARTBEAT_INTERVAL);
> > ```
> >
> > and the subscriber case something like:
> > ```
> > heartbeater = new Heartbeater<UUID>(
> > "subscriber " + stringify(http.streamId),
> > http,
> > DEFAULT_HEARTBEAT_INTERVAL,
> > DEFAULT_HEARTBEAT_INTERVAL);
> > ```
> >
> > what do you think?
I am also thinking about that too previously. Just not sure if this is compatible with our furture implementation.
- Quinn
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/#review182958
-----------------------------------------------------------
On Aug. 14, 2017, 9:51 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2017, 9:51 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Greg Mann.
>
>
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b
> include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
>
>
> Diff: https://reviews.apache.org/r/61262/diff/5/
>
>
> Testing
> -------
>
> make check -j48
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/#review182958
-----------------------------------------------------------
src/master/master.hpp
Lines 309-311 (patched)
<https://reviews.apache.org/r/61262/#comment258950>
Now that we're sending heartbeats for multiple purposes, I'm concerned that the logging will be ambiguous. Instead of templating this class on the ID type, perhaps it makes more sense to initialize it with a string that it can use for logging purposes. For example:
```
class Heartbeater : public process::Process<Heartbeater>
{
public:
Heartbeater(const string& _message,
const HttpConnection& _http,
const Duration& _interval,
const Duration& _delay = Seconds(0))
: process::ProcessBase(process::ID::generate("heartbeater")),
message(_message),
http(_http),
interval(_interval),
delay(_delay) {}
...
private:
void heartbeat()
{
// Only send a heartbeat if the connection is not closed.
if (http.closed().isPending()) {
VLOG(1) << "Sending heartbeat to " << message;
etc...
```
and then for the framework case we would do something like:
```
heartbeater = new Heartbeater<FrameworkID>(
"framework " + stringify(info.id()),
http.get(),
DEFAULT_HEARTBEAT_INTERVAL);
```
and the subscriber case something like:
```
heartbeater = new Heartbeater<UUID>(
"subscriber " + stringify(http.streamId),
http,
DEFAULT_HEARTBEAT_INTERVAL,
DEFAULT_HEARTBEAT_INTERVAL);
```
what do you think?
src/tests/api_tests.cpp
Lines 2381 (patched)
<https://reviews.apache.org/r/61262/#comment258960>
How about:
"Verifies that 'HEARTBEAT' events are sent at the correct times."
src/tests/api_tests.cpp
Lines 2444 (patched)
<https://reviews.apache.org/r/61262/#comment258961>
You should also verify that the type of the event is correct here.
- Greg Mann
On Aug. 14, 2017, 9:51 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2017, 9:51 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Greg Mann.
>
>
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b
> include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
>
>
> Diff: https://reviews.apache.org/r/61262/diff/5/
>
>
> Testing
> -------
>
> make check -j48
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Quinn Leng <qu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/
-----------------------------------------------------------
(Updated Aug. 14, 2017, 9:51 p.m.)
Review request for mesos, Anand Mazumdar and Greg Mann.
Bugs: MESOS-7695
https://issues.apache.org/jira/browse/MESOS-7695
Repository: mesos
Description
-------
Added the 'HEARTBEAT' event for the operator API, modified other
related test cases to accept heartbeats.
Diffs (updated)
-----
include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b
include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8
src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
src/tests/api_tests.cpp 75f7a585ec75f1d8b78453dd203bb78822ee7bc9
Diff: https://reviews.apache.org/r/61262/diff/4/
Changes: https://reviews.apache.org/r/61262/diff/3-4/
Testing
-------
make check -j48
Thanks,
Quinn Leng
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Quinn Leng <qu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/#review182627
-----------------------------------------------------------
src/master/master.hpp
Lines 1826 (patched)
<https://reviews.apache.org/r/61262/#comment258602>
Move the templated Heartbeater before these usages in subscriber and framework.
src/master/master.hpp
Line 1830 (original), 1874 (patched)
<https://reviews.apache.org/r/61262/#comment258603>
don't have to pass in the uuid, also not necessary to keep it as a class memeber.
src/tests/api_tests.cpp
Lines 2189 (patched)
<https://reviews.apache.org/r/61262/#comment258604>
put int into the for bracket.
- Quinn Leng
On July 31, 2017, 5:30 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> -----------------------------------------------------------
>
> (Updated July 31, 2017, 5:30 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Greg Mann.
>
>
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto 0e4c30e3df704a929a3bd2e1787a76421d14a983
> include/mesos/v1/master/master.proto c04fd1602396a58086331f5fa56518c5dee9af89
> src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70
> src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043
> src/master/master.cpp 7bb3adf48431cd97b3c404b8a1eba4ac6a01efcc
> src/tests/api_tests.cpp 1d5b080c809248bdf4c76ddad382d714692c804b
>
>
> Diff: https://reviews.apache.org/r/61262/diff/3/
>
>
> Testing
> -------
>
> make check -j48
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/#review182735
-----------------------------------------------------------
Patch looks great!
Reviews applied: [61262]
Logs available here: http://104.210.40.105/logs/master/61262
- Mesos Reviewbot Windows
On July 31, 2017, 5:30 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> -----------------------------------------------------------
>
> (Updated July 31, 2017, 5:30 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Greg Mann.
>
>
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto 0e4c30e3df704a929a3bd2e1787a76421d14a983
> include/mesos/v1/master/master.proto c04fd1602396a58086331f5fa56518c5dee9af89
> src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70
> src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043
> src/master/master.cpp 7bb3adf48431cd97b3c404b8a1eba4ac6a01efcc
> src/tests/api_tests.cpp 1d5b080c809248bdf4c76ddad382d714692c804b
>
>
> Diff: https://reviews.apache.org/r/61262/diff/3/
>
>
> Testing
> -------
>
> make check -j48
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61262: Added 'heartbeat' event for the operator
API.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61262/#review182746
-----------------------------------------------------------
Patch looks great!
Reviews applied: [61262]
Logs available here: http://104.210.40.105/logs/master/61262
- Mesos Reviewbot Windows
On July 31, 2017, 10:30 a.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> -----------------------------------------------------------
>
> (Updated July 31, 2017, 10:30 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Greg Mann.
>
>
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto 0e4c30e3df704a929a3bd2e1787a76421d14a983
> include/mesos/v1/master/master.proto c04fd1602396a58086331f5fa56518c5dee9af89
> src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70
> src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043
> src/master/master.cpp 7bb3adf48431cd97b3c404b8a1eba4ac6a01efcc
> src/tests/api_tests.cpp 1d5b080c809248bdf4c76ddad382d714692c804b
>
>
> Diff: https://reviews.apache.org/r/61262/diff/3/
>
>
> Testing
> -------
>
> make check -j48
>
>
> Thanks,
>
> Quinn Leng
>
>