> 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
> 
>

Reply via email to