In order to validate that unifying the event listener impls between
"native" and "jpa" is actually workable I applied that idea on top of 5.2.
And it works.  ANd since I did the work, I created a PR from it for us to:

   1. review that specific set of changes (slightly different accounting
   for ongoing 6.0 work)
   2. consider incorporating it into 5.2

https://github.com/hibernate/hibernate-orm/pull/1541


On Thu, Sep 1, 2016 at 1:11 AM Gunnar Morling <gun...@hibernate.org> wrote:

> > Do we want to change JpaIntegrator#integrate signature to pass its
> context
> > as a parameter object
>
> If you think that's the better approach, I'd say 6 is the right time to do
> it. Users (or integrators) will be prepared for this sort of breakage in a
> major.
>
> > do other projects supply custom
> > org.hibernate.service.spi.SessionFactoryServiceInitiator impls
>
> Yes, OGM does. Though I'm not too concerned about such change, it should
> be easy for us to adjust and at this point we don't aim
> for compatibility with several ORM (major) versions.
>
> 2016-09-01 4:09 GMT+02:00 Steve Ebersole <st...@hibernate.org>:
>
>> One contract I would need to adjust for this
>> is
>> org.hibernate.service.spi.SessionFactoryServiceInitiator#initiateService.
>> I can find all the implementations of that in ORM, but do other projects
>> supply custom org.hibernate.service.spi.SessionFactoryServiceInitiator
>> impls?
>>
>>
>> On Wed, Aug 31, 2016 at 5:18 AM andrea boriero <and...@hibernate.org>
>> wrote:
>>
>> > 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 <st...@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 <st...@hibernate.org>
>> >> wrote:
>> >>
>> >> > On Tue, Aug 30, 2016 at 6:27 AM Sanne Grinovero <sa...@hibernate.org
>> >
>> >> > wrote:
>> >> >
>> >> >> On 30 August 2016 at 10:09, Emmanuel Bernard <
>> emman...@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@lists.jboss.org
>> >> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>> >>
>> >
>> _______________________________________________
>> hibernate-dev mailing list
>> hibernate-dev@lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>
>
>
_______________________________________________
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev

Reply via email to