[
https://issues.apache.org/jira/browse/YARN-1028?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13849811#comment-13849811
]
Karthik Kambatla commented on YARN-1028:
----------------------------------------
Uploaded a new patch that addresses most of [~vinodkv]'s and [~bikassaha]'s
comments.
bq. RMFailoverProxyProvider doesn't need to be public? For java users, the
clients are the public interfaces, so they don't need to use these directly?
Left it public. If users want to implement their own RMFailoverProxyProvider,
they should be able to, and this should be public.
bq. Declaration of INSTANCE in ServerRMProxy and ClientRMProxy is missing types.
bq. We don't expect users to create ClientRMProxy and ServerRMProxy. Create and
make constructors private?
bq. Now that you've created an INSTANCE, you can move createRMProxy() up into
RMProxy itself and reuse code.
bq. Why do we need this singleton like abstraction now?
Created private constructors for ClientRMProxy and ServerRMProxy, and moved the
crux of createRMProxy to RMProxy. Along with this, moved INSTANCE to RMProxy.
{{createRMProxy}} is a static method, requiring INSTANCE to be static
(singleton-like). However, INSTANCE is defined in a static block of subclasses
- ClientRMProxy and ServerRMProxy - so, left it untyped.
bq. Shouldn't RMProxy.createFailoverProxyProvider actually be in
RMFailoverProxyProvider itself?
Left it in RMFailoverProxyProvider as it is an interface. Can change it to
abstract class and move it there if preferred.
bq. Is it possible to consolidate the configuration properties for both HA and
non-HA case? If not, we'll have to fix documentation in yarn-default for non-ha
configs like connect.max-wait.ms that they are not used in HA mode.
bq. Handle HA related code up-front? Don't need to read other configs if HA is
enabled.
Changed configuration. yarn.client.failover.* configs override other configs -
described in yarn-default.xml. However, when failover configs are not
specified, they are inferred from the non-HA case. For instance, wait for ever
fails over for ever etc.
bq. instead of a config YARN_MINICLUSTER_USE_RPC, why not indicate as part of
constructor?
Left it as a config. Two reasons: (1) Constructor with a bunch of ints and
bools might not be very user-friendly, and can lead to an explosion in the
number of constructors. (2) It is confusing to have some properties in the
constructor and some in the config.
bq. This code seems to be duplicating stuff RMProxy/ClientRMProxy. Can we call
the existing methods in RMProxy instead?
Good catch. Fixed.
bq. Is this logic not valid in an HA scenario?
As discussed in this JIRA earlier, exceptionToPolicyMap logic can be extended
to apply here, but we will have to add a bunch of connection-related exceptions
here. Adding these doesn't give us much because FailoverOnNetworkException
already handles this logic for us.
bq. Will mini yarn HA cluster work without fixed ports? Can the test cover that
case also if simple to extend.
TestMiniYARNClusterForHA tests this.
bq. Dont see the initial value of this being set to -1 anywhere.
MiniYARNCluster.getActiveRMIndex() returns -1 if it can't find an active
ResourceManager. Added this information to the javadoc comments for the method.
bq. New class does not seem to add anything?
It appears so in the patch, because I am splitting an existing class
CustomNodeManager after a method. Applying the patch will show the contents. :)
bq. ConfiguredFailoverProxyProvider. the common one is also doing
proxy-caching, closing clients on provider.close()
bq. Similarly some type information missing from
RMProxy.createFailoverProxyProvider.
bq. Annotations missing for ConfiguredRMFailoverProxyProvider
bq. Nice test! It would help that after failing over we can verify that the new
active is really the one we expect to be active.
Fixed.
> Add FailoverProxyProvider like capability to RMProxy
> ----------------------------------------------------
>
> Key: YARN-1028
> URL: https://issues.apache.org/jira/browse/YARN-1028
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Bikas Saha
> Assignee: Karthik Kambatla
> Attachments: yarn-1028-1.patch, yarn-1028-2.patch, yarn-1028-3.patch,
> yarn-1028-4.patch, yarn-1028-5.patch, yarn-1028-6.patch, yarn-1028-7.patch,
> yarn-1028-8.patch, yarn-1028-9.patch, yarn-1028-draft-cumulative.patch
>
>
> RMProxy layer currently abstracts RM discovery and implements it by looking
> up service information from configuration. Motivated by HDFS and using
> existing classes from Common, we can add failover proxy providers that may
> provide RM discovery in extensible ways.
--
This message was sent by Atlassian JIRA
(v6.1.4#6159)
