> On Feb. 8, 2018, 8:22 p.m., Vinod Kone wrote: > > src/master/master.cpp > > Lines 2186 (patched) > > <https://reviews.apache.org/r/65571/diff/1/?file=1954571#file1954571line2186> > > > > s/Leader detector indicated no master elected/No master was elected/ > > > > More importantly, this changes the semantics a bit. Previously if this > > master was the current leader it committed suicide even in this case. But > > we don't anymore. Is that what we want? > > > > > > Also, where in the interface does it say that None() is retryable. It > > says retryable errors are handled internally by the detector?
Ok, I guess I misinterpreted the documentation on `MasterDetector::detect()`: ``` * Returns MasterInfo after an election has occurred and the elected * master is different than that specified (if any), or NONE if an * election occurs and no master is elected (e.g., all masters are * lost). A failed future is returned if the detector is unable to * detect the leading master due to a non-retryable error. ``` Since electing no master sounds like an error, and the future is not failed, I assumed that this case was implicitly classifid as retryable error. Anyways, for the semantics I assume that not aborting is at least what the original author intended, otherwise there would be no point to differentiate between `Error` and `None` in the API. Additionally, the code that causes the master to crash in this situation was introduced relatively recently (11 July 2017, a8c7ae44c8), before that the `detected()`-handler would have just set `leader` to `None` and quietly continued. So I would argue that this fix is actually restoring the previously existing behaviour, not changing it. - Benno ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65571/#review197103 ----------------------------------------------------------- On Feb. 8, 2018, 4:50 p.m., Benno Evers wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65571/ > ----------------------------------------------------------- > > (Updated Feb. 8, 2018, 4:50 p.m.) > > > Review request for mesos, Andrei Budnik and Vinod Kone. > > > Bugs: MESOS-8550 > https://issues.apache.org/jira/browse/MESOS-8550 > > > Repository: mesos > > > Description > ------- > > The function `MasterDetector::detect()` returns a value of type > `Future<Option<MasterInfo>>`, which, according to its documentation, > can be `None` if an election occured and no master is elected. > > However, the code in `Master::detected()` was only handling the > cases of a failed future or a valid `MasterInfo` object. > > > Diffs > ----- > > src/master/master.cpp cc2685a6bc14103c639ce776cf1c912361e93381 > src/tests/cluster.hpp ad2b80e658d2f8afcefe9969d62cd33f0c475ce9 > src/tests/master_tests.cpp 7112bb4efcc312dc4f68bdc44fb685c7624002b1 > > > Diff: https://reviews.apache.org/r/65571/diff/1/ > > > Testing > ------- > > `./mesos-tests` > > > Thanks, > > Benno Evers > >
