Hi Ivan, Thanks for the feedback. I've attempted to respond inline with some points I may not have explained well in the original post.
On Thu, Sep 17, 2020 at 3:18 PM ivan bella <i...@ivan.bella.name> wrote: > > I like the idea of simplifying the dependencies that Accumulo has. As > long as we can still have the ability to monitor a file system (e.g. hdfs) > and reload the jars from a specified classpath when it has changed. This is > needed for loading jars for our deployed application and managing patches, > but then a second class loader would load the accumulo jars from disk. If the > reloading classloader is the system classloader, will it still be able to > fall back to the java classloader that uses CLASSPATH? Also I missed where > the reloading vfs classloader would actually live? > Using a system class loader instead of the baked-in, there should be no change in the use cases supported... only how it is configured. Setting it as a system class loader would make it "sit on top of" the default system class loader that Java uses, and it could be implemented to either check the parent first, or attempt loading itself before falling back to the parent. As for where it would actually live... that was one of the things I specifically named in my original post as requesting feedback on. The options we have considered are: trying to get it pushed into commons-vfs, or just creating a new repository / subproject under Accumulo (just like our accumulo-maven-plugin or accumulo-proxy repos). > I believe we will still end up having a stack of classloaders as we do > today so I am not sure this is really simpler. It appears we are simply > replacing the Vfs classloader with a reloading classloader that can then > delegate to the vfs classloader. Perhaps I am missing something here. The code in Accumulo would be simpler. We wouldn't need to maintain the 5 separate class loaders in Accumulo today: ls start/src/main/**/*lass*oader*.java And, we wouldn't need to immediately launch a separate thread to run our main methods under a different class loader, or remember to do anything special when loading pluggable classes set in the config (unless they are loaded in a context). The context class loader would be simpler, because we'd maintain only a simple reference implementation for that. In the separate repo, the new reloading vfs class loader could be simpler, too... as it could focus on a narrow set of easy-to-configure features, rather than trying to intermingle or handle special cases to bootstrap Accumulo code... all that bootstrapping would be handled by Java's own feature on its own. The dependency tree (and potential dependency conflicts) would be simpler as well, if the feature is not used (since it would be optional). Of course, this wouldn't apply to those who *do* use the feature, but it means the blame for problems using the feature should be easier to identify (and possibly correct). Accumulo build testing would also be simpler, as we'd have fewer complications setting up our test cases (and a few fewer tests to run). > > > > On September 14, 2020 5:30 PM Christopher <ctubb...@apache.org> wrote: > > > > > > Hi Accumulo Devs, > > > > Lately, Dave Marion (Apache ID: dlmarion) has been working on > > prototyping some new class loader concepts for Accumulo that he and I > > have discussed, and I wanted to pitch the idea here for consideration > > for the project. > > > > # Background: > > > > Accumulo currently has two classloaders that are instantiated at > > startup, and which can be used to bootstrap Accumulo dependencies (at > > least, those not needed for the classloader code itself). This allows > > us to use the `general.classpaths`[1] and > > `general.dynamic.classpaths`[2] properties, as well as the per-context > > classloaders (`general.vfs.*`[3] and `table.classpath.context`[4]) for > > things like iterator class isolation. Since 2.0.0, we have deprecated > > `general.classpaths` and `general.dynamic.classpaths`, the former > > supplanted by the better use of the `CLASSPATH` environment variable > > (along with much improved scripts in 2.0.0), and the latter being > > replaceable by a user-provided class loader using the built-in Java > > property, `java.system.class.loader`[5], at their discretion. > > > > # The Problem: > > > > The main problem with the current code is: complexity. Accumulo is > > already complex enough without needing to be in the business of > > developing and supporting complex custom class loading features, > > especially when users have viable alternatives that can be better > > supported by independent, dedicated projects. Furthermore, these > > custom class loaders also have a dependency on commons-vfs2, which has > > been the source of numerous problems and bugs that we have needed to > > deal with, and which affect Accumulo, even though they are not > > necessarily bugs in Accumulo itself. This also brings in a lot of > > optional dependencies that aren't needed by users who don't rely on > > these features. > > > > # The Requirements: > > > > In spite of these problems, I believe we still want to enable the use > > cases that our classloaders are currently enabling. > > > > Specifically, > > 1) the ability to have separate contexts for iterator class isolation > > (A/B testing of iterators, updating iterators in a live system, etc.), > > and > > 2) the ability for users to bootstrap their class path from some other > > distributed storage than local disk. > > > > # The Proposal: > > > > 1. Create a new reloading vfs class loader, with similar functionality > > as our current two-classloaders that do the reloading and provide vfs > > features, that can be easily used as a system class loader, if the > > user chooses to, and deprecate (for removal in 3.0) the built-in > > implementations. This class loader could not only be used with > > Accumulo, but it could also be used by any other project that chooses > > to use it, because it will not have much, if any, dependencies beyond > > commons-vfs2, and will certainly not depend on Accumulo. Creating this > > separate class loader provides us a path forward to simplify Accumulo > > by removing these features from Accumulo directly (the properties are > > already deprecated), and enabling it to be maintained independently. > > 2. Create a new class loader factory property in Accumulo, with > > corresponding SPI interface, for users to provide their own > > implementation of a class loader factory, that can map a per-table > > "context" to a ClassLoader of the implementation's choosing. > > > > The result of doing these two things will allow us to more flexibly > > support user class loading needs, without being directly responsible > > for class loading implementations inside Accumulo's core code. All the > > same functionality that is available today will continue to exist, but > > will be configured differently. The resulting code in Accumulo will be > > dramatically simpler, as we would no longer have any complex class > > loading implementations in our code base, and we would no longer have > > any direct dependency on commons-vfs2, which has been problematic. > > Independent implementations may use commons-vfs2, or something else, > > but will be more easily testable and maintainable as independent > > projects that are pluggable in Accumulo. > > > > Dave has already been working on prototyping these proposed changes, > > and it is looking very feasible. > > > > We are now ready to: > > 1. get feedback on the overall proposal, and > > 2. decide on where to maintain the separate class loader. > > > > For where to maintain, the options seem to be: A) try to donate to > > commons-vfs2 OR B) maintain as a new repository, > > accumulo-vfs-classloader. > > > > Note that we have not yet proposed the idea of a user-facing, > > configurable, reloading vfs classloader to the commons-vfs2 > > developers. We wanted to get our own community's feedback on this > > first. > > > > Please discuss. > > > > Thanks, > > Christopher (in collaboration with Dave) > > > > [1]: > > https://accumulo.apache.org/docs/2.x/configuration/server-properties#general_classpaths > > [2]: > > https://accumulo.apache.org/docs/2.x/configuration/server-properties#general_dynamic_classpaths > > [3]: > > https://accumulo.apache.org/docs/2.x/configuration/server-properties#general_vfs_context_classpath_prefix > > [4]: > > https://accumulo.apache.org/docs/2.x/configuration/server-properties#table_classpath_context > > [5]: > > https://docs.oracle.com/javase/8/docs/api/java/lang/ClassLoader.html#getSystemClassLoader%E2%80%93
