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
