You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2015/07/15 03:47:27 UTC
Review Request 36497: Implemented the SUBSCRIBE Event handler in the
scheduler driver.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36497/
-----------------------------------------------------------
Review request for mesos and Vinod Kone.
Bugs: MESOS-2910
https://issues.apache.org/jira/browse/MESOS-2910
Repository: mesos
Description
-------
This one is non-trivial, note that we follow MESOS-786 with the exception of the 3rd case, since it is not possible and schedulers could not have possibly relied on getting registered instead of re-registered in that case.
We now need to store the MasterInfo coming from the detector, as it is not provided in Event.
Diffs
-----
src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f
src/tests/scheduler_event_call_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/36497/diff/
Testing
-------
Added a test.
Thanks,
Ben Mahler
Re: Review Request 36497: Implemented the SUBSCRIBE Event handler in
the scheduler driver.
Posted by Ben Mahler <be...@gmail.com>.
> On July 16, 2015, 6:41 p.m., Vinod Kone wrote:
> >
Split the test apart.
> On July 16, 2015, 6:41 p.m., Vinod Kone wrote:
> > src/sched/sched.cpp, line 1393
> > <https://reviews.apache.org/r/36497/diff/1/?file=1011992#file1011992line1393>
> >
> > Why store both 'master' and 'masterInfo'? I think you can get rid of 'master'. Gets us away from having to make sure they are in sync.
Done, pulled this out into a separate review.
> On July 16, 2015, 6:41 p.m., Vinod Kone wrote:
> > src/tests/scheduler_event_call_tests.cpp, line 59
> > <https://reviews.apache.org/r/36497/diff/1/?file=1011993#file1011993line59>
> >
> > Can you expand on the comment here? This test is a bit complicated and could use some comments on what you are doing and testing.
Split the test per your other suggestion.
> On July 16, 2015, 6:41 p.m., Vinod Kone wrote:
> > src/tests/scheduler_event_call_tests.cpp, lines 83-84
> > <https://reviews.apache.org/r/36497/diff/1/?file=1011993#file1011993line83>
> >
> > Why are you doing this test?
To get the framework id from the message, is there a better way?
> On July 16, 2015, 6:41 p.m., Vinod Kone wrote:
> > src/tests/scheduler_event_call_tests.cpp, lines 89-91
> > <https://reviews.apache.org/r/36497/diff/1/?file=1011993#file1011993line89>
> >
> > pull this below the expectation.
Hm.. will require a sweep across all the tests here, so I'll punt since it's not posssible to do consistently in the tests (some tests require things from the event in the expectation). Ok?
> On July 16, 2015, 6:41 p.m., Vinod Kone wrote:
> > src/tests/scheduler_event_call_tests.cpp, line 101
> > <https://reviews.apache.org/r/36497/diff/1/?file=1011993#file1011993line101>
> >
> > s/call/message/
It's the call that I'm testing, not the message :)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36497/#review91934
-----------------------------------------------------------
On July 15, 2015, 1:47 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36497/
> -----------------------------------------------------------
>
> (Updated July 15, 2015, 1:47 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-2910
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This one is non-trivial, note that we follow MESOS-786 with the exception of the 3rd case, since it is not possible and schedulers could not have possibly relied on getting registered instead of re-registered in that case.
>
> We now need to store the MasterInfo coming from the detector, as it is not provided in Event.
>
>
> Diffs
> -----
>
> src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f
> src/tests/scheduler_event_call_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36497/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 36497: Implemented the SUBSCRIBE Event handler in
the scheduler driver.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36497/#review91934
-----------------------------------------------------------
src/sched/sched.cpp (lines 453 - 454)
<https://reviews.apache.org/r/36497/#comment145667>
Do you want to add a CHECK to make sure that 'master' is also None? I propose that we get rid of 'master' altogether. See my comment below.
src/sched/sched.cpp (line 463)
<https://reviews.apache.org/r/36497/#comment145669>
except for the 3rd case. can you add that to the comment. you can copy paste what you wrote in the description of this review.
src/sched/sched.cpp (line 1393)
<https://reviews.apache.org/r/36497/#comment145665>
Why store both 'master' and 'masterInfo'? I think you can get rid of 'master'. Gets us away from having to make sure they are in sync.
src/tests/scheduler_event_call_tests.cpp (line 59)
<https://reviews.apache.org/r/36497/#comment145670>
Can you expand on the comment here? This test is a bit complicated and could use some comments on what you are doing and testing.
src/tests/scheduler_event_call_tests.cpp (line 73)
<https://reviews.apache.org/r/36497/#comment145674>
Needs a comment on why you are dropping the message.
src/tests/scheduler_event_call_tests.cpp (line 76)
<https://reviews.apache.org/r/36497/#comment145681>
why pausing the clock? comment.
src/tests/scheduler_event_call_tests.cpp (lines 83 - 84)
<https://reviews.apache.org/r/36497/#comment145671>
Why are you doing this test?
src/tests/scheduler_event_call_tests.cpp (lines 89 - 91)
<https://reviews.apache.org/r/36497/#comment145673>
pull this below the expectation.
src/tests/scheduler_event_call_tests.cpp (line 97)
<https://reviews.apache.org/r/36497/#comment145672>
comment.
src/tests/scheduler_event_call_tests.cpp (line 101)
<https://reviews.apache.org/r/36497/#comment145675>
s/call/message/
src/tests/scheduler_event_call_tests.cpp (line 115)
<https://reviews.apache.org/r/36497/#comment145676>
comment.
src/tests/scheduler_event_call_tests.cpp (lines 119 - 121)
<https://reviews.apache.org/r/36497/#comment145677>
hmm. can you split this into its own test.
this test is huge!
src/tests/scheduler_event_call_tests.cpp (line 139)
<https://reviews.apache.org/r/36497/#comment145679>
comment.
src/tests/scheduler_event_call_tests.cpp (line 145)
<https://reviews.apache.org/r/36497/#comment145678>
ditto. split this into its own test.
src/tests/scheduler_event_call_tests.cpp (line 158)
<https://reviews.apache.org/r/36497/#comment145680>
comment.
- Vinod Kone
On July 15, 2015, 1:47 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36497/
> -----------------------------------------------------------
>
> (Updated July 15, 2015, 1:47 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-2910
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This one is non-trivial, note that we follow MESOS-786 with the exception of the 3rd case, since it is not possible and schedulers could not have possibly relied on getting registered instead of re-registered in that case.
>
> We now need to store the MasterInfo coming from the detector, as it is not provided in Event.
>
>
> Diffs
> -----
>
> src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f
> src/tests/scheduler_event_call_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36497/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 36497: Implemented the SUBSCRIBE Event handler in
the scheduler driver.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36497/#review92116
-----------------------------------------------------------
Ship it!
src/sched/sched.cpp (line 453)
<https://reviews.apache.org/r/36497/#comment146054>
there is no master UPID anymore. update the comment?
src/sched/sched.cpp (line 465)
<https://reviews.apache.org/r/36497/#comment146055>
s/required/relied/ ?
- Vinod Kone
On July 17, 2015, 1:36 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36497/
> -----------------------------------------------------------
>
> (Updated July 17, 2015, 1:36 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-2910
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This one is non-trivial, note that we follow MESOS-786 with the exception of the 3rd case, since it is not possible and schedulers could not have possibly relied on getting registered instead of re-registered in that case.
>
> We now need to store the MasterInfo coming from the detector, as it is not provided in Event.
>
>
> Diffs
> -----
>
> src/sched/sched.cpp 839fcbf0169fecf74fb17cab94fe4d35a1e20a10
> src/tests/scheduler_event_call_tests.cpp cf6aa19a644580ff9d805e919763e9342d72677f
>
> Diff: https://reviews.apache.org/r/36497/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 36497: Implemented the SUBSCRIBE Event handler in
the scheduler driver.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36497/
-----------------------------------------------------------
(Updated July 17, 2015, 1:36 a.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Split apart the test.
Bugs: MESOS-2910
https://issues.apache.org/jira/browse/MESOS-2910
Repository: mesos
Description
-------
This one is non-trivial, note that we follow MESOS-786 with the exception of the 3rd case, since it is not possible and schedulers could not have possibly relied on getting registered instead of re-registered in that case.
We now need to store the MasterInfo coming from the detector, as it is not provided in Event.
Diffs (updated)
-----
src/sched/sched.cpp 839fcbf0169fecf74fb17cab94fe4d35a1e20a10
src/tests/scheduler_event_call_tests.cpp cf6aa19a644580ff9d805e919763e9342d72677f
Diff: https://reviews.apache.org/r/36497/diff/
Testing
-------
Added a test.
Thanks,
Ben Mahler