----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18675/#review36260 -----------------------------------------------------------
Ship it! src/master/registrar.cpp <https://reviews.apache.org/r/18675/#comment67221> To the best of my knowledge this is a new style in the code base. The one issue I potentially perceive here is that actors waiting on 'recovery' (or later below on 'recovered' will actually print out errors before this LOG(ERROR) gets printed because the Promise::fail callback will invoke the callbacks first. While it might seem like more of a PITA I wonder if we're really better off doing: string failure = "..."; LOG(ERROR) << failure; recovered.get()->fail(failure); src/master/registrar.cpp <https://reviews.apache.org/r/18675/#comment67222> Not that you need it, but this was lost in the refactor FYI. src/master/registrar.cpp <https://reviews.apache.org/r/18675/#comment67220> Did you mean s/Recovery/Recover/ ? src/master/registrar.cpp <https://reviews.apache.org/r/18675/#comment67218> Technically this is a 'failed to persist master info' case as well. src/master/registrar.cpp <https://reviews.apache.org/r/18675/#comment67219> This is the hardest code to reason about because it requires non-local reasoning, i.e., the variable gets updated via the 'update' call in '_recover' and thus it is 'Some' here which is why you can set 'recovered' to it. Can you enhance this comment to make this flow more understandable to a future reader? Thanks! src/master/registrar.cpp <https://reviews.apache.org/r/18675/#comment67214> Consider killing this line to be consistent with other continuations above (or add lines above). src/tests/registrar_tests.cpp <https://reviews.apache.org/r/18675/#comment67217> Here's a nudge to adding a test which does admit, readmit, or remove before recover and does an AWAIT_FAILED. ;) - Benjamin Hindman On March 4, 2014, 8:05 p.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18675/ > ----------------------------------------------------------- > > (Updated March 4, 2014, 8:05 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 > ------- > > As a safety measure, we would like to force a version change on the Registry > as a result of the recovery process. This helps prevent a "rogue" Master > (that still believes it is leading) from performing writes to the Registry. > > This change adds a 'Recover' operation which adds the latest MasterInfo to > the Registry. Unfortunately, since the Registrar is injected to the Master, I > had to add 'MasterInfo' as an argument to the 'recover' function, thus > altering the design somewhat. > > The current semantics are that there one must call 'recover' before calling > other operations, otherwise these other operations will fail. > > > Diffs > ----- > > src/master/registrar.hpp 20734afc69055197e9ab90d42253c56e4af4b97c > src/master/registrar.cpp 37337c07b24a96e71910b7c83085d159361a1188 > src/tests/registrar_tests.cpp 3bf42bd77a10470a2afc6fd8e1da30d6134e792c > > Diff: https://reviews.apache.org/r/18675/diff/ > > > Testing > ------- > > Added a small test. > > > Thanks, > > Ben Mahler > >
