You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Charlie Carson <cc...@twitter.com> on 2013/12/13 00:39:10 UTC
Review Request 16226: add end point to master for observing health
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16226/
-----------------------------------------------------------
Review request for mesos and Jeff Currier.
Bugs: https://issues.apache.org/jira/browse/MESOS-880
https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880
Repository: mesos-git
Description
-------
add an observe endpoint to master. add a stubbed RepairCoordinator.
also reordered the existing routes so they are alphabetical
add a post method to the libprocess library
added test / modified master_tests to allow for a MockRepairCoordinator
^refactored two methods which were nearly identical into a common path so that I didn't make it triplicate
Diffs
-----
3rdparty/libprocess/include/process/http.hpp 5bdd520c9e0bcc0a2d3a4554cc4ced99dcf78b51
3rdparty/libprocess/src/process.cpp 2d193b13cde92a061b02f903a5d6471ff90cb12a
src/Makefile.am 5f211a244f6f64aef4ababebdb542b40d6086b0b
src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
src/master/http.cpp d7cd89f0a3446f4c2e65ecd259544149bf92faf8
src/master/main.cpp b1e45766281c5ea0b8a3cee89e390ea60a97c5e4
src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b
src/master/master.cpp cb8e613b31f39329d61a490f5d3f74fd44ffb08c
src/master/repair_coordinator.hpp PRE-CREATION
src/master/repair_coordinator.cpp PRE-CREATION
src/tests/cluster.hpp 065976c19170e995bd3766bcc7a9b0a244776108
src/tests/master_tests.cpp d34450bf84704b224f4e2dbc61ce100b33d14027
src/tests/mesos.hpp b1239a653e515115a71e436c79b2d11db4a209f9
src/tests/mesos.cpp 5359394f45475803e05d281710139e8cbe7c7364
Diff: https://reviews.apache.org/r/16226/diff/
Testing
-------
new tests to verify the end point is passing data to the mock repair coordinator coorectly.
make check to verify that existing tests are passing
Thanks,
Charlie Carson
Re: Review Request 16226: add end point to master for observing health
Posted by Jeff Currier <jc...@twitter.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16226/#review30299
-----------------------------------------------------------
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/16226/#comment58127>
I know we're only supporting GET & POST now but perhaps it would be better to make the bool isGet a enum with the different verbs we support. We could just keep it at GET & POST for now but if were to ever say at PUT, just as an example, the churn we would take here would be reduced quite a bit.
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/16226/#comment58007>
NIT: Drop the space.
src/master/repair_coordinator.hpp
<https://reviews.apache.org/r/16226/#comment58135>
Perhaps add comment to explain what this does?
src/tests/master_tests.cpp
<https://reviews.apache.org/r/16226/#comment58137>
It may be worthwhile to add tests where each observe POST sends a request missing at least one of the keys.
I know the check today is a key size check but it we alter that later then having a test to verify this could be useful.
- Jeff Currier
On Dec. 12, 2013, 11:45 p.m., Charlie Carson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16226/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2013, 11:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jeff Currier, and Vinod Kone.
>
>
> Bugs: https://issues.apache.org/jira/browse/MESOS-880
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> add an observe endpoint to master. add a stubbed RepairCoordinator.
>
>
> also reordered the existing routes so they are alphabetical
>
> add a post method to the libprocess library
>
> added test / modified master_tests to allow for a MockRepairCoordinator
> ^refactored two methods which were nearly identical into a common path so that I didn't make it triplicate
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 5bdd520c9e0bcc0a2d3a4554cc4ced99dcf78b51
> 3rdparty/libprocess/src/process.cpp 2d193b13cde92a061b02f903a5d6471ff90cb12a
> src/Makefile.am 5f211a244f6f64aef4ababebdb542b40d6086b0b
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/master/http.cpp d7cd89f0a3446f4c2e65ecd259544149bf92faf8
> src/master/main.cpp b1e45766281c5ea0b8a3cee89e390ea60a97c5e4
> src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b
> src/master/master.cpp cb8e613b31f39329d61a490f5d3f74fd44ffb08c
> src/master/repair_coordinator.hpp PRE-CREATION
> src/master/repair_coordinator.cpp PRE-CREATION
> src/tests/cluster.hpp 065976c19170e995bd3766bcc7a9b0a244776108
> src/tests/master_tests.cpp d34450bf84704b224f4e2dbc61ce100b33d14027
> src/tests/mesos.hpp b1239a653e515115a71e436c79b2d11db4a209f9
> src/tests/mesos.cpp 5359394f45475803e05d281710139e8cbe7c7364
>
> Diff: https://reviews.apache.org/r/16226/diff/
>
>
> Testing
> -------
>
> new tests to verify the end point is passing data to the mock repair coordinator coorectly.
>
> make check to verify that existing tests are passing
>
>
> Thanks,
>
> Charlie Carson
>
>
Re: Review Request 16226: add end point to master for observing health
Posted by Charlie Carson <cc...@twitter.com>.
> On Dec. 17, 2013, 11:42 p.m., Ben Mahler wrote:
> > Hey Charlie! I haven't taken a look at the full diff just yet, but I wanted to make a few comments here in terms of how we do reviews in Mesos.
> >
> > When a review is eventually pulled down and committed, the review summary and description are used in the log message, so we try to keep these sanitary and meaningful so that we can directly use these fields as part of the commit message / description. This also has the added benefit of helping provide context to reviewers.
> >
> > The second thing is that because libprocess and stout are open source projects as well, changes made to these must be done through separate reviews so that we can easily contribute changes back to these projects. When changes are mixed between mesos / stout / libprocess, this process becomes extremely tedious, ideally we would enforce this in the review tooling.
> >
> > The third thing is that again since we pull down reviews directly as commits, we try to separate logical changes into multiple reviews to keep a sane commit history for the project (in this case it would be nice to pull out the http changes to libprocess, although from my previous comment we would have to do this anyway :)). We have a great tool that assists with this process: ./support/post-reviews.py. This tool takes each commit in your local branch and creates / updates a review for each commit. This makes it easy to split changes up into multiple commits and it also forces one to keep a meaningful commit history. Hope this helps and I look forward to seeing your updates!
Thanks for the heads up Ben - (BTW, I was on vacation so sorry for the delay). I'll pull this monolithic review and submit smaller ones scoped to the 3 projects. I'll also scan some of the existing mesos reviews to get a better idea of the correct level of explanation / context for the review comments.
thanks again,
cc
- Charlie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16226/#review30585
-----------------------------------------------------------
On Dec. 12, 2013, 11:45 p.m., Charlie Carson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16226/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2013, 11:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jeff Currier, and Vinod Kone.
>
>
> Bugs: https://issues.apache.org/jira/browse/MESOS-880
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> add an observe endpoint to master. add a stubbed RepairCoordinator.
>
>
> also reordered the existing routes so they are alphabetical
>
> add a post method to the libprocess library
>
> added test / modified master_tests to allow for a MockRepairCoordinator
> ^refactored two methods which were nearly identical into a common path so that I didn't make it triplicate
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 5bdd520c9e0bcc0a2d3a4554cc4ced99dcf78b51
> 3rdparty/libprocess/src/process.cpp 2d193b13cde92a061b02f903a5d6471ff90cb12a
> src/Makefile.am 5f211a244f6f64aef4ababebdb542b40d6086b0b
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/master/http.cpp d7cd89f0a3446f4c2e65ecd259544149bf92faf8
> src/master/main.cpp b1e45766281c5ea0b8a3cee89e390ea60a97c5e4
> src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b
> src/master/master.cpp cb8e613b31f39329d61a490f5d3f74fd44ffb08c
> src/master/repair_coordinator.hpp PRE-CREATION
> src/master/repair_coordinator.cpp PRE-CREATION
> src/tests/cluster.hpp 065976c19170e995bd3766bcc7a9b0a244776108
> src/tests/master_tests.cpp d34450bf84704b224f4e2dbc61ce100b33d14027
> src/tests/mesos.hpp b1239a653e515115a71e436c79b2d11db4a209f9
> src/tests/mesos.cpp 5359394f45475803e05d281710139e8cbe7c7364
>
> Diff: https://reviews.apache.org/r/16226/diff/
>
>
> Testing
> -------
>
> new tests to verify the end point is passing data to the mock repair coordinator coorectly.
>
> make check to verify that existing tests are passing
>
>
> Thanks,
>
> Charlie Carson
>
>
Re: Review Request 16226: add end point to master for observing health
Posted by Charlie Carson <cc...@twitter.com>.
Hi Ben, I just sent a review that is chopped down to just be changes to
libprocess. Basically, it's just adding a post function by refactoring the
existing get function. I also gave a more verbose description in both the
JIRA / CR / git commit. Finally, I was able to use support/post-reviews.py
this time (it was giving me errors before), so hopefully that covers it.
thanks and looking forward to round 2 of feedback,
cc
On Tue, Dec 17, 2013 at 3:42 PM, Ben Mahler <be...@gmail.com>wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16226/
>
> Hey Charlie! I haven't taken a look at the full diff just yet, but I wanted to make a few comments here in terms of how we do reviews in Mesos.
>
> When a review is eventually pulled down and committed, the review summary and description are used in the log message, so we try to keep these sanitary and meaningful so that we can directly use these fields as part of the commit message / description. This also has the added benefit of helping provide context to reviewers.
>
> The second thing is that because libprocess and stout are open source projects as well, changes made to these must be done through separate reviews so that we can easily contribute changes back to these projects. When changes are mixed between mesos / stout / libprocess, this process becomes extremely tedious, ideally we would enforce this in the review tooling.
>
> The third thing is that again since we pull down reviews directly as commits, we try to separate logical changes into multiple reviews to keep a sane commit history for the project (in this case it would be nice to pull out the http changes to libprocess, although from my previous comment we would have to do this anyway :)). We have a great tool that assists with this process: ./support/post-reviews.py. This tool takes each commit in your local branch and creates / updates a review for each commit. This makes it easy to split changes up into multiple commits and it also forces one to keep a meaningful commit history. Hope this helps and I look forward to seeing your updates!
>
>
> - Ben Mahler
>
> On December 12th, 2013, 11:45 p.m. UTC, Charlie Carson wrote:
> Review request for mesos, Benjamin Hindman, Jeff Currier, and Vinod
> Kone.
> By Charlie Carson.
>
> *Updated Dec. 12, 2013, 11:45 p.m.*
> *Bugs: * https://issues.apache.org/jira/browse/MESOS-880<https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880>
> *Repository: * mesos-git
> Description
>
> add an observe endpoint to master. add a stubbed RepairCoordinator.
>
>
> also reordered the existing routes so they are alphabetical
>
> add a post method to the libprocess library
>
> added test / modified master_tests to allow for a MockRepairCoordinator
> ^refactored two methods which were nearly identical into a common path so that I didn't make it triplicate
>
>
> Testing
>
> new tests to verify the end point is passing data to the mock repair coordinator coorectly.
>
> make check to verify that existing tests are passing
>
> Diffs
>
> - 3rdparty/libprocess/include/process/http.hpp
> (5bdd520c9e0bcc0a2d3a4554cc4ced99dcf78b51)
> - 3rdparty/libprocess/src/process.cpp
> (2d193b13cde92a061b02f903a5d6471ff90cb12a)
> - src/Makefile.am (5f211a244f6f64aef4ababebdb542b40d6086b0b)
> - src/local/local.cpp (83a7f913afb1441e9137c7fcec8fd81815714905)
> - src/master/http.cpp (d7cd89f0a3446f4c2e65ecd259544149bf92faf8)
> - src/master/main.cpp (b1e45766281c5ea0b8a3cee89e390ea60a97c5e4)
> - src/master/master.hpp (6c168a2cdbd8343516cb47adceaff70c3d46690b)
> - src/master/master.cpp (cb8e613b31f39329d61a490f5d3f74fd44ffb08c)
> - src/master/repair_coordinator.hpp (PRE-CREATION)
> - src/master/repair_coordinator.cpp (PRE-CREATION)
> - src/tests/cluster.hpp (065976c19170e995bd3766bcc7a9b0a244776108)
> - src/tests/master_tests.cpp (d34450bf84704b224f4e2dbc61ce100b33d14027)
> - src/tests/mesos.hpp (b1239a653e515115a71e436c79b2d11db4a209f9)
> - src/tests/mesos.cpp (5359394f45475803e05d281710139e8cbe7c7364)
>
> View Diff <https://reviews.apache.org/r/16226/diff/>
>
Re: Review Request 16226: add end point to master for observing health
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16226/#review30585
-----------------------------------------------------------
Hey Charlie! I haven't taken a look at the full diff just yet, but I wanted to make a few comments here in terms of how we do reviews in Mesos.
When a review is eventually pulled down and committed, the review summary and description are used in the log message, so we try to keep these sanitary and meaningful so that we can directly use these fields as part of the commit message / description. This also has the added benefit of helping provide context to reviewers.
The second thing is that because libprocess and stout are open source projects as well, changes made to these must be done through separate reviews so that we can easily contribute changes back to these projects. When changes are mixed between mesos / stout / libprocess, this process becomes extremely tedious, ideally we would enforce this in the review tooling.
The third thing is that again since we pull down reviews directly as commits, we try to separate logical changes into multiple reviews to keep a sane commit history for the project (in this case it would be nice to pull out the http changes to libprocess, although from my previous comment we would have to do this anyway :)). We have a great tool that assists with this process: ./support/post-reviews.py. This tool takes each commit in your local branch and creates / updates a review for each commit. This makes it easy to split changes up into multiple commits and it also forces one to keep a meaningful commit history. Hope this helps and I look forward to seeing your updates!
- Ben Mahler
On Dec. 12, 2013, 11:45 p.m., Charlie Carson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16226/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2013, 11:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jeff Currier, and Vinod Kone.
>
>
> Bugs: https://issues.apache.org/jira/browse/MESOS-880
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> add an observe endpoint to master. add a stubbed RepairCoordinator.
>
>
> also reordered the existing routes so they are alphabetical
>
> add a post method to the libprocess library
>
> added test / modified master_tests to allow for a MockRepairCoordinator
> ^refactored two methods which were nearly identical into a common path so that I didn't make it triplicate
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 5bdd520c9e0bcc0a2d3a4554cc4ced99dcf78b51
> 3rdparty/libprocess/src/process.cpp 2d193b13cde92a061b02f903a5d6471ff90cb12a
> src/Makefile.am 5f211a244f6f64aef4ababebdb542b40d6086b0b
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/master/http.cpp d7cd89f0a3446f4c2e65ecd259544149bf92faf8
> src/master/main.cpp b1e45766281c5ea0b8a3cee89e390ea60a97c5e4
> src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b
> src/master/master.cpp cb8e613b31f39329d61a490f5d3f74fd44ffb08c
> src/master/repair_coordinator.hpp PRE-CREATION
> src/master/repair_coordinator.cpp PRE-CREATION
> src/tests/cluster.hpp 065976c19170e995bd3766bcc7a9b0a244776108
> src/tests/master_tests.cpp d34450bf84704b224f4e2dbc61ce100b33d14027
> src/tests/mesos.hpp b1239a653e515115a71e436c79b2d11db4a209f9
> src/tests/mesos.cpp 5359394f45475803e05d281710139e8cbe7c7364
>
> Diff: https://reviews.apache.org/r/16226/diff/
>
>
> Testing
> -------
>
> new tests to verify the end point is passing data to the mock repair coordinator coorectly.
>
> make check to verify that existing tests are passing
>
>
> Thanks,
>
> Charlie Carson
>
>
Re: Review Request 16226: add end point to master for observing health
Posted by Jeff Currier <jc...@twitter.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16226/#review31683
-----------------------------------------------------------
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/16226/#comment60391>
NIT: Perhaps it would be better to use a enum where each element of the enum represented the HTTP verb we were going to use rather than using isGet to do this.
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/16226/#comment60390>
NIT: Drop the trailing space.
- Jeff Currier
On Dec. 12, 2013, 11:45 p.m., Charlie Carson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16226/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2013, 11:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jeff Currier, and Vinod Kone.
>
>
> Bugs: https://issues.apache.org/jira/browse/MESOS-880
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> add an observe endpoint to master. add a stubbed RepairCoordinator.
>
>
> also reordered the existing routes so they are alphabetical
>
> add a post method to the libprocess library
>
> added test / modified master_tests to allow for a MockRepairCoordinator
> ^refactored two methods which were nearly identical into a common path so that I didn't make it triplicate
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 5bdd520c9e0bcc0a2d3a4554cc4ced99dcf78b51
> 3rdparty/libprocess/src/process.cpp 2d193b13cde92a061b02f903a5d6471ff90cb12a
> src/Makefile.am 5f211a244f6f64aef4ababebdb542b40d6086b0b
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/master/http.cpp d7cd89f0a3446f4c2e65ecd259544149bf92faf8
> src/master/main.cpp b1e45766281c5ea0b8a3cee89e390ea60a97c5e4
> src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b
> src/master/master.cpp cb8e613b31f39329d61a490f5d3f74fd44ffb08c
> src/master/repair_coordinator.hpp PRE-CREATION
> src/master/repair_coordinator.cpp PRE-CREATION
> src/tests/cluster.hpp 065976c19170e995bd3766bcc7a9b0a244776108
> src/tests/master_tests.cpp d34450bf84704b224f4e2dbc61ce100b33d14027
> src/tests/mesos.hpp b1239a653e515115a71e436c79b2d11db4a209f9
> src/tests/mesos.cpp 5359394f45475803e05d281710139e8cbe7c7364
>
> Diff: https://reviews.apache.org/r/16226/diff/
>
>
> Testing
> -------
>
> new tests to verify the end point is passing data to the mock repair coordinator coorectly.
>
> make check to verify that existing tests are passing
>
>
> Thanks,
>
> Charlie Carson
>
>
Re: Review Request 16226: add end point to master for observing health
Posted by Charlie Carson <cc...@twitter.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16226/
-----------------------------------------------------------
(Updated Dec. 12, 2013, 11:45 p.m.)
Review request for mesos, Benjamin Hindman, Jeff Currier, and Vinod Kone.
Changes
-------
added Ben & Vinod to review the changes I made to cluster.hpp & porcess.cpp
Bugs: https://issues.apache.org/jira/browse/MESOS-880
https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880
Repository: mesos-git
Description
-------
add an observe endpoint to master. add a stubbed RepairCoordinator.
also reordered the existing routes so they are alphabetical
add a post method to the libprocess library
added test / modified master_tests to allow for a MockRepairCoordinator
^refactored two methods which were nearly identical into a common path so that I didn't make it triplicate
Diffs
-----
3rdparty/libprocess/include/process/http.hpp 5bdd520c9e0bcc0a2d3a4554cc4ced99dcf78b51
3rdparty/libprocess/src/process.cpp 2d193b13cde92a061b02f903a5d6471ff90cb12a
src/Makefile.am 5f211a244f6f64aef4ababebdb542b40d6086b0b
src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
src/master/http.cpp d7cd89f0a3446f4c2e65ecd259544149bf92faf8
src/master/main.cpp b1e45766281c5ea0b8a3cee89e390ea60a97c5e4
src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b
src/master/master.cpp cb8e613b31f39329d61a490f5d3f74fd44ffb08c
src/master/repair_coordinator.hpp PRE-CREATION
src/master/repair_coordinator.cpp PRE-CREATION
src/tests/cluster.hpp 065976c19170e995bd3766bcc7a9b0a244776108
src/tests/master_tests.cpp d34450bf84704b224f4e2dbc61ce100b33d14027
src/tests/mesos.hpp b1239a653e515115a71e436c79b2d11db4a209f9
src/tests/mesos.cpp 5359394f45475803e05d281710139e8cbe7c7364
Diff: https://reviews.apache.org/r/16226/diff/
Testing
-------
new tests to verify the end point is passing data to the mock repair coordinator coorectly.
make check to verify that existing tests are passing
Thanks,
Charlie Carson