[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3136: WIP - remove VFS and only search file system for jars for context classloading
ctubbsii commented on code in PR #3136:
URL: https://github.com/apache/accumulo/pull/3136#discussion_r1099852723
##
core/src/main/java/org/apache/accumulo/core/classloader/DefaultContextClassLoaderFactory.java:
##
@@ -48,55 +46,49 @@ public class DefaultContextClassLoaderFactory implements
ContextClassLoaderFacto
private static final Logger LOG =
LoggerFactory.getLogger(DefaultContextClassLoaderFactory.class);
private static final String className =
DefaultContextClassLoaderFactory.class.getName();
- @SuppressWarnings("removal")
- private static final Property VFS_CONTEXT_CLASSPATH_PROPERTY =
- Property.VFS_CONTEXT_CLASSPATH_PROPERTY;
+ // Do we set a max size here and how long until we expire the classloader?
+ private final Cache contexts =
+ Caffeine.newBuilder().maximumSize(100).expireAfterAccess(1,
TimeUnit.DAYS).build();
- public DefaultContextClassLoaderFactory(final AccumuloConfiguration accConf)
{
+ public DefaultContextClassLoaderFactory() {
if (!isInstantiated.compareAndSet(false, true)) {
throw new IllegalStateException("Can only instantiate " + className + "
once");
}
-Supplier> contextConfigSupplier =
-() ->
accConf.getAllPropertiesWithPrefix(VFS_CONTEXT_CLASSPATH_PROPERTY);
-setContextConfig(contextConfigSupplier);
-LOG.debug("ContextManager configuration set");
-startCleanupThread(accConf, contextConfigSupplier);
}
- @SuppressWarnings("deprecation")
- private static void setContextConfig(Supplier>
contextConfigSupplier) {
-org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader
-.setContextConfig(contextConfigSupplier);
- }
+ @Override
+ public ClassLoader getClassLoader(String contextName) {
Review Comment:
I was just calling it "context". The semantics of the context, whether it's
a URL, a path, a name, etc., depends on the implementation. But generically,
it's just a context, and the factory's job is to map a context to a
ClassLoader, effectively equivalent to `Function`. You
could call it `context` or `contextString` or similar. I agree it doesn't make
sense to call it a `name`, though it could be a name for some implementations.
Another thing about naming: this should not be called
`DefaultContextClassLoaderFactory`. It should be named for how it behaves...
not it's current status as the default. It will be very confusing (and I've
seen this happen) if the default is changed, and it's no longer the default.
So, for example, the default could change from `DefaultThing` to `MyThing`, and
so then when people say `default thing`, you don't know if they mean the
default `MyThing` or the non-default `DefaultThing`. In this case, it could
just be `URLClassLoaderFactory` or `URLContextClassLoaderFactory`, and return
instances of `URLClassLoader` objects, given a context that represents a list
of URLs.
--
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: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3136: WIP - remove VFS and only search file system for jars for context classloading
ctubbsii commented on code in PR #3136:
URL: https://github.com/apache/accumulo/pull/3136#discussion_r1052580972
##
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##
@@ -1092,21 +1079,8 @@ public enum Property {
+ "constraint.",
"2.0.0"),
- // VFS ClassLoader properties
-
- // this property shouldn't be used directly; it exists solely to document
the default value
- // defined by its use in AccumuloVFSClassLoader when generating the property
documentation
- @Deprecated(since = "2.1.0", forRemoval = true)
- VFS_CLASSLOADER_SYSTEM_CLASSPATH_PROPERTY(
-
org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.VFS_CLASSLOADER_SYSTEM_CLASSPATH_PROPERTY,
- "", PropertyType.STRING,
- "Configuration for a system level vfs classloader. Accumulo jar can be"
- + " configured here and loaded out of HDFS.",
- "1.5.0"),
- @Deprecated(since = "2.1.0", forRemoval = true)
- VFS_CONTEXT_CLASSPATH_PROPERTY(
-
org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.VFS_CONTEXT_CLASSPATH_PROPERTY,
- null, PropertyType.PREFIX,
+ // ClassLoader properties
+ CONTEXT_CLASSPATH_PROPERTY(ContextManager.CONTEXT_CLASSPATH_PROPERTY, null,
PropertyType.PREFIX,
Review Comment:
The idea previously discussed was that the
`general.context.class.loader.factory` is the singleton service for creating
class loaders based on a specified context in the `table.class.loader.context`
property. This is also what the descriptions of those properties say. So, the
interpretation of the `table.class.loader.context` value is dependent on which
implementation factory has been set in `general.context.class.loader.factory`.
If you set a URLClassLoader factory, you can just set the
`table.class.loader.context` property to be a set of URLs. You don't need a
separate property at all. You can also have a more advanced factory that just
takes a simplified name, and maps that name to a set of URLs or whatever, in
its own configuration file.
Basically, all we need is the factory that does `String context ->
ClassLoader` mapping, and the property that stores the `String context`. That
context could be a name, a number, a JSON blob, a comma-separated set of URLs
(or using another delimiter than a comma), or whatever the user can imagine for
their particular implementation.
By default, though, we discussed having a URLClassLoader that interprets the
context field as a set of URLs. If you're concerned about misconfiguration, the
implementation could require the set of URLs have some kind of prefix to
validate, as in `urls:url1,url2,url3,...`, and a separate project could create
a VFS classloader that had similar validation, `vfs:url1,url2,url3,...`, so
mis-configurations are more detectable. But, validation like that is not a
requirement of the implementing factory.
--
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: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3136: WIP - remove VFS and only search file system for jars for context classloading
ctubbsii commented on code in PR #3136:
URL: https://github.com/apache/accumulo/pull/3136#discussion_r1052569717
##
shell/src/main/java/org/apache/accumulo/shell/commands/ClasspathCommand.java:
##
@@ -30,9 +30,9 @@ public class ClasspathCommand extends Command {
public int execute(final String fullCommand, final CommandLine cl, final
Shell shellState) {
final PrintWriter writer = shellState.getWriter();
-
org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.printClassPath(s
-> {
- writer.print(s);
-}, true);
+//
org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.printClassPath(s
-> {
+// writer.print(s);
+// }, true);
Review Comment:
Was the command previously deprecated? If not, we could at least dump
`java.class.path` to make it *somewhat* useful without just removing it, and
possibly causing scripts to break.
--
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: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3136: WIP - remove VFS and only search file system for jars for context classloading
ctubbsii commented on code in PR #3136:
URL: https://github.com/apache/accumulo/pull/3136#discussion_r1052506732
##
start/src/main/java/org/apache/accumulo/start/classloader/vfs/ContextManager.java:
##
@@ -19,95 +19,96 @@
package org.apache.accumulo.start.classloader.vfs;
import java.io.IOException;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import java.util.function.Supplier;
+import java.util.stream.Collectors;
-import org.apache.commons.vfs2.FileSystemException;
-import org.apache.commons.vfs2.FileSystemManager;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@Deprecated
-class ContextManager {
+//@Deprecated
+public class ContextManager {
Review Comment:
This class was deprecated with the expectation to remove it entirely. What
do we still need it for?
##
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##
@@ -1092,21 +1079,8 @@ public enum Property {
+ "constraint.",
"2.0.0"),
- // VFS ClassLoader properties
-
- // this property shouldn't be used directly; it exists solely to document
the default value
- // defined by its use in AccumuloVFSClassLoader when generating the property
documentation
- @Deprecated(since = "2.1.0", forRemoval = true)
- VFS_CLASSLOADER_SYSTEM_CLASSPATH_PROPERTY(
-
org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.VFS_CLASSLOADER_SYSTEM_CLASSPATH_PROPERTY,
- "", PropertyType.STRING,
- "Configuration for a system level vfs classloader. Accumulo jar can be"
- + " configured here and loaded out of HDFS.",
- "1.5.0"),
- @Deprecated(since = "2.1.0", forRemoval = true)
- VFS_CONTEXT_CLASSPATH_PROPERTY(
-
org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.VFS_CONTEXT_CLASSPATH_PROPERTY,
- null, PropertyType.PREFIX,
+ // ClassLoader properties
+ CONTEXT_CLASSPATH_PROPERTY(ContextManager.CONTEXT_CLASSPATH_PROPERTY, null,
PropertyType.PREFIX,
Review Comment:
This is a new property. I don't think we need this. We are removing this
feature in favor of the system classloader, which is already configurable by
Java. We never need to specify a classpath in properties, just a context name
on a per-table basis, which is interpreted based on the implementation details
of the classloader factory, which should be pluggable SPI.
##
shell/src/main/java/org/apache/accumulo/shell/commands/ClasspathCommand.java:
##
@@ -30,9 +30,9 @@ public class ClasspathCommand extends Command {
public int execute(final String fullCommand, final CommandLine cl, final
Shell shellState) {
final PrintWriter writer = shellState.getWriter();
-
org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.printClassPath(s
-> {
- writer.print(s);
-}, true);
+//
org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.printClassPath(s
-> {
+// writer.print(s);
+// }, true);
Review Comment:
Showing the contents of the `java.class.path` system property is probably
enough here. For a configured context classloader factory, we could extend the
SPI to have a method to show information about each context and call that here
also. But, since this is run on the client side in the shell, it would have to
be set up on the client side, and not just the server-side... unless we had a
new RPC to specifically ask a random server to give us that info. And, in any
case, we wouldn't be able to have much insight into the user's configured
system classloader, if they set that... because that could do anything.
--
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: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
