ctubbsii commented on code in PR #3400:
URL: https://github.com/apache/accumulo/pull/3400#discussion_r1192914147
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -36,25 +39,30 @@ private ClassLoaderUtil() {
/**
* Initialize the ContextClassLoaderFactory
*/
- public static synchronized void initContextFactory(AccumuloConfiguration
conf) {
+ public static synchronized void initContextFactory(final
AccumuloConfiguration conf) {
if (FACTORY == null) {
LOG.debug("Creating {}", ContextClassLoaderFactory.class.getName());
String factoryName =
conf.get(Property.GENERAL_CONTEXT_CLASSLOADER_FACTORY);
if (factoryName == null || factoryName.isEmpty()) {
- // load the default implementation
LOG.info("Using default {}, which is subject to change in a future
release",
ContextClassLoaderFactory.class.getName());
- FACTORY = new DefaultContextClassLoaderFactory(conf);
- } else {
- // load user's selected implementation
- try {
- var factoryClass =
Class.forName(factoryName).asSubclass(ContextClassLoaderFactory.class);
- LOG.info("Creating {}: {}",
ContextClassLoaderFactory.class.getName(), factoryName);
- FACTORY = factoryClass.getDeclaredConstructor().newInstance();
- } catch (ReflectiveOperationException e) {
- throw new IllegalStateException("Unable to load and initialize
class: " + factoryName, e);
- }
+ factoryName = DefaultContextClassLoaderFactory.class.getName();
+ }
+
+ try {
+ var factoryClass =
Class.forName(factoryName).asSubclass(ContextClassLoaderFactory.class);
+ LOG.info("Creating {}: {}", ContextClassLoaderFactory.class.getName(),
factoryName);
+ FACTORY = factoryClass.getDeclaredConstructor().newInstance();
+ } catch (ReflectiveOperationException e) {
+ throw new IllegalStateException("Unable to load and initialize class:
" + factoryName, e);
}
+
+ FACTORY.setEnvironment(new ContextClassLoaderEnvironment() {
+ @Override
+ public ServiceEnvironment.Configuration getConfiguration() {
+ return new ConfigurationImpl(conf);
+ }
+ });
Review Comment:
This can be a lambda instead of an anonymous class.
##########
core/src/main/java/org/apache/accumulo/core/spi/common/ContextClassLoaderEnvironment.java:
##########
Review Comment:
I tried to see if we could just use ServiceEnvironment here instead of
creating a custom environment that had only a subset of ServiceEnvironment
(it's Configuration). However, that became difficult because we have
ServiceEnvironmentImpl defined the server/base jar, and the ClassLoaderUtil is
defined in the core jar, because we also use it for the shell (a self-imposed
limitation because of a previous decision we're stuck with, to make server-side
classloading features available in the client-side shell). So, I then tried to
see if I could pass the ServiceEnvironment (ClientServiceEnvironmentImpl from
the shell, and ServiceEnvironmentImpl for server-side processes), but things
got unwieldy. Plus, ServiceEnvironment has methods to instantiate classes, and
creates a confusing cyclic dependency in the API, where the environment uses
the classloader, and the classloader uses the environment. This bootstrapping
issue was one of the reasons we decided against granting this factory acces
s to Accumulo environment, and specifically made a javadoc comment explaining
that its configuration needed to be passed in differently.
However, while there are problems providing a full service environment, this
PR is providing a much more narrow, configuration-only environment, which I
think is acceptable.
##########
core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfigurationAdapter.java:
##########
Review Comment:
This file isn't needed if the default factory is loaded directly instead of
via reflection, as it was doing before.
##########
core/src/main/java/org/apache/accumulo/core/classloader/DefaultContextClassLoaderFactory.java:
##########
Review Comment:
No changes need to be made to the default factory if this is loaded directly
in ClassLoaderUtil instead of via reflection, like it was doing before this
change. This file can be reverted.
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -36,25 +39,30 @@ private ClassLoaderUtil() {
/**
* Initialize the ContextClassLoaderFactory
*/
- public static synchronized void initContextFactory(AccumuloConfiguration
conf) {
+ public static synchronized void initContextFactory(final
AccumuloConfiguration conf) {
if (FACTORY == null) {
LOG.debug("Creating {}", ContextClassLoaderFactory.class.getName());
String factoryName =
conf.get(Property.GENERAL_CONTEXT_CLASSLOADER_FACTORY);
if (factoryName == null || factoryName.isEmpty()) {
- // load the default implementation
LOG.info("Using default {}, which is subject to change in a future
release",
ContextClassLoaderFactory.class.getName());
- FACTORY = new DefaultContextClassLoaderFactory(conf);
- } else {
- // load user's selected implementation
- try {
- var factoryClass =
Class.forName(factoryName).asSubclass(ContextClassLoaderFactory.class);
- LOG.info("Creating {}: {}",
ContextClassLoaderFactory.class.getName(), factoryName);
- FACTORY = factoryClass.getDeclaredConstructor().newInstance();
- } catch (ReflectiveOperationException e) {
- throw new IllegalStateException("Unable to load and initialize
class: " + factoryName, e);
- }
+ factoryName = DefaultContextClassLoaderFactory.class.getName();
Review Comment:
The default implementation is intentionally treated differently, so users
cannot configure it or instantiate it directly. It's for internal use only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
