You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2015/04/01 02:20:18 UTC
Re: Review Request 32505: Added SHUTDOWN scheduler call.
-----------------------------------------------------------
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>.
> 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
>
>