[hibernate-dev] Centralized access to "bootstrap only" resources

Wed Aug 31 06:17:36 EDT 2016

I'm fine with combining native and JPA events handling, about the second
point, ideally I would change the signature but due to the problems you
listed I vote for the in-line solution.

On 30 August 2016 at 19:20, Steve Ebersole <steve at hibernate.org> wrote:

> Any thoughts on the JpaIntegrator parts of the discussion?   Specifically
> there are 2 main considerations:
>
>    1. To change the Integrator#integrate contract - ideally, in retrospect,
>    #integrate probably should have taken a "parameter object" to help
> insulate
>    from these types of changes.  But I wanted to get y'alls thoughts on
> this
>    especially since this one potentially causes upgrade problems in terms
> of
>    applications or problems supporting multiple ORM versions in terms
>    of integrations.
>    2. The alternative I mentioned was to move the JpaIntegrator#integrate
>    functionality in-line with the building of the SessionFactory.  This has
>    some really nice benefits as discussed (like JPA callback support from
>    native bootstrapping), but it has some challenges to handle as well
> mainly
>    in terms of seamlessly combining the different Hibernate event listeners
>    used to implement the native versus JPA behavior.  The simple JPA
>    callback/listener case is pretty easy to support regardless.  The more
>    difficult ones are event listeners that implement event handling
>    differently () or the ones that cascade different actions depending on
>    native/jpa bootstrapping ().  I think even the latter bucket may be
> easy to
>    handle leveraging SessionFactoryOptions#isJpaBootstrap inside the
>    listeners.  The former bucket is really the one I am more concerned
> with.
>    So let's look at this as 2 distinct questions:
>       1. Do we want to combine event listeners for native and JPA handling
>       of events?
>       2. Do we want to change JpaIntegrator#integrate signature to pass its
>       context as a parameter object in order to facilitate this?  Or do we
>       in-line the decisions/actions done in JpaIntegrator into
> SessionFactory
>       init?
>
>
> On Tue, Aug 30, 2016 at 8:50 AM Steve Ebersole <steve at hibernate.org>
> wrote:
>
> > On Tue, Aug 30, 2016 at 6:27 AM Sanne Grinovero <sanne at hibernate.org>
> > wrote:
> >
> >> On 30 August 2016 at 10:09, Emmanuel Bernard <emmanuel at hibernate.org>
> >> wrote:
> >> > I am not sure if that is still relevant but in the past, either
> HSEARCH
> >> > or HV were keeping the ReflectionManager around to use it at runtime
> >> > (either because metadata was loaded lazily or because of a reboot of
> the
> >> > factories due to a configuration change.
> >> >
> >> > So we need to check that losing access to ReflectionManager after SF
> is
> >> > created won't be problematic for these projects.
> >>
> >> In the "dynamic reconfiguration" case we create our own
> >> ReflectionManager instance:
> >>  -
> >> https://github.com/hibernate/hibernate-search/blob/
> fd4acb5d8f396201f5dccc89ba3cbc07becea08a/engine/src/main/
> java/org/hibernate/search/engine/impl/IncrementalSearchConfiguration
> .java#L26-L35
> >
> >
> > Interesting that y'all do not specify classloading behavior there (the
> > ClassLoaderDelegate stuff I added to HCANN)...
> >
> >
> >
> >> Steve, we had a similar notion of "boot only available components" in
> >> Search but over time we started to have various "special needs" of
> >> various other components holding a reference on these.
> >> When I later tried to re-instate order, it was too late and we got in
> >> arguments like the API's intent not having been clear enough and too
> >> much entanglement had happened.
> >>
> >
> > Hard to say without specifics. I hate "general rules" :)
> >
> > So let's look at the specifics in terms of things I have moved to
> > BootstrapContext...
> >
> >
> >    1. HCANN ReflectionManager - as you said, y'all create your own for
> >    your use case.  You'd own the lifecycle of that one you create.  I
> see no
> >    conflict there.   Also we know that in 7.0 HCANN use will go away and
> we
> >    will move to Jandex.  The Jandex IndexView reference is only valid
> for a
> >    limited period of time when WF hands it to us.
> >    2.  JPA "temp ClassLoader" - I think this one is self-evident.  JPA
> >    states that this ClassLoader (if one) is available for only a limited
> time.
> >    3. ClassmateContext - I centralized this so that we did not have to
> >    keep "priming" the classmate caches each time we needed to use
> classmate.
> >    Aside from a possible performance hit, there really is nothing
> special here
> >    versus creating a new ClassmateContext each time you need it.  For
> ORM we
> >    currently never use classmate outside of bootstrap.  Could that
> change?
> >    Maybe, and we'd deal with that if/when it does.
> >    4. scanning components
> >    (ArchiveDescriptorFactory, ScanOptions, ScanEnvironment, Scanner) -
> maybe
> >    going back to your "dynamic reconfiguration" scenario this makes
> sense.  No
> >    idea.  But in ORM holding on to these after bootstrap makes no sense.
> >    5. I've also started making BootstrapContext the holder for bootstrap
> >    metadata-related collectors.  Here we collect
> >    SQLFunctions, AuxiliaryDatabaseObjects, AttributeConverterDefinitions,
> >    and CacheRegionDefinitions.
> >    6. There are 2 other (new in 6.0) delegates that I keep here too.
> >    Interestingly, one is fully intended to be held beyond bootstrap.
> But I
> >    think that these intentions just need to be documented.
> >
> >
> > Overall I'd view a "dynamic reconfiguration" scenario very much like a
> > limite bootstrap scenario.  Personally I'd expect to have to maker many
> of
> > these "boot only resources" available to that process.  Not necessarily
> the
> > same ones as used during the primary bootstrap though.  I personally
> would
> > prefer to not hold reference to these "just in case" we have a "dynamic
> > reconfiguration" situation later; I'd just rebuild them.  Granted things
> > like a WF-handed Jandex IndexView would be difficult to handle in there,
> > but that is the case regardless of whether we hold reference to it or
> not;
> > that has to do with WF eventually invalidating that reference it handed
> us.
> >
> >
> > So while I think it's a good idea, and also Search should try this
> >> again, I think we'd need to design it from day 1 to be defensive
> >> against future code attempting to hold on these services.
> >> Not sure what would be the best approach for ORM, but I guess that
> >> simply invalidating/closing these components after bootstrap and
> >> having these throw an exception after that would be a good start.
> >>
> >
> > That is roughly what I do.  There is a BootstrapContext#release method.
> > It in turn releases the delegates it holds.  I can add some defensive
> > checking for throwing some "unavailable" exceptions in case stuff holds
> > references to these.  That's a good idea.
> >
> >
> > However, please allow some flexibility for the case in which someone
> >> really needs one of the services you're dooming at runtime.
> >> For example Search might need to re-read configuration properties at
> >> runtime; we can of course make a copy, but then we'd need a way to be
> >> able to make such a copy (We currently actually make such a copy of
> >> the cfg Properties).
> >> Configuration properties being just an example, maybe we need a
> >> generic way to be able to declare which services should not be cleaned
> >> up after bootstrap?
> >>
> >
> > We already hold on to configuration properties into the SF.  See
> > ConfigurationService.
> >
> >
> >
> >> In practice, the services you've listed should be fine today but the
> >> need for us to make a copy (or to invoke some API to ask for a life
> >> extension) might show up in future.
> >>
> >> Rough proposal :
> >>
> >> interface BootService {
> >>   void flagForUsageBeyondBootstrap();
> >> }
> >>
> >
> > -1 I think the BootstrapContext is not the right place for this.  It is
> > not the BootstrapContext itself that needs to remain valid, it is the
> > delegates it exposes.  That is where the "extension" should be allowed.
> If
> > that is voted as generally worthwhile, I can see 2 options:
> >
> >    1. Expose #allowExtendedAccess (or somesuch method name) to the actual
> >    delegates.  This would be an indicator to not release its resources
> when
> >    the BootstrapContext#release method tells the delegate to release
> itself.
> >    2. Allow OGM, Search, etc to specify specific impls for these
> >    delegates.  It could handle the delegate's #release method however it
> >    wanted.
> >
> > However, realize that if these things are not released by
> > BootstrapContext#release then ORM washes its hands of cleaning them up
> (it
> > would have no "scope" to do that).
> >
> _______________________________________________
> hibernate-dev mailing list
> hibernate-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>