Load event handling does not have "anything" parameters. Am I understanding you correctly?
On Fri, Jun 5, 2020, 11:42 AM Gail Badner <gbad...@redhat.com> wrote: > 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