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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
