I *think* so. On Fri, Jun 5, 2020 at 12:45 PM Steve Ebersole <st...@hibernate.org> wrote:
> 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