You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Isabel Jimenez <co...@isabeljimenez.com> on 2014/07/02 00:46:28 UTC
Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/
-----------------------------------------------------------
(Updated July 1, 2014, 10:46 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
Changes
-------
Changes after Benh's and Adam's comments
Bugs: MESOS-1390
https://issues.apache.org/jira/browse/MESOS-1390
Repository: mesos-git
Description
-------
HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
Diffs (updated)
-----
src/Makefile.am 12d84bf
src/master/http.cpp 5d86976
src/master/master.hpp 5fef354
src/master/master.cpp 474014b
src/slave/http.cpp cd7f692
src/slave/slave.hpp 605ee4a
src/slave/slave.cpp f42ab60
src/tests/shutdown_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22832/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/#review47177
-----------------------------------------------------------
Patch looks great!
Reviews applied: [22832]
All tests passed.
- Mesos ReviewBot
On July 1, 2014, 10:46 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
>
> (Updated July 1, 2014, 10:46 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
>
>
> Bugs: MESOS-1390
> https://issues.apache.org/jira/browse/MESOS-1390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
>
>
> Diffs
> -----
>
> src/Makefile.am 12d84bf
> src/master/http.cpp 5d86976
> src/master/master.hpp 5fef354
> src/master/master.cpp 474014b
> src/slave/http.cpp cd7f692
> src/slave/slave.hpp 605ee4a
> src/slave/slave.cpp f42ab60
> src/tests/shutdown_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22832/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/#review47415
-----------------------------------------------------------
Ship it!
Great! Thanks Isabel!
- Benjamin Hindman
On July 7, 2014, 8:59 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
>
> (Updated July 7, 2014, 8:59 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
>
>
> Bugs: MESOS-1390
> https://issues.apache.org/jira/browse/MESOS-1390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
>
>
> Diffs
> -----
>
> src/Makefile.am e3ff6d7
> src/master/http.cpp 5d86976
> src/master/master.hpp 5fef354
> src/master/master.cpp 251b699
> src/slave/http.cpp cd7f692
> src/slave/slave.hpp 605ee4a
> src/slave/slave.cpp f42ab60
> src/tests/shutdown_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22832/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint
Posted by Adam B <ad...@mesosphere.io>.
> On July 7, 2014, 3:03 p.m., Adam B wrote:
> > src/master/http.cpp, line 637
> > <https://reviews.apache.org/r/22832/diff/3/?file=625040#file625040line637>
> >
> > Can we get some more descriptive error strings, please? ("Mesos master" everywhere may not be descriptive enough when debugging)
>
> Benjamin Hindman wrote:
> This is actually the "realm", not an error message. Do you have a suggestion for a better realm? Thinking about Kerberos, what constraints will we have on the realm (e.g., no spaces)?
Kerberos realms are usually an ALL.UPPERCASE DNS name (no spaces), like APACHE.ORG
See http://www.cmf.nrl.navy.mil/krb/kerberos-faq.html#realms
> On July 7, 2014, 3:03 p.m., Adam B wrote:
> > src/slave/slave.hpp, line 341
> > <https://reviews.apache.org/r/22832/diff/3/?file=625044#file625044line341>
> >
> > Why did the const Slave& have to become a Slave* too?
>
> Benjamin Hindman wrote:
> Done for consistency, since we've also used * for nested classes in other places (the Master::Http and Slave::Http classes were the only remaining ones that I was aware of that didn't pass a *).
Understood & accepted.
> On July 7, 2014, 3:03 p.m., Adam B wrote:
> > src/master/http.cpp, lines 622-623
> > <https://reviews.apache.org/r/22832/diff/3/?file=625040#file625040line622>
> >
> > Does this mean that the /shutdown API only works when authentication is enabled (or at least credentials are provided)? What if I just want to /shutdown on my local test cluster? Shouldn't there be a way to allow /shutdown without authentication?
>
> Benjamin Hindman wrote:
> This is a good point, let's circle back to this in a follow up review Isabel.
Thanks. Should be easy to fix.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/#review47417
-----------------------------------------------------------
On July 7, 2014, 1:59 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
>
> (Updated July 7, 2014, 1:59 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
>
>
> Bugs: MESOS-1390
> https://issues.apache.org/jira/browse/MESOS-1390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
>
>
> Diffs
> -----
>
> src/Makefile.am e3ff6d7
> src/master/http.cpp 5d86976
> src/master/master.hpp 5fef354
> src/master/master.cpp 251b699
> src/slave/http.cpp cd7f692
> src/slave/slave.hpp 605ee4a
> src/slave/slave.cpp f42ab60
> src/tests/shutdown_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22832/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On July 7, 2014, 10:03 p.m., Adam B wrote:
> > src/master/http.cpp, lines 622-623
> > <https://reviews.apache.org/r/22832/diff/3/?file=625040#file625040line622>
> >
> > Does this mean that the /shutdown API only works when authentication is enabled (or at least credentials are provided)? What if I just want to /shutdown on my local test cluster? Shouldn't there be a way to allow /shutdown without authentication?
This is a good point, let's circle back to this in a follow up review Isabel.
> On July 7, 2014, 10:03 p.m., Adam B wrote:
> > src/master/http.cpp, line 637
> > <https://reviews.apache.org/r/22832/diff/3/?file=625040#file625040line637>
> >
> > Can we get some more descriptive error strings, please? ("Mesos master" everywhere may not be descriptive enough when debugging)
This is actually the "realm", not an error message. Do you have a suggestion for a better realm? Thinking about Kerberos, what constraints will we have on the realm (e.g., no spaces)?
> On July 7, 2014, 10:03 p.m., Adam B wrote:
> > src/slave/slave.hpp, line 341
> > <https://reviews.apache.org/r/22832/diff/3/?file=625044#file625044line341>
> >
> > Why did the const Slave& have to become a Slave* too?
Done for consistency, since we've also used * for nested classes in other places (the Master::Http and Slave::Http classes were the only remaining ones that I was aware of that didn't pass a *).
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/#review47417
-----------------------------------------------------------
On July 7, 2014, 8:59 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
>
> (Updated July 7, 2014, 8:59 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
>
>
> Bugs: MESOS-1390
> https://issues.apache.org/jira/browse/MESOS-1390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
>
>
> Diffs
> -----
>
> src/Makefile.am e3ff6d7
> src/master/http.cpp 5d86976
> src/master/master.hpp 5fef354
> src/master/master.cpp 251b699
> src/slave/http.cpp cd7f692
> src/slave/slave.hpp 605ee4a
> src/slave/slave.cpp f42ab60
> src/tests/shutdown_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22832/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/#review47417
-----------------------------------------------------------
Looks great! I just wonder if we should also allow /shutdown to be called on an unauthenticated cluster (no --credentials).
src/master/http.cpp
<https://reviews.apache.org/r/22832/#comment83159>
Does this mean that the /shutdown API only works when authentication is enabled (or at least credentials are provided)? What if I just want to /shutdown on my local test cluster? Shouldn't there be a way to allow /shutdown without authentication?
src/master/http.cpp
<https://reviews.apache.org/r/22832/#comment83160>
Can we get some more descriptive error strings, please? ("Mesos master" everywhere may not be descriptive enough when debugging)
src/master/http.cpp
<https://reviews.apache.org/r/22832/#comment83162>
Please end your comment with punctuation.
src/slave/slave.hpp
<https://reviews.apache.org/r/22832/#comment83156>
Why did the const Slave& have to become a Slave* too?
- Adam B
On July 7, 2014, 1:59 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
>
> (Updated July 7, 2014, 1:59 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
>
>
> Bugs: MESOS-1390
> https://issues.apache.org/jira/browse/MESOS-1390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
>
>
> Diffs
> -----
>
> src/Makefile.am e3ff6d7
> src/master/http.cpp 5d86976
> src/master/master.hpp 5fef354
> src/master/master.cpp 251b699
> src/slave/http.cpp cd7f692
> src/slave/slave.hpp 605ee4a
> src/slave/slave.cpp f42ab60
> src/tests/shutdown_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22832/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/
-----------------------------------------------------------
(Updated July 7, 2014, 8:59 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
Bugs: MESOS-1390
https://issues.apache.org/jira/browse/MESOS-1390
Repository: mesos-git
Description
-------
HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
Diffs (updated)
-----
src/Makefile.am e3ff6d7
src/master/http.cpp 5d86976
src/master/master.hpp 5fef354
src/master/master.cpp 251b699
src/slave/http.cpp cd7f692
src/slave/slave.hpp 605ee4a
src/slave/slave.cpp f42ab60
src/tests/shutdown_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22832/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On July 1, 2014, 11:04 p.m., Dominic Hamon wrote:
> > src/slave/slave.hpp, line 324
> > <https://reviews.apache.org/r/22832/diff/2/?file=622206#file622206line324>
> >
> > is this change just for symmetry with Master::Http?
Yes, this change is just for consistency
> On July 1, 2014, 11:04 p.m., Dominic Hamon wrote:
> > src/master/http.cpp, line 649
> > <https://reviews.apache.org/r/22832/diff/2/?file=622202#file622202line649>
> >
> > how long do you expect this call to take? ie, will the client timeout waiting for a response? should this dispatch the request to the master instead and return Accepted()?
I put on comment for this to be changed in a future patch
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/#review47156
-----------------------------------------------------------
On July 7, 2014, 8:59 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
>
> (Updated July 7, 2014, 8:59 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
>
>
> Bugs: MESOS-1390
> https://issues.apache.org/jira/browse/MESOS-1390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
>
>
> Diffs
> -----
>
> src/Makefile.am e3ff6d7
> src/master/http.cpp 5d86976
> src/master/master.hpp 5fef354
> src/master/master.cpp 251b699
> src/slave/http.cpp cd7f692
> src/slave/slave.hpp 605ee4a
> src/slave/slave.cpp f42ab60
> src/tests/shutdown_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22832/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/#review47156
-----------------------------------------------------------
src/Makefile.am
<https://reviews.apache.org/r/22832/#comment82743>
nit: watch the tabs/spaces here :)
src/master/http.cpp
<https://reviews.apache.org/r/22832/#comment82745>
would you mind changing the indent here to match the lines above?
src/master/http.cpp
<https://reviews.apache.org/r/22832/#comment82746>
how long do you expect this call to take? ie, will the client timeout waiting for a response? should this dispatch the request to the master instead and return Accepted()?
src/slave/slave.hpp
<https://reviews.apache.org/r/22832/#comment82747>
is this change just for symmetry with Master::Http?
src/tests/shutdown_tests.cpp
<https://reviews.apache.org/r/22832/#comment82748>
our test methods usually have capital initials:
TEST_F(ShutdownTest, ShutdownEndpoint)
src/tests/shutdown_tests.cpp
<https://reviews.apache.org/r/22832/#comment82751>
i think each of these test cases should be a different test.
src/tests/shutdown_tests.cpp
<https://reviews.apache.org/r/22832/#comment82749>
you can also use OK().status (which is more descriptive)
src/tests/shutdown_tests.cpp
<https://reviews.apache.org/r/22832/#comment82750>
also test with authorization header and bad credentials?
- Dominic Hamon
On July 1, 2014, 3:46 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
>
> (Updated July 1, 2014, 3:46 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
>
>
> Bugs: MESOS-1390
> https://issues.apache.org/jira/browse/MESOS-1390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
>
>
> Diffs
> -----
>
> src/Makefile.am 12d84bf
> src/master/http.cpp 5d86976
> src/master/master.hpp 5fef354
> src/master/master.cpp 474014b
> src/slave/http.cpp cd7f692
> src/slave/slave.hpp 605ee4a
> src/slave/slave.cpp f42ab60
> src/tests/shutdown_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22832/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>