accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From thomas.a.nelson@gmail.com <thomas.a.nel...@gmail.com>
Subject Re: commons-vfs2.jar 2.2 buggy
Date Fri, 02 Nov 2018 17:08:13 GMT


On 2018/10/30 13:41:24, Christopher <ctubbsii@apache.org> wrote: 
> On Fri, Oct 26, 2018 at 1:34 PM Andrew Hulbert <andrew.hulbert@ccri.com> wrote:
> >
> > Matt,
> >
> > We are running into similar issues with the 2.2 VFS jar running on
> > Accumulo 1.9.2 after upgrading from 1.8.1 but have been restarting
> > tservers to work around it and other issues with putting the iterators
> > in /tmp on certain systems.
> >
> > In general though we love it because we can run multiple versions of
> > iterators on the same cluster and we have it deployed on several systems
> > with our clients for that specific use case.
> >
> > Sean/Chris, if we rip it out would you imagine iterators being more like
> > HBase where you are basically bound to the startup classpath as the
> > baseline mechanism (with user-enabled specific class loaders). Or do you
> > imagine another upgrade/configuration mechanism? FYI we do VFS and the
> > general accumulo mechanism for configuring iterators and the iterator
> > api design because its pretty user/developer friendly.
> >
> 
> My suggestion wasn't really about loss of functionality. Rather, the
> way I envision it, it is about supporting that same functionality with
> a modular design that is more maintainable, flexible, uses standard
> configuration mechanisms, and is properly segregated from the core
> code base.
> 
> Even after "ripping out" our custom class loading from Accumulo's core
> code base, you could still do what you're doing today... it just might
> have to be configured more explicitly when you deploy your Accumulo
> cluster. And, the class loader code itself may be developed and
> maintained as a separate library from Accumulo itself. Accumulo
> wouldn't be tightly bound to that specific class loader
> implementation, and that specific implementation wouldn't be tightly
> bound to Accumulo.
> 
> Regardless of changes to the startup class loading, we also have the
> per-table contexts in Accumulo, which allow separate class paths for
> each table, which we'd still support in some way (though perhaps in
> future, it could be implemented a bit better and configuring it would
> be more explicit).
> 
> > Thanks,
> >
> > Andrew
> >
> >
> > On 10/24/2018 10:55 PM, Christopher wrote:
> > > The idea that Dave is talking about is that I don't think we should be
> > > doing any classloader special sauce in accumulo-start at all, and we
> > > might even be able to remove accumulo-start as a module entirely,
> > > since this is its primary (sole?) purpose.
> > >
> > > It's just a rough idea that I've tossed around with a few people, but
> > > haven't really spent any time materializing it into a proposal, PR, or
> > > experiment. Basically, I think we should rip out all classloader
> > > special sauce. If a user still wishes to use a custom classloader for
> > > any reason, using vfs2 or anything else, they can set a system class
> > > loader with -Djava.system.class.loader=my.custom.CustomClassLoader
> > > when they run java. This is an advanced Java option supported by Java
> > > itself, and really shouldn't be a problem to punt this downstream.
> > > Classloading is way outside the scope of what Accumulo does anyway,
> > > and Accumulo should have its complexity centered around what it does,
> > > and not "bells and whistles" on top of basic Java language functions.
> > >
> > > If we wanted to, we could use our current classloading code to create
> > > a classloader which could be used this way... and maybe provide it as
> > > an example or explain it in a blog post. But, Accumulo shouldn't be
> > > doing special sauce class loading... there are other projects that are
> > > better suited to specializing that for any Java application... and
> > > there's no reason we need it so tightly coupled to Accumulo.
> > >
> > > Of course, there's still some utility in the per-table context
> > > classloaders for pluggable components like iterators... and there's
> > > probably room for improvement in the configuration of those... but the
> > > main startup classloading is probably best to rip out.
> > >
> > > I'm not sure if it should be done for 2.0 or not... maybe yes. I'd be
> > > willing to rip it out... I enjoy ripping things out and reducing code
> > > complexity. But, I don't really have a desire to do the work of
> > > implementing or blogging about alternatives, if that's even necessary.
> > > I'd hope that somebody else would do that, if they felt it was really
> > > necessary once the built-in stuff was ripped out. For me, I'd be happy
> > > mentioning the feature in the release notes, maybe linking to the docs
> > > on the feature, and leaving implementation as an exercise for
> > > downstream, with an open invitation for a guest blog on our website
> > > about how it could be done.
> > >
> > > I've been thinking we're probably going to want a second alpha... or a
> > > beta, before 2.0 final... and if we did this for 2.0, I'd definitely
> > > want another pre-release release first.
> > >
> > > On Wed, Oct 24, 2018 at 3:10 PM Dave Marion <dmarion18@gmail.com> wrote:
> > >> I have talked with Christopher about the VFS class loader in general and
I
> > >> think he has a good approach. He can elaborate further if needed, but the
> > >> approach is to move it out of the core project and allow users to configure
> > >> it at runtime using the java.system.class.loader system property. There
are
> > >> organizations using the VFSClassloader successfully, maybe it just needs
to
> > >> be reimplemented.
> > >>
> > >> On Wed, Oct 24, 2018 at 2:58 PM Sean Busbey <busbey@cloudera.com.invalid>
> > >> wrote:
> > >>
> > >>> sounds like a good DISCUSS thread for 2.0?
> > >>> On Wed, Oct 24, 2018 at 1:43 PM Josh Elser <elserj@apache.org>
wrote:
> > >>>> It seems like commons-vfs2 is just a pile of crap.
> > >>>>
> > >>>> It's been known to have bugs for years and we've seen zero progress
from
> > >>>> them on making them better.
> > >>>>
> > >>>> IMO, rip the whole damn thing out.
> > >>>>
> > >>>> On 10/24/18 12:42 PM, Matthew Peterson wrote:
> > >>>>> Hello Accumulo,
> > >>>>>
> > >>>>> Summary: commons-vfs2 version 2.2 seems to have problems and
it may be
> > >>>>> worth rolling back to version 2.1 of commons-vfs2.
> > >>>>>
> > >>>>> My project upgraded a system from Accumulo 1.8.1 to 1.9.2.
 Immediately
> > >>>>> after switching vfs contexts we saw problems.  The tservers
would
> > >>> error in
> > >>>>> iterators about missing classes that were clearly on the classpath.
> > >>> The
> > >>>>> problems were persistent until we replaced the commons-vfs2.jar
with
> > >>>>> version 2.1 (Accumulo 1.9.2 uses version 2.2).  Until we rolled
vfs
> > >>> back,
> > >>>>> we received errors particularly with Spring code trying to
access
> > >>> various
> > >>>>> classes and files within the jars.  It looks like in 2.2, commons-vfs
> > >>>>> implemented a doDetach method which closed the zip files. 
We suspect
> > >>> that
> > >>>>> code is the problem but haven't tested that theory.
> > >>>>>
> > >>>>> I suspect that most users don't use this feature.
> > >>>>>
> > >>>>> Thanks!
> > >>>>> Matt
> > >>>>>
> > >>>
> > >>>
> > >>> --
> > >>> busbey
> > >>>
> >
> I wrote a unit test in commons-vfs2 that opens and accesses a jar file with multiple
threads. It will often throw the same exception we see (that the ZipFileObject is closed).
By changing the CacheStrategy to MANUAL instead of ON_RESOLVE the test no longer throws the
exception because the new code in ZipFileObject::doDetach (which closes the file ZipFileSystem)
is not called.

AbstractFileSystem::resolveFile checks if the CacheStrategy is ON_RESOLVE, and if so, it calls
file.refresh() which calls detach() which calls doDetach().

Mime
View raw message