[
https://issues.apache.org/jira/browse/YARN-4577?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15263335#comment-15263335
]
Sangjin Lee commented on YARN-4577:
-----------------------------------
Would you be able to come up with a unit test still? I know it's somewhat
tricky, but you might be able to find a way to test a few things. You might
want to take a look at {{TestRunJar.testClientClassLoader()}} to see if you can
reuse some of the ideas there.
Coupled with the unit test strategy, you might want to consider supporting some
type of an override mechanism for system classes. Other use cases of
{{ApplicationClassLoader}} provide this (although it's case by case). If users
ever run into a classloading issue which can be fixed by a small modification
to the system classes, providing the override mechanism would be a big win. And
it might come in handy in writing unit tests too.
Also, please take a look at the checkstyle and findbug issues.
(AuxServices.java)
- l.132: some INFO level logging here would be helpful (like the name of the
aux service that's using the custom classloader, etc.)
- l.146: I know it's existing code, but the C-style equal check is not
necessary with java. I would simply go with {{sClass == null}}.
- l.160: same as above.
(AuxiliaryServiceWithCustomClassLoader.java)
- in {{serviceInit()}}, {{serviceStart()}}, {{serviceStop()}}, shouldn't we
call {{wrapped.serviceInit()}} instead of {{wrapper.init()}}, and so on?
- l.46: We might want to change this a little. It appears
{{AbstractService.init()}} already sets the configuration (with the original
configuration) before {{AuxiliaryServiceWithCustomClassLoader.serviceInit()}}
is invoked. This may lead to confusion down the road. Let's reset the
configuration here via {{setConfig()}}. In other words,
{code}
@Override
protected void serviceInit(Configuration conf) throws Exception {
Configuration config = new Configuration(conf);
// reset the service configuration
setConfig(config);
config.setClassLoader(customClassLoader);
ClassLoader original = Thread.currentThread().getContextClassLoader();
Thread.currentThread().setContextClassLoader(customClassLoader);
try {
wrapped.serviceInit(config);
} finally {
Thread.currentThread().setContextClassLoader(original);
}
}
{code}
Also, I don't think we need {{Configuration}} as a member variable.
> Enable aux services to have their own custom classpath/jar file
> ---------------------------------------------------------------
>
> Key: YARN-4577
> URL: https://issues.apache.org/jira/browse/YARN-4577
> Project: Hadoop YARN
> Issue Type: Improvement
> Affects Versions: 2.8.0
> Reporter: Xuan Gong
> Assignee: Xuan Gong
> Attachments: YARN-4577.1.patch, YARN-4577.2.patch,
> YARN-4577.20160119.1.patch, YARN-4577.20160204.patch, YARN-4577.3.patch,
> YARN-4577.3.rebase.patch, YARN-4577.4.patch, YARN-4577.5.patch,
> YARN-4577.poc.patch
>
>
> Right now, users have to add their jars to the NM classpath directly, thus
> put them on the system classloader. But if multiple versions of the plugin
> are present on the classpath, there is no control over which version actually
> gets loaded. Or if there are any conflicts between the dependencies
> introduced by the auxiliary service and the NM itself, they can break the NM,
> the auxiliary service, or both.
> The solution could be: to instantiate aux services using a classloader that
> is different from the system classloader.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
