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
