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 2014/03/26 01:22:01 UTC
Review Request 19657: Added an endpoint for retrieving the registry.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19657/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Vinod Kone.
Bugs: MESOS-764
https://issues.apache.org/jira/browse/MESOS-764
Repository: mesos-git
Description
-------
Added an endpoint for retrieving the registry contents.
Diffs
-----
src/master/registrar.cpp ba39ad4a92aff97cb74c00f3a865ce670f22c29a
Diff: https://reviews.apache.org/r/19657/diff/
Testing
-------
make check and hit the endpoint before a slave registered, after a slave registered, after a slave was removed.
Thanks,
Ben Mahler
Re: Review Request 19657: Added an endpoint for retrieving the registry.
Posted by Ben Mahler <be...@gmail.com>.
> On March 26, 2014, 12:38 a.m., Vinod Kone wrote:
> > src/master/registrar.cpp, line 61
> > <https://reviews.apache.org/r/19657/diff/1/?file=536714#file536714line61>
> >
> > Why is this not at the same place as Request and Response below?
This is because there is a mesos::Request and mesos::Response, I'll add a comment!
> On March 26, 2014, 12:38 a.m., Vinod Kone wrote:
> > src/master/registrar.cpp, line 98
> > <https://reviews.apache.org/r/19657/diff/1/?file=536714#file536714line98>
> >
> > const?
> >
> > Also, why make it a method instead of just string like we did in master::http and slave::http?
> >
> > const static string REGISTRY_HELP;
> >
> > const string RegistrarProcess::REGISTRY_HELP = HELP();
> >
re: const
You cannot have a const static function, 'const' is reserved for member functions.
re: method vs static variable
We should avoid static non-POD variables because of the previous discussion around this:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Global_Variables
We had a crash during process exit due to this.
Dominic brought this up and fixed several locations, but not master/slave http yet.
> On March 26, 2014, 12:38 a.m., Vinod Kone wrote:
> > src/master/registrar.cpp, line 162
> > <https://reviews.apache.org/r/19657/diff/1/?file=536714#file536714line162>
> >
> > Can you show an example in the description like we did for monitor?
Sure! I wanted to originally but it was pretty big. Would be nice if stringify for JSON was for humans and we added a way to "serialize" JSON for computers (no whitespace). If we had this we could use mock protobufs to avoid needing to hand-write the JSON in HELP.
> On March 26, 2014, 12:38 a.m., Vinod Kone wrote:
> > src/master/registrar.cpp, lines 47-59
> > <https://reviews.apache.org/r/19657/diff/1/?file=536714#file536714line47>
> >
> > These are not alphabetical.
Hm.. looks like we're inconsistent in this respect (master/http.cpp vs. status_update_manager.cpp). I was copying master/http.cpp but I'll update this.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19657/#review38562
-----------------------------------------------------------
On March 26, 2014, 12:22 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19657/
> -----------------------------------------------------------
>
> (Updated March 26, 2014, 12:22 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-764
> https://issues.apache.org/jira/browse/MESOS-764
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added an endpoint for retrieving the registry contents.
>
>
> Diffs
> -----
>
> src/master/registrar.cpp ba39ad4a92aff97cb74c00f3a865ce670f22c29a
>
> Diff: https://reviews.apache.org/r/19657/diff/
>
>
> Testing
> -------
>
> make check and hit the endpoint before a slave registered, after a slave registered, after a slave was removed.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 19657: Added an endpoint for retrieving the registry.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19657/#review38562
-----------------------------------------------------------
src/master/registrar.cpp
<https://reviews.apache.org/r/19657/#comment70826>
These are not alphabetical.
src/master/registrar.cpp
<https://reviews.apache.org/r/19657/#comment70825>
Why is this not at the same place as Request and Response below?
src/master/registrar.cpp
<https://reviews.apache.org/r/19657/#comment70823>
const?
Also, why make it a method instead of just string like we did in master::http and slave::http?
const static string REGISTRY_HELP;
const string RegistrarProcess::REGISTRY_HELP = HELP();
src/master/registrar.cpp
<https://reviews.apache.org/r/19657/#comment70822>
Can you show an example in the description like we did for monitor?
- Vinod Kone
On March 26, 2014, 12:22 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19657/
> -----------------------------------------------------------
>
> (Updated March 26, 2014, 12:22 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-764
> https://issues.apache.org/jira/browse/MESOS-764
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added an endpoint for retrieving the registry contents.
>
>
> Diffs
> -----
>
> src/master/registrar.cpp ba39ad4a92aff97cb74c00f3a865ce670f22c29a
>
> Diff: https://reviews.apache.org/r/19657/diff/
>
>
> Testing
> -------
>
> make check and hit the endpoint before a slave registered, after a slave registered, after a slave was removed.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 19657: Added an endpoint for retrieving the registry.
Posted by Ben Mahler <be...@gmail.com>.
> On March 26, 2014, 7:22 p.m., Dominic Hamon wrote:
> > src/master/registrar.cpp, line 92
> > <https://reviews.apache.org/r/19657/diff/2/?file=537196#file537196line92>
> >
> > s/REGISTRY_HELP()/help()/ ?
Changed to registryHelp() (to make it clear that it corresponds to the 'registry' endpoint).
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19657/#review38651
-----------------------------------------------------------
On March 26, 2014, 1:34 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19657/
> -----------------------------------------------------------
>
> (Updated March 26, 2014, 1:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-764
> https://issues.apache.org/jira/browse/MESOS-764
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added an endpoint for retrieving the registry contents.
>
>
> Diffs
> -----
>
> src/master/registrar.cpp ba39ad4a92aff97cb74c00f3a865ce670f22c29a
>
> Diff: https://reviews.apache.org/r/19657/diff/
>
>
> Testing
> -------
>
> make check and hit the endpoint before a slave registered, after a slave registered, after a slave was removed.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 19657: Added an endpoint for retrieving the registry.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19657/#review38651
-----------------------------------------------------------
src/master/registrar.cpp
<https://reviews.apache.org/r/19657/#comment70912>
s/REGISTRY_HELP()/help()/ ?
- Dominic Hamon
On March 25, 2014, 6:34 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19657/
> -----------------------------------------------------------
>
> (Updated March 25, 2014, 6:34 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-764
> https://issues.apache.org/jira/browse/MESOS-764
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added an endpoint for retrieving the registry contents.
>
>
> Diffs
> -----
>
> src/master/registrar.cpp ba39ad4a92aff97cb74c00f3a865ce670f22c29a
>
> Diff: https://reviews.apache.org/r/19657/diff/
>
>
> Testing
> -------
>
> make check and hit the endpoint before a slave registered, after a slave registered, after a slave was removed.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 19657: Added an endpoint for retrieving the registry.
Posted by Ben Mahler <be...@gmail.com>.
> On March 26, 2014, 7:20 p.m., Vinod Kone wrote:
> > src/master/registrar.cpp, lines 53-63
> > <https://reviews.apache.org/r/19657/diff/1-2/?file=536714#file536714line53>
> >
> > Not sure what this order is :)
> >
> > >>> sorted(["Failure", "Future", "DESCRIPTION", "HELP", "Owned", "Process", "Promise", "TLDR", "USAGE"])
> > ['DESCRIPTION', 'Failure', 'Future', 'HELP', 'Owned', 'Process', 'Promise', 'TLDR', 'USAGE']
> >
Whoops, moved DESCRIPTION to the top.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19657/#review38647
-----------------------------------------------------------
On March 26, 2014, 1:34 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19657/
> -----------------------------------------------------------
>
> (Updated March 26, 2014, 1:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-764
> https://issues.apache.org/jira/browse/MESOS-764
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added an endpoint for retrieving the registry contents.
>
>
> Diffs
> -----
>
> src/master/registrar.cpp ba39ad4a92aff97cb74c00f3a865ce670f22c29a
>
> Diff: https://reviews.apache.org/r/19657/diff/
>
>
> Testing
> -------
>
> make check and hit the endpoint before a slave registered, after a slave registered, after a slave was removed.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 19657: Added an endpoint for retrieving the registry.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19657/#review38647
-----------------------------------------------------------
Ship it!
src/master/registrar.cpp
<https://reviews.apache.org/r/19657/#comment70908>
should come after process::terminate.
src/master/registrar.cpp
<https://reviews.apache.org/r/19657/#comment70911>
Not sure what this order is :)
>>> sorted(["Failure", "Future", "DESCRIPTION", "HELP", "Owned", "Process", "Promise", "TLDR", "USAGE"])
['DESCRIPTION', 'Failure', 'Future', 'HELP', 'Owned', 'Process', 'Promise', 'TLDR', 'USAGE']
- Vinod Kone
On March 26, 2014, 1:34 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19657/
> -----------------------------------------------------------
>
> (Updated March 26, 2014, 1:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-764
> https://issues.apache.org/jira/browse/MESOS-764
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added an endpoint for retrieving the registry contents.
>
>
> Diffs
> -----
>
> src/master/registrar.cpp ba39ad4a92aff97cb74c00f3a865ce670f22c29a
>
> Diff: https://reviews.apache.org/r/19657/diff/
>
>
> Testing
> -------
>
> make check and hit the endpoint before a slave registered, after a slave registered, after a slave was removed.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 19657: Added an endpoint for retrieving the registry.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19657/
-----------------------------------------------------------
(Updated March 26, 2014, 10:29 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
NNFR.
Bugs: MESOS-764
https://issues.apache.org/jira/browse/MESOS-764
Repository: mesos-git
Description
-------
Added an endpoint for retrieving the registry contents.
Diffs (updated)
-----
src/master/registrar.cpp ba39ad4a92aff97cb74c00f3a865ce670f22c29a
Diff: https://reviews.apache.org/r/19657/diff/
Testing
-------
make check and hit the endpoint before a slave registered, after a slave registered, after a slave was removed.
Thanks,
Ben Mahler
Re: Review Request 19657: Added an endpoint for retrieving the registry.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19657/#review38575
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19657]
All tests passed.
- Mesos ReviewBot
On March 26, 2014, 1:34 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19657/
> -----------------------------------------------------------
>
> (Updated March 26, 2014, 1:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-764
> https://issues.apache.org/jira/browse/MESOS-764
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added an endpoint for retrieving the registry contents.
>
>
> Diffs
> -----
>
> src/master/registrar.cpp ba39ad4a92aff97cb74c00f3a865ce670f22c29a
>
> Diff: https://reviews.apache.org/r/19657/diff/
>
>
> Testing
> -------
>
> make check and hit the endpoint before a slave registered, after a slave registered, after a slave was removed.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 19657: Added an endpoint for retrieving the registry.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19657/#review38659
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On March 26, 2014, 1:34 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19657/
> -----------------------------------------------------------
>
> (Updated March 26, 2014, 1:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-764
> https://issues.apache.org/jira/browse/MESOS-764
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added an endpoint for retrieving the registry contents.
>
>
> Diffs
> -----
>
> src/master/registrar.cpp ba39ad4a92aff97cb74c00f3a865ce670f22c29a
>
> Diff: https://reviews.apache.org/r/19657/diff/
>
>
> Testing
> -------
>
> make check and hit the endpoint before a slave registered, after a slave registered, after a slave was removed.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 19657: Added an endpoint for retrieving the registry.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19657/
-----------------------------------------------------------
(Updated March 26, 2014, 1:34 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Alphabetized includes and added example JSON.
Bugs: MESOS-764
https://issues.apache.org/jira/browse/MESOS-764
Repository: mesos-git
Description
-------
Added an endpoint for retrieving the registry contents.
Diffs (updated)
-----
src/master/registrar.cpp ba39ad4a92aff97cb74c00f3a865ce670f22c29a
Diff: https://reviews.apache.org/r/19657/diff/
Testing
-------
make check and hit the endpoint before a slave registered, after a slave registered, after a slave was removed.
Thanks,
Ben Mahler