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

> 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 <steve(a)hibernate.org&gt;: > 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 <andrea(a)hibernate.org&gt; > 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 <steve(a)hibernate.org&gt; 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(a)hibernate.org&gt; > >> wrote: > >> > >> > On Tue, Aug 30, 2016 at 6:27 AM Sanne Grinovero <sanne(a)hibernate.org > > > >> > wrote: > >> > > >> >> On 30 August 2016 at 10:09, Emmanuel Bernard < > emmanuel(a)hibernate.org&gt; > >> >> 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/fd4acb5d8f396201f5dccc... > >> > > >> > > >> > 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(a)lists.jboss.org > >> https://lists.jboss.org/mailman/listinfo/hibernate-dev > >> > > > _______________________________________________ > hibernate-dev mailing list > hibernate-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev >