Remove deprecated AccumuloVFSClassLoader

[cshannon]

This PR is to resolve issue #3131 as the VFS classloader has been pulled out into an external project https://github.com/apache/accumulo-classloaders . All references to the VFS classloader have been removed (including properties, documentation, tests, etc). The classloader can still be used if desired by configuring the property GENERAL_CONTEXT_CLASSLOADER_FACTORY to use the VFS classloader that was pulled into the external project.

[cshannon]

Thanks for the feedback @ctubbsii and @dlmarion , I will rework this to remove the new property I added and to just have the existing properties and let the user provide the implementation for the class loader factory. I'll be looking at this again right after the new year to make the updates.

[cshannon]

I started taking a peak at this again and I should hopefully have time to update with the changes requested this upcoming week at some point.

[cshannon]

@ctubbsii and @dlmarion - I just pushed a new update and I have significantly reworked the PR. There's still work to be done...comments need updating, some tests still need fixing and to be updated, and there is still some old VFS references laying around, etc. But it's in a good enough state to be looked at again to make sure this new direction is correct before I finish polishing everything up.

The latest update now implements the agreed upon behavior of just re-using the existing 2 properties general.context.class.loader.factory and table.class.loader.context

If not specified the DefaultContextClassLoaderFactory now will just load a new URLClassLoader and use the table context property as a list of URIs. I got rid of the ContextManager and simplified things to just cache classloaders for a period of time in a cache (for how long is something I wasn't sure of yet). I got rid of the complex logic to remove contexts as it seemed unnecessary and a simple Caffeine cache should work.

[dlmarion]

I didn't look at the tests yet. My thought on that is that only the tests that were testing the table context need to remain, the tests that were testing the VFSClassLoader as the system classloader can probably be removed.

[cshannon]

@ctubbsii and @dlmarion - This should be ready for a final review, I ran through the sunny IT tests and they passed. I also have kicked off a full IT build as well.

[ctubbsii]

I also think it would be good to compare the before/after on the shell's classpath command and the bin/accumulo classpath command to make sure we didn't lose anything important in the output options.

[cshannon]

Something else I realized is I forgot to test this against https://github.com/apache/accumulo-classloaders to verify we can still use the VFS classloader using this new approach. That's something I will do as well (assuming the full IT tests pass and everything is good)

[dlmarion]

Looks good to me, one comment and one question

[cshannon]

I ran the full ITs and noticed just 1 failure in org.apache.accumulo.test.functional.ClassLoaderIT which I need to investigate but the other tests all passed.

[cshannon]

I think this is ready to go, I pushed more updates including removing the broken ClassLoaderIT that was failing as that no loner applies (it only was there to test the VFS reloading) and I also removed the no longer needed code from the Main start class that also only applied to VFS.

I spent a bunch of time testing to make sure the VFS classloader worked with the changes by using the accumulo-classloaders project and seems to be working well. I used these instructions https://github.com/apache/accumulo-classloaders/blob/main/modules/vfs-class-loader/TESTING.md and I tested against Uno. I tested both Setting a system classloader as well as using a context using the new instructions and properties.

A couple changes will be needed to the documentation to the instructions page (property rename for the table context, need to mention to make sure to add the commons VFS jar since it was removed from 3.0.0) otherwise the testing instructions are pretty much up to date. I can submit a small PR with the changes probably tomorrow.

I kicked off another full IT so this should be ready to merge if that passes.

[cshannon]

Full IT build passed all the tests for this, I also just pushed a PR to update testing instructions for accumulo-classloaders: apache/accumulo-classloaders#17

@ctubbsii - I think this is ready to merge, did you want to do a final review?

[ctubbsii]

@ctubbsii - I think this is ready to merge, did you want to do a final review?

You merged while I was giving it another lookover, but I don't see anything that stands out. I think you addressed all the previous things I noticed.

[cshannon]

@ctubbsii - I think this is ready to merge, did you want to do a final review?

You merged while I was giving it another lookover, but I don't see anything that stands out. I think you addressed all the previous things I noticed.

Sorry about that, it had been about a week so figured it was good to go since @dlmarion said it looked good. I also figured if we found something after that needed fixing I could go back and fix it with a follow on PR.

[ctubbsii]

No worries. You did the right thing. I was merely observing the coincidence. That we both were looking at it in the same few minutes after a week of inaction was amusing to me 😁