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 <[email protected]>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/> >
