My primary concern with the current 2.8.0 code is confusing configs. The reason we called the existing leader election and the corresponding config embedded was because we wanted to highlight it is embedded in the RM. The plan, at the time, was to add ZKFC-based leader election as well. We should likely leave that config (yarn.resourcemanager.ha.automatic-failover.embedded) alone unless we make a decision that we will not add ZKFC-type of leader election that runs in a different process.
the ultimate goal is to remove the old EmbeddedElectorService
We are in agreement here. I am comfortable with ripping out the current implementation of EmbeddedElectorService in 2.8.0 and replacing it with the curator-based implementation.
Any reason we are not replacing the implementation? Do we just want to be safe and have a workaround in case the curator-based elector turns out to broken? If that is the case, the config that determines the implementation should be removed in a subsequent release and accordingly be called out @Unstable. We should likely not list it in yarn-default.xml.
don't think it needs to be the case even for the old EmbeddedElectorService too, if you look at the implementation, there's no dependency between the EmbeddedElectorService and AdminService at all.
I see your point.
On the dependency front, EmbeddedElectorService does not depend on AdminService. However, if we were to implement a ZKFC-based elector, that would have to depend on the AdminService to affect any transitions at all. I believe Bikas has recommended we keep the same code path irrespective of whether leader election is embedded. I see merit to that argument.
If we do decide on moving the initialization, we should move it for both implementations and not just one. Implementation-based initialization points is confusing for any new person looking at the code. Even for those looking at it after a while (like me).