Hi Steve, Sorry, I have not read the entire thread carefully, so please disregard if not relevant.
Would this provide functionality like what we discussed for an OperationContext? https://hibernate.atlassian.net/browse/HHH-10478 Thanks, Gail On Fri, May 29, 2020 at 4:21 AM Steve Ebersole <st...@hibernate.org> wrote: > If/when it comes to needing that, I kind of like that continuation design. > But I agree, Yoann is right - let's leave it off until needed or until we > have specific requirements. > > Thanks everyone! > > On Fri, May 29, 2020 at 2:19 AM Sanne Grinovero <sa...@hibernate.org> > wrote: > > > On Fri, 29 May 2020 at 07:22, Yoann Rodiere <yo...@hibernate.org> wrote: > > > > > > +1 not to add surround capability initially. Sounds better to start > > simple and make things more complex when we actually need it :) > > > > Right. I didn't mean to raise additional requirements without having > > investigated those tracing libraries - what I meant really is just to > > raise awareness that we'll likely need to evolve it further when it > > comes to finally implement such things. > > > > > > > > Yoann Rodière > > > Hibernate Team > > > yo...@hibernate.org > > > > > > > > > On Fri, 29 May 2020 at 07:25, Sanne Grinovero <sa...@hibernate.org> > > wrote: > > >> > > >> Yes, I agree. > > >> > > >> On Thu, 28 May 2020, 22:11 Steve Ebersole, <st...@hibernate.org> > wrote: > > >>> > > >>> Wanted to clarify - > > >>> > > >>> Regarding incremental addition of "surround listeners", so long as we > > are all in agreement that this simply means there will be absolutely no > > surround capability ***initially*** then I am fine with that. > > >>> > > >>> On Thu, May 28, 2020 at 4:10 PM Steve Ebersole <st...@hibernate.org> > > wrote: > > >>>> > > >>>> Hm, the dynamic enable/disable stuff should be easy to handle, no? > > Depends on what specific library you are thinking of and exactly how that > > detail gets propagated to us. At the end of the day, its really as > simple > > as protecting the creation of some of these objects with `if > > (enabled)`-type checks. > > >>>> > > >>>> But again, if you have specific details in mind we can take a look. > > >>>> > > >>>> Also, I think it is not at all a good idea to even plan for > > "different types of events". In fact I'm fine with getting rid of > > LoadEvent completely from that contract and simply directly passing the > > information that is likely useful. I mean at the end of the day a > listener > > for load events is going to be interested in the same set of information. > > Yes, some will not need all of that information but that's not really a > > concern IMO. Especially if we inline the parameters and completely avoid > > the event object instantiation > > >>>> > > >>>> Regarding incremental addition of "surround listeners", so long as > we > > are all in agreement that this simply means there will be absolutely no > > surround capability then I am fine with that. > > >>>> > > >>>> > > >>>> On Thu, May 28, 2020 at 3:55 PM Sanne Grinovero < > sa...@hibernate.org> > > wrote: > > >>>>> > > >>>>> On Thu, 28 May 2020 at 21:27, Steve Ebersole <st...@hibernate.org> > > wrote: > > >>>>> > > > >>>>> > Any thoughts on this "continuation" approach? > > >>>>> > > >>>>> I love the pattern! Maybe we'll need also some ability to not > capture > > >>>>> the state for events which don't have any? > > >>>>> > > >>>>> I wonder if that implies we'll need two different event contracts: > > one > > >>>>> for the listeners which need state and one for those which don't; > but > > >>>>> I'm not eager to overcomplicate this. > > >>>>> > > >>>>> > Or maybe its just not important (yet) to handle "surround" > > handling? > > >>>>> > > >>>>> I'm confident that integration with tracing libraries would be very > > >>>>> useful and interesting to have - but indeed not having time to > > >>>>> research it properly I'm a bit afraid that it might need further > > >>>>> changes to reach excellent performance. > > >>>>> > > >>>>> For example one thing I remember is that with some libraries you're > > >>>>> supposed to have the option to enable/disable the profiling options > > >>>>> dynamically, and since there's an expectation of no overhead when > > it's > > >>>>> disabled this would need to imply having a way for the overhead of > > >>>>> allocating space for the captured state to "vanish": this might be > a > > >>>>> bit more complicated, or need to be able to take advantage of JIT > > >>>>> optimisations. > > >>>>> > > >>>>> So if we end up thinking that such event APIs need to be different > > >>>>> depending on the need for state, perhaps indeed it's better to > > >>>>> postpone the design of those with state to when someone has time to > > >>>>> research an optimal integration with a tracing library. It might > not > > >>>>> be too hard, I just haven't explored it myself yet. > > >>>>> > > >>>>> Maybe let's do this incrementally, considering the "continuation" > > >>>>> approach a next step? > > >>>>> > > >>>>> Thanks, > > >>>>> Sanne > > >>>>> > > >>>>> > > > >>>>> > On Wed, May 27, 2020 at 9:27 AM Steve Ebersole < > > st...@hibernate.org> wrote: > > >>>>> >> > > >>>>> >> Inline... > > >>>>> >> > > >>>>> >> On Wed, May 27, 2020 at 8:10 AM Sanne Grinovero < > > sa...@hibernate.org> wrote: > > >>>>> >>> > > >>>>> >>> At high level I agree, just have 3 more thoughts: > > >>>>> >>> > > >>>>> >>> # Regarding the "swap" of information between listeners - could > > that > > >>>>> >>> even work? I might have misunderstood something, but wouldn't > we > > >>>>> >>> require listeners to run in some specific order for such > > swapping to > > >>>>> >>> work? > > >>>>> >> > > >>>>> >> > > >>>>> >> This is why we allow control over the ordering of the registered > > listeners. And yes, that is and was a hokey solution. Nothing changes > > there really if that is why you are using load listener. > > >>>>> >> > > >>>>> >> > > >>>>> >>> > > >>>>> >>> # The "surround advice" you mention for e.g. timing seems very > > >>>>> >>> interesting, especially as I'd love us to be able to integrate > > with > > >>>>> >>> tracing libraries - but these would need to be able to > co-relate > > the > > >>>>> >>> pre-load event with some post-load event. How would that work? > > I'd > > >>>>> >>> expect these to need having a single listener implementation > > which > > >>>>> >>> implements both PreLoadEventListener and PostLoadEventListener, > > but > > >>>>> >>> also they'll likely need some capability to store some > > information > > >>>>> >>> contextual to the "event". > > >>>>> >> > > >>>>> >> > > >>>>> >> I was just thinking through this one as well. My initial > thought > > was exactly what you proposed - some combination of pre/post listener > with > > some ability to store state between. But that gets ugly. > > >>>>> >> > > >>>>> >> Another option I thought about is easier to illustrate, but > > basically works on the principle of "continuation" many surround advice > > solutions are based on: > > https://gist.github.com/sebersole/142765fe2417492061e92726e7cb6bd8 > > >>>>> >> > > >>>>> >> I kept the name LoadEventListener there, but since it changes > the > > contract anyway I'd probably rename this to something like > > SurroundLoadEventListener > > >>>>> >> > > >>>>> >> > > >>>>> >>> > > >>>>> >>> # To clarify on my previous comment regarding why I'd consider > > having > > >>>>> >>> an actual Event class more maintainable: > > >>>>> >>> Sure we won't have inline classes widely used for a while, but > I > > >>>>> >>> prefer planning for the long term - also we could start using > > them > > >>>>> >>> very soon via multi-release jars, which would simply imply that > > users > > >>>>> >>> on newer JDKs would see more benefits than other users. > > >>>>> >>> But especially, such event instances are passed over and over > > across > > >>>>> >>> many methods; so in terms of maintenance and readability, such > > methods > > >>>>> >>> would need to pass many parameters rather than one: the example > > made > > >>>>> >>> above is oversimplifying our use. Also while I understand it's > > >>>>> >>> unlikely, having a "cheap" event objects makes it easier to > > change the > > >>>>> >>> exact types being passed on. > > >>>>> >>> BTW stack space is cheap but forcing many references to be > > passed when > > >>>>> >>> one single reference could do might also have some performance > > >>>>> >>> implications since these are passed many times - I've never > > tested > > >>>>> >>> this scientifically though :) Inline objects would typically > be > > >>>>> >>> allocated on the stack as well, but they don't force the JVM to > > do so. > > >>>>> >>> > > >>>>> >>> > > >>>>> >>> Also while I said that it's unlikely we want to change those > > types, > > >>>>> >>> the very coming of inline types might actually encourage us to > > make > > >>>>> >>> changes in this area, even though these events have been stable > > for > > >>>>> >>> years; for example "String entityName" seems like an excellent > > >>>>> >>> candidate to become "EntityName typeIdentifier" - and then > allow > > us to > > >>>>> >>> improve the persister maps, which have been a bottleneck in the > > past. > > >>>>> >>> So sure we could remove them and just pass parameters, we'd > just > > need > > >>>>> >>> to change more code if such a situation arises - I'm just > > highliting > > >>>>> >>> the drawbacks for our consideration, not recommending against > it > > :) > > >>>>> >> > > >>>>> >> > > >>>>> >> Maybe its simply a difference of wording, but to me none of this > > validates how keeping an event class is more maintainable. If you want > to > > say that eventually the overhead of having an actual event class will be > > less, ok, but that's different. > > >>>>> >> > > >>>>> >> For sure though we'd have lots of uses for in-line value types > > throughout the code base. Just not sure this really an argument for > > keeping the event impl in-and-of-itself. > > >>>>> >> > > > _______________________________________________ > 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
