I don't think this comment still makes sense since we removed the extra abdication parameters.

This is a good start for a class doc :thumbup: However I would be also interested in the complete chain of events e.g.:

• -> where do ZK/CuratorService events come in?
• -> what happens after we're elected? What events are sent through the system?
• -> what parts of the chain happens synchronously vs. asynchronously and can happen parallel to other events?

I know that "the code is self-explanatory" and "why dont you just read the code" but having full chain of events, concurrency model and guaranties explained goes a long way. Sry if it feels like I'm the pain in the neck :)

So if we were offered leadership twice in a short period of time, neither will succeed and marathon will exit? Can't this lead to a scenario where marathon gets too many leadership offers but is not able to act on them successfully?

I think it still makes sense because it is more about leadership abdication, not the method's parameters. It is more about the fact that driver.foreach is used, because it can be set to None in stopDriver() before this line gets to execute. What do you think?

where do ZK/CuratorService events come in? — I don't get what you mean here.
what happens after we're elected? — it is already documented in both ElectionService and ElectionCandidate.
what events are sent through the system? — done.
what parts of the chain happens synchronously vs. asynchronously and can happen parallel to other events? — All methods are synchronous which should be expected by default because no future is returned and no callback is accepted in any method.

If a leadership is offered more than once, then it is a (I would say, severe) bug in the code. As of now, only MarathonScheculerService invokes this method, and surely it is supposed to do so only once during start-up. Therefore, stopping is a proper action here, I deem.

I commented in a ticket that reporting leader metrics for non-ha mode is misleading. But, it matches old behavior and I think this is great for now. Let's move forward.

Hm, it seems this class shares a lot of logic with CuratorElectionService. How are they different? Is there a way to share some logic?

It seems we are not testing CuratorElectionService. Is this impression correct?

Sweet!

I see a vicious circle right there. I feel like I can write a book about trade-offs when implementing leader election which relies on an external service like ZooKeeper. :)

This is exactly what I was asked for from the very beginning: get rid of ElectionServiceBase. My first step was to simplify things, and reduce the number of states and their transitions in ElectionServiceBase, which lead to to renaming of it to ElectionServiceFSM because it started to look like a real FSM. It wasn't enough, and it led to this further simplification at the expense of having this duplicated code. It seems we are headed towards Akka Streams. This code duplication won't be there anymore after another round of refactoring of this code. On the other hand, if one imagines a two-level inheritance tree with one parent and multiple children as a horizontal thing as a apposed to a vertical thing, where the parent make calls into children and vice versa, it can be seen very well as a message-passing with compile-time checks of it.

Long story short, it is what it is. The decision was made, it is too late to change it drastically at this point. Though I do see that

1. CuratorElectionService is harder to test now, because previously most of it was tested by testing ElectionServiceBase
2. There is code duplication.

we should keep this intermediate approach, move on, and have it refactored to make it leverage Akka Streams later, after 1.10 release.

It is correct. Please refer to my comment to your first one. Most of it was tested using ElectionServiceBase/FSM before, but since most of the code is not shared between CuratorElectionService and PseudoElectionService, it is gone. Now testing of it would require running ZK, which looks more like SI testing at least.

seems unrelated to leader election. why is this change here?

why is this linter rule needed?

why not a sealed trait here?

case object (if we use sealed trait above)?

not that it's related to this change (because this just looks like a rename) but what's the impact of using a separate execution context here, vs. the "global" one that has support for the injected Context stuff Jason wrote?

https://github.com/mesosphere/marathon/blob/master/src/main/scala/mesosphere/marathon/core/async/ExecutionContexts.scala

leadershipOffered and acquiringLeadership function more like single-use latches. it might make sense to either (a) document that, or else; (b) wrap with a helper type that only allows a single-use one-way transition:

package mesosphere.marathon;
package core.async;

final class Latch {
  private[this] val counter = new Semaphore(1)
  def acquire: Option[Done] = 
    // use of Option return type forces pattern matches to deal w/ all alternatives, likely highly desirable
    if(counter.tryAcquire()) Some(Done) else None 
}

I like the idea of limiting transitions via a Latch type because it enforces the design as intended

nit: extract constant

looks like the ACLs changed here. why is this part of this PR and not a separate changeset? seems significant enough?

nit: extract constants

is it possible that startMetrics and stopMetrics could be called concurrently? if so, the atomic boolean guard won't help you avoid a data race. if not, then i'd like to see that assumption documented someplace

ditto re: Latch suggestion

nit: extract constant