You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2015/03/26 00:12:05 UTC
Review Request 32505: Added SHUTDOWN scheduler call.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32505/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Ben Mahler.
Bugs: MESOS-1127 and MESOS-330
https://issues.apache.org/jira/browse/MESOS-1127
https://issues.apache.org/jira/browse/MESOS-330
Repository: mesos
Description
-------
This is a new call added to explicitly shutdown an executor.
Diffs
-----
include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8
src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d
src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1
src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8
src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a
src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae
src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16
src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657
Diff: https://reviews.apache.org/r/32505/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 32505: Added SHUTDOWN scheduler call.
Posted by Vinod Kone <vi...@gmail.com>.
> On March 27, 2015, 3:22 p.m., Alexander Rojas wrote:
> > src/scheduler/scheduler.cpp, lines 679-680
> > <https://reviews.apache.org/r/32505/diff/1/?file=906104#file906104line679>
> >
> > Do you mind adding a comment why an exited executor is always marked as a failure? It is not clear to me.
This is the only event present in mesos.proto to capture terminated executors. I agree that a 'FAILURE' event is misleading. I've added a comment on the design doc. Will also add it on the proto.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32505/#review78050
-----------------------------------------------------------
On March 31, 2015, 12:09 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32505/
> -----------------------------------------------------------
>
> (Updated March 31, 2015, 12:09 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1127 and MESOS-330
> https://issues.apache.org/jira/browse/MESOS-1127
> https://issues.apache.org/jira/browse/MESOS-330
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a new call added to explicitly shutdown an executor.
>
>
> Diffs
> -----
>
> include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8
> src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8
> src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a
> src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae
> src/slave/slave.cpp 0f70ebafb0f5b16c560decc66e22a43a52732d09
> src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657
>
> Diff: https://reviews.apache.org/r/32505/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 32505: Added SHUTDOWN scheduler call.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32505/#review78050
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/32505/#comment126439>
I think the common pattern is to do early return on failure, i.e.:
```cpp
if (framework == NULL) {
return;
}
…
```
src/scheduler/scheduler.cpp
<https://reviews.apache.org/r/32505/#comment126449>
Do you mind adding a comment why an exited executor is always marked as a failure? It is not clear to me.
- Alexander Rojas
On March 26, 2015, 12:12 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32505/
> -----------------------------------------------------------
>
> (Updated March 26, 2015, 12:12 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1127 and MESOS-330
> https://issues.apache.org/jira/browse/MESOS-1127
> https://issues.apache.org/jira/browse/MESOS-330
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a new call added to explicitly shutdown an executor.
>
>
> Diffs
> -----
>
> include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8
> src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d
> src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1
> src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8
> src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a
> src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae
> src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16
> src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657
>
> Diff: https://reviews.apache.org/r/32505/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 32505: Added SHUTDOWN scheduler call.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32505/#review81020
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Rukletsov
On April 20, 2015, 8:01 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32505/
> -----------------------------------------------------------
>
> (Updated April 20, 2015, 8:01 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1127 and MESOS-330
> https://issues.apache.org/jira/browse/MESOS-1127
> https://issues.apache.org/jira/browse/MESOS-330
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a new call added to explicitly shutdown an executor.
>
>
> Diffs
> -----
>
> include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8
> src/master/master.hpp c10e7c08c191acef9d31d98018a47a2a952a4dfc
> src/master/master.cpp e30b951eda2b3b0d5b2a80716f0b32c6bbe041bc
> src/messages/messages.proto 2d242dcfc54f9146721a1b8fbef02e4de050cf58
> src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a
> src/slave/slave.hpp 9495c704ca4bde4ab283d12efa3ea9b2f1158a4c
> src/slave/slave.cpp 8ec80ed26f338690e0a1e712065750ab77a724cd
> src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657
>
> Diff: https://reviews.apache.org/r/32505/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 32505: Added SHUTDOWN scheduler call.
Posted by Vinod Kone <vi...@gmail.com>.
> On Nov. 20, 2015, 6:36 p.m., Zhitao Li wrote:
> > src/master/master.cpp, line 3498
> > <https://reviews.apache.org/r/32505/diff/5/?file=939961#file939961line3498>
> >
> > Hi [~vinodkone], can you please explain what it takes to "repliabily forward" this message? Do you mean we should perform some kind of retry action, or send this message at a different message path?
> >
> > I'm working on [MESOS-313](https://issues.apache.org/jira/browse/MESOS-313) and would like to get it to the last mile.
> >
> > Thanks!
I haven't thought through on how to solve this yet, but yes, there needs to be some kind of retry and acknowledgement mechanism involved.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32505/#review107389
-----------------------------------------------------------
On April 22, 2015, 9:41 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32505/
> -----------------------------------------------------------
>
> (Updated April 22, 2015, 9:41 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1127 and MESOS-330
> https://issues.apache.org/jira/browse/MESOS-1127
> https://issues.apache.org/jira/browse/MESOS-330
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a new call added to explicitly shutdown an executor.
>
>
> Diffs
> -----
>
> include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8
> src/master/master.hpp 550d2c50cd01aa5830a7e7e03ab4a0240c221b15
> src/master/master.cpp f3462d15005e24ab28e8265484b0d3810f21bd47
> src/messages/messages.proto bdf474b6614fad6eebdd79c636e820f9c30cf0c9
> src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a
> src/slave/slave.hpp d214ddb168298071dd90ad6d7f9bc6fd3d78d986
> src/slave/slave.cpp 60345ec0d05a8150e61d71a6510824a7f2ca9524
> src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657
>
> Diff: https://reviews.apache.org/r/32505/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 32505: Added SHUTDOWN scheduler call.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32505/#review107389
-----------------------------------------------------------
src/master/master.cpp (line 3498)
<https://reviews.apache.org/r/32505/#comment166484>
Hi [~vinodkone], can you please explain what it takes to "repliabily forward" this message? Do you mean we should perform some kind of retry action, or send this message at a different message path?
I'm working on [MESOS-313](https://issues.apache.org/jira/browse/MESOS-313) and would like to get it to the last mile.
Thanks!
src/slave/slave.cpp (line 3274)
<https://reviews.apache.org/r/32505/#comment166485>
Same question, can you explain what it takes for "reliabily forward this message"?
- Zhitao Li
On April 22, 2015, 9:41 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32505/
> -----------------------------------------------------------
>
> (Updated April 22, 2015, 9:41 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1127 and MESOS-330
> https://issues.apache.org/jira/browse/MESOS-1127
> https://issues.apache.org/jira/browse/MESOS-330
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a new call added to explicitly shutdown an executor.
>
>
> Diffs
> -----
>
> include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8
> src/master/master.hpp 550d2c50cd01aa5830a7e7e03ab4a0240c221b15
> src/master/master.cpp f3462d15005e24ab28e8265484b0d3810f21bd47
> src/messages/messages.proto bdf474b6614fad6eebdd79c636e820f9c30cf0c9
> src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a
> src/slave/slave.hpp d214ddb168298071dd90ad6d7f9bc6fd3d78d986
> src/slave/slave.cpp 60345ec0d05a8150e61d71a6510824a7f2ca9524
> src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657
>
> Diff: https://reviews.apache.org/r/32505/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 32505: Added SHUTDOWN scheduler call.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32505/
-----------------------------------------------------------
(Updated April 22, 2015, 9:41 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
rebased. NNFR.
Bugs: MESOS-1127 and MESOS-330
https://issues.apache.org/jira/browse/MESOS-1127
https://issues.apache.org/jira/browse/MESOS-330
Repository: mesos
Description
-------
This is a new call added to explicitly shutdown an executor.
Diffs (updated)
-----
include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8
src/master/master.hpp 550d2c50cd01aa5830a7e7e03ab4a0240c221b15
src/master/master.cpp f3462d15005e24ab28e8265484b0d3810f21bd47
src/messages/messages.proto bdf474b6614fad6eebdd79c636e820f9c30cf0c9
src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a
src/slave/slave.hpp d214ddb168298071dd90ad6d7f9bc6fd3d78d986
src/slave/slave.cpp 60345ec0d05a8150e61d71a6510824a7f2ca9524
src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657
Diff: https://reviews.apache.org/r/32505/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 32505: Added SHUTDOWN scheduler call.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32505/
-----------------------------------------------------------
(Updated April 20, 2015, 8:01 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
Alex's. NNFR.
Bugs: MESOS-1127 and MESOS-330
https://issues.apache.org/jira/browse/MESOS-1127
https://issues.apache.org/jira/browse/MESOS-330
Repository: mesos
Description
-------
This is a new call added to explicitly shutdown an executor.
Diffs (updated)
-----
include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8
src/master/master.hpp c10e7c08c191acef9d31d98018a47a2a952a4dfc
src/master/master.cpp e30b951eda2b3b0d5b2a80716f0b32c6bbe041bc
src/messages/messages.proto 2d242dcfc54f9146721a1b8fbef02e4de050cf58
src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a
src/slave/slave.hpp 9495c704ca4bde4ab283d12efa3ea9b2f1158a4c
src/slave/slave.cpp 8ec80ed26f338690e0a1e712065750ab77a724cd
src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657
Diff: https://reviews.apache.org/r/32505/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 32505: Added SHUTDOWN scheduler call.
Posted by Vinod Kone <vi...@gmail.com>.
> On April 8, 2015, 5:52 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, line 3475
> > <https://reviews.apache.org/r/32505/diff/3/?file=915052#file915052line3475>
> >
> > How about moving this after `Master::reconcile()`?
i'm keeping them close to their continuations for ease in readability in cpp.
> On April 8, 2015, 5:52 p.m., Alexander Rukletsov wrote:
> > src/tests/scheduler_tests.cpp, line 325
> > <https://reviews.apache.org/r/32505/diff/3/?file=915057#file915057line325>
> >
> > s/> >/>> here and below.
fixed here and everywhere else in this file.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32505/#review79369
-----------------------------------------------------------
On April 3, 2015, 11:38 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32505/
> -----------------------------------------------------------
>
> (Updated April 3, 2015, 11:38 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1127 and MESOS-330
> https://issues.apache.org/jira/browse/MESOS-1127
> https://issues.apache.org/jira/browse/MESOS-330
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a new call added to explicitly shutdown an executor.
>
>
> Diffs
> -----
>
> include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8
> src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8
> src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a
> src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae
> src/slave/slave.cpp 0f70ebafb0f5b16c560decc66e22a43a52732d09
> src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657
>
> Diff: https://reviews.apache.org/r/32505/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 32505: Added SHUTDOWN scheduler call.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32505/#review79369
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/32505/#comment128640>
How about moving this after `Master::reconcile()`?
src/tests/scheduler_tests.cpp
<https://reviews.apache.org/r/32505/#comment128645>
s/> >/>> here and below.
src/tests/scheduler_tests.cpp
<https://reviews.apache.org/r/32505/#comment128648>
Mind leaving a comment why we expect failure here? Maybe it makes sense to write a short comment to the test as well.
- Alexander Rukletsov
On April 3, 2015, 11:38 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32505/
> -----------------------------------------------------------
>
> (Updated April 3, 2015, 11:38 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1127 and MESOS-330
> https://issues.apache.org/jira/browse/MESOS-1127
> https://issues.apache.org/jira/browse/MESOS-330
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a new call added to explicitly shutdown an executor.
>
>
> Diffs
> -----
>
> include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8
> src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8
> src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a
> src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae
> src/slave/slave.cpp 0f70ebafb0f5b16c560decc66e22a43a52732d09
> src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657
>
> Diff: https://reviews.apache.org/r/32505/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 32505: Added SHUTDOWN scheduler call.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32505/
-----------------------------------------------------------
(Updated April 3, 2015, 11:38 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's comments.
Bugs: MESOS-1127 and MESOS-330
https://issues.apache.org/jira/browse/MESOS-1127
https://issues.apache.org/jira/browse/MESOS-330
Repository: mesos
Description
-------
This is a new call added to explicitly shutdown an executor.
Diffs (updated)
-----
include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8
src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5
src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8
src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a
src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae
src/slave/slave.cpp 0f70ebafb0f5b16c560decc66e22a43a52732d09
src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657
Diff: https://reviews.apache.org/r/32505/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 32505: Added SHUTDOWN scheduler call.
Posted by Vinod Kone <vi...@gmail.com>.
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > Modulo comments.
> >
> > I noticed you added SlaveID on Shutdown, can you add it to Kill as well?
Yup. Will send a patch for it as well.
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > include/mesos/scheduler/scheduler.proto, line 127
> > <https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line127>
> >
> > Do you want to put this below KILL?
I reordered them all in the very last review of this chain.
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > include/mesos/scheduler/scheduler.proto, line 174
> > <https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line174>
> >
> > "Shuts down a custom executor"
> > "When the executor"
done.
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > include/mesos/scheduler/scheduler.proto, line 176
> > <https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line176>
> >
> > "If the"
done
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > include/mesos/scheduler/scheduler.proto, line 180
> > <https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line180>
> >
> > "transition"
done
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > include/mesos/scheduler/scheduler.proto, line 188
> > <https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line188>
> >
> > Should you add a required SlaveID to Kill as well since that's the same API as Shutdown?
will do it in a separate review.
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > include/mesos/scheduler/scheduler.proto, line 235
> > <https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line235>
> >
> > Below Kill?
Reordered in the last review of this chain.
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > src/master/master.hpp, lines 463-465
> > <https://reviews.apache.org/r/32505/diff/2/?file=910159#file910159line463>
> >
> > Above reconcile if you put it below Kill in the API?
see above
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1608-1613
> > <https://reviews.apache.org/r/32505/diff/2/?file=910160#file910160line1608>
> >
> > Ditto here.
will do in the final review where i reorder everything.
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > src/master/master.cpp, line 3454
> > <https://reviews.apache.org/r/32505/diff/2/?file=910160#file910160line3454>
> >
> > Shouldn't this TODO be in the slave..?
added there too.
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 3475-3493
> > <https://reviews.apache.org/r/32505/diff/2/?file=910160#file910160line3475>
> >
> > Ditto about ordering w.r.t. other calls.
will do once in the final review.
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 3481-3485
> > <https://reviews.apache.org/r/32505/diff/2/?file=910160#file910160line3481>
> >
> > Looks like you forgot to put the `return` statement here? :)
> >
> > It would be great if you could re-use `drop` here, either by doing the `Slave*` lookup in `receive`, much like we do the framework lookup and drop if not found, or by adding a `drop` overload. But the former seems cleaner.
Added return. Good catch!
Not all calls have slave id in them unlike framework id. So looking up Slave* in receive doesn't seem correct?
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > src/slave/slave.cpp, lines 3433-3437
> > <https://reviews.apache.org/r/32505/diff/2/?file=910164#file910164line3433>
> >
> > The top two LOG(WARNING)s are wrapped on the next line, but the next two are wrapped at the first line, mind just wrapping them all at the first line?
done
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > src/slave/slave.cpp, lines 3451-3452
> > <https://reviews.apache.org/r/32505/diff/2/?file=910164#file910164line3451>
> >
> > Mind putting the space in front of "yet" instead of at the end of "not"? Makes it a bit clearer that the spaces are correct IMHO because its quick to scan down a straight line with the eyes as opposed to the jagged end of the lines (and we do this above in this function :)).
> >
> > I remember I discussed wrapping || and && conditionals at the front of the lines (for the same reason), but that ship has sailed..
done
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > src/tests/scheduler_tests.cpp, lines 346-349
> > <https://reviews.apache.org/r/32505/diff/2/?file=910165#file910165line346>
> >
> > Ditto from my other review, don't you need this above the Mesos construction?
no. per my comment in the previous review.
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > src/tests/scheduler_tests.cpp, line 384
> > <https://reviews.apache.org/r/32505/diff/2/?file=910165#file910165line384>
> >
> > newline here?
done
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > src/tests/scheduler_tests.cpp, line 386
> > <https://reviews.apache.org/r/32505/diff/2/?file=910165#file910165line386>
> >
> > newline here?
done
> On April 1, 2015, 12:20 a.m., Ben Mahler wrote:
> > src/tests/scheduler_tests.cpp, line 407
> > <https://reviews.apache.org/r/32505/diff/2/?file=910165#file910165line407>
> >
> > newline here?
done
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32505/#review78415
-----------------------------------------------------------
On March 31, 2015, 12:09 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32505/
> -----------------------------------------------------------
>
> (Updated March 31, 2015, 12:09 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1127 and MESOS-330
> https://issues.apache.org/jira/browse/MESOS-1127
> https://issues.apache.org/jira/browse/MESOS-330
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a new call added to explicitly shutdown an executor.
>
>
> Diffs
> -----
>
> include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8
> src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8
> src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a
> src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae
> src/slave/slave.cpp 0f70ebafb0f5b16c560decc66e22a43a52732d09
> src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657
>
> Diff: https://reviews.apache.org/r/32505/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 32505: Added SHUTDOWN scheduler call.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32505/#review78415
-----------------------------------------------------------
Ship it!
Modulo comments.
I noticed you added SlaveID on Shutdown, can you add it to Kill as well?
include/mesos/scheduler/scheduler.proto
<https://reviews.apache.org/r/32505/#comment127126>
Do you want to put this below KILL?
include/mesos/scheduler/scheduler.proto
<https://reviews.apache.org/r/32505/#comment127142>
"Shuts down a custom executor"
"When the executor"
include/mesos/scheduler/scheduler.proto
<https://reviews.apache.org/r/32505/#comment127144>
"If the"
include/mesos/scheduler/scheduler.proto
<https://reviews.apache.org/r/32505/#comment127146>
"transition"
include/mesos/scheduler/scheduler.proto
<https://reviews.apache.org/r/32505/#comment127151>
How about a newline for readability?
```
// transitions its active tasks to TASK_LOST.
//
// TODO(vinod): ...
```
include/mesos/scheduler/scheduler.proto
<https://reviews.apache.org/r/32505/#comment127192>
Should you add a required SlaveID to Kill as well since that's the same API as Shutdown?
include/mesos/scheduler/scheduler.proto
<https://reviews.apache.org/r/32505/#comment127161>
Below Kill?
src/master/master.hpp
<https://reviews.apache.org/r/32505/#comment127163>
Above reconcile if you put it below Kill in the API?
src/master/master.cpp
<https://reviews.apache.org/r/32505/#comment127164>
Ditto here.
src/master/master.cpp
<https://reviews.apache.org/r/32505/#comment127168>
Whoops, this should already be saying either "exited with status X" or "terminated with signal X".
src/master/master.cpp
<https://reviews.apache.org/r/32505/#comment127170>
Shouldn't this TODO be in the slave..?
src/master/master.cpp
<https://reviews.apache.org/r/32505/#comment127171>
Ditto about ordering w.r.t. other calls.
src/master/master.cpp
<https://reviews.apache.org/r/32505/#comment127172>
Did you forget the 'return' here?
src/master/master.cpp
<https://reviews.apache.org/r/32505/#comment127174>
Looks like you forgot to put the `return` statement here? :)
It would be great if you could re-use `drop` here, either by doing the `Slave*` lookup in `receive`, much like we do the framework lookup and drop if not found, or by adding a `drop` overload. But the former seems cleaner.
src/slave/slave.cpp
<https://reviews.apache.org/r/32505/#comment127181>
"shutdownFramework"..?
Do you need this comment now that you've split out a separate `_shutdownExecutor` function?
src/slave/slave.cpp
<https://reviews.apache.org/r/32505/#comment127187>
The top two LOG(WARNING)s are wrapped on the next line, but the next two are wrapped at the first line, mind just wrapping them all at the first line?
src/slave/slave.cpp
<https://reviews.apache.org/r/32505/#comment127186>
Mind putting the space in front of "yet" instead of at the end of "not"? Makes it a bit clearer that the spaces are correct IMHO because its quick to scan down a straight line with the eyes as opposed to the jagged end of the lines (and we do this above in this function :)).
I remember I discussed wrapping || and && conditionals at the front of the lines (for the same reason), but that ship has sailed..
src/tests/scheduler_tests.cpp
<https://reviews.apache.org/r/32505/#comment127188>
Ditto from my other review, don't you need this above the Mesos construction?
src/tests/scheduler_tests.cpp
<https://reviews.apache.org/r/32505/#comment127189>
newline here?
src/tests/scheduler_tests.cpp
<https://reviews.apache.org/r/32505/#comment127190>
newline here?
src/tests/scheduler_tests.cpp
<https://reviews.apache.org/r/32505/#comment127191>
newline here?
- Ben Mahler
On March 31, 2015, 12:09 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32505/
> -----------------------------------------------------------
>
> (Updated March 31, 2015, 12:09 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1127 and MESOS-330
> https://issues.apache.org/jira/browse/MESOS-1127
> https://issues.apache.org/jira/browse/MESOS-330
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a new call added to explicitly shutdown an executor.
>
>
> Diffs
> -----
>
> include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8
> src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8
> src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a
> src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae
> src/slave/slave.cpp 0f70ebafb0f5b16c560decc66e22a43a52732d09
> src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657
>
> Diff: https://reviews.apache.org/r/32505/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 32505: Added SHUTDOWN scheduler call.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32505/
-----------------------------------------------------------
(Updated March 31, 2015, 12:09 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
alex's comments.
Bugs: MESOS-1127 and MESOS-330
https://issues.apache.org/jira/browse/MESOS-1127
https://issues.apache.org/jira/browse/MESOS-330
Repository: mesos
Description
-------
This is a new call added to explicitly shutdown an executor.
Diffs (updated)
-----
include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8
src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5
src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8
src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a
src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae
src/slave/slave.cpp 0f70ebafb0f5b16c560decc66e22a43a52732d09
src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657
Diff: https://reviews.apache.org/r/32505/diff/
Testing
-------
make check
Thanks,
Vinod Kone