Review Request 36497: Implemented the SUBSCRIBE Event handler in the scheduler driver.

2015-07-14 Thread Ben Mahler 

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

2015-07-16 Thread Vinod Kone 

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



src/sched/sched.cpp (lines 453 - 454)


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)


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)


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)


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)


Needs a comment on why you are dropping the message.



src/tests/scheduler_event_call_tests.cpp (line 76)


why pausing the clock? comment.



src/tests/scheduler_event_call_tests.cpp (lines 83 - 84)


Why are you doing this test?



src/tests/scheduler_event_call_tests.cpp (lines 89 - 91)


pull this below the expectation.



src/tests/scheduler_event_call_tests.cpp (line 97)


comment.



src/tests/scheduler_event_call_tests.cpp (line 101)


s/call/message/



src/tests/scheduler_event_call_tests.cpp (line 115)


comment.



src/tests/scheduler_event_call_tests.cpp (lines 119 - 121)


hmm. can you split this into its own test.

this test is huge!



src/tests/scheduler_event_call_tests.cpp (line 139)


comment.



src/tests/scheduler_event_call_tests.cpp (line 145)


ditto. split this into its own test.



src/tests/scheduler_event_call_tests.cpp (line 158)


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.

2015-07-16 Thread Ben Mahler 


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

2015-07-16 Thread Ben Mahler 

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

Re: Review Request 36497: Implemented the SUBSCRIBE Event handler in the scheduler driver.

2015-07-17 Thread Vinod Kone 

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


there is no master UPID anymore. update the comment?



src/sched/sched.cpp (line 465)


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