ctubbsii commented on code in PR #3678:
URL: https://github.com/apache/accumulo/pull/3678#discussion_r1287192471
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,11 +75,28 @@ static synchronized void resetContextFactoryForTests() {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- if (context != null && !context.isEmpty()) {
- return FACTORY.getClassLoader(context);
- } else {
+ if (FACTORY == null) {
return
org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
}
Review Comment:
FACTORY isn't supposed to be null here when this is called, or below where a
similar check is made. I'm not sure I understand this part of these changes.
If these methods can be called prior to the FACTORY being instantiated, then
the FACTORY instantiation should be done lazily with a `Suppliers.memoize()`.
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,11 +75,28 @@ static synchronized void resetContextFactoryForTests() {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- if (context != null && !context.isEmpty()) {
- return FACTORY.getClassLoader(context);
- } else {
+ if (FACTORY == null) {
return
org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
}
+ ClassLoader cl;
+ try {
+ cl = FACTORY.getClassLoader(context);
+ return (cl == null)
Review Comment:
I do like the idea that the factory can return null, which just means, defer
to the system classloader. That behavior should be added to the javadoc, though.
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -82,6 +82,14 @@ public static ClassLoader getClassLoader(String context) {
}
}
+ public static boolean isValidContext(String context) {
+ if (context != null && !context.isEmpty()) {
Review Comment:
> > The empty string is the default value for `table.class.loader.context`,
so it most definitely should be valid.
>
> I'm not sure about that. There are no properties of PropertyType.STRING in
Property.java whose default value is `null`. They all either have a value, or
an empty string. I don't think that empty string is a valid String property
value in all cases. I'm sure that in some cases it means the property is not
set.
I don't think we've done a good job in the internal config code of having a
way to distinguish between set or not set, or allowing a value at one scope to
be overridden by unsetting it at a different scope. This is a shortcoming in
the current implementation, but far out of scope to fix here. The fact that
`null` isn't used is really more of a byproduct of how commons-configuration
works (it's really hard to set it as a value, because it either looks like an
empty string, or looks like no entry). In theory, it could be a value, but in
practice, you can't really set it. But empty string certainly can be set, and
is set as the default in many properties. I don't think we have anything
enforcing that `null` can't be the default value, though. If we're going to
rely on that, we should probably enforce it.
>
> > If the context property is not set for a particular table, the factory
should still get consulted.
>
> It's never worked like this. Even in `main` where the VFS stuff has been
ripped out ClassLoaderUtil only consults the ContextClassLoaderFactory when the
context name is set to a non-empty String.
That is surprising to me. I was thinking it should always be consulted where
there is a context possible. But now I'm not so sure what the right behavior
should be, because there are too many edge cases and failure scenarios. Trying
to put myself in the shoes of a user who has implemented a custom factory, it's
getting harder to imagine how they might expect things to behave with respect
to their implementation under all these scenarios. I'm leaning towards always
consulting the factory, and leaving the "return null" mean "defer to system
classloader", but I can also see an argument for "use context classloader
factory only if a non-empty context is provided" (which is the current
behavior). I'm fine with leaving the current behavior while I think about it
some more.
> However, I think I have a solution - I pushed the null and empty checks
into the factory in
[a4750ab](https://github.com/apache/accumulo/commit/a4750ab12a9694c8b2c0a5590451bdaefa311ed5).
I think pushing these checks into the factory makes sense, and allowing the
factory to return null, which causes Accumulo to use its system class loader.
However, it's still a little unclear whether contexts that result in returning
`null`, and deferring to the system class loader, are "valid" or not. Does
"valid" mean "I won't cause an error" or does valid mean "I won't return null"?
--
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]
