[
https://issues.apache.org/jira/browse/YARN-5709?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15726506#comment-15726506
]
Karthik Kambatla edited comment on YARN-5709 at 12/6/16 7:50 PM:
-----------------------------------------------------------------
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.
bq. 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.
bq. 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).
was (Author: kasha):
My primary concern with the current 2.8.0 code is confusing configs. The reason
we called it 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.
bq. 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.
bq. 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).
> Cleanup leader election related configuration mess
> --------------------------------------------------
>
> Key: YARN-5709
> URL: https://issues.apache.org/jira/browse/YARN-5709
> Project: Hadoop YARN
> Issue Type: Improvement
> Components: resourcemanager
> Affects Versions: 2.8.0
> Reporter: Karthik Kambatla
> Assignee: Daniel Templeton
> Priority: Blocker
>
> While reviewing YARN-5677 and YARN-5694, I noticed we could make the
> curator-based election code cleaner. It is nicer to get this fixed in 2.8
> before we ship it, but this can be done at a later time as well.
> # By EmbeddedElector, we meant it was running as part of the RM daemon. Since
> the Curator-based elector is also running embedded, I feel the code should be
> checking for {{!curatorBased}} instead of {{isEmbeddedElector}}
> # {{LeaderElectorService}} should probably be named
> {{CuratorBasedEmbeddedElectorService}} or some such.
> # The code that initializes the elector should be at the same place
> irrespective of whether it is curator-based or not.
> # We seem to be caching the CuratorFramework instance in RM. It makes more
> sense for it to be in RMContext. If others are okay with it, we might even be
> better of having {{RMContext#getCurator()}} method to lazily create the
> curator framework and then cache it.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
