Re: [ovirt-devel] hibernate's internal PersistentBag sent to FE

From: "Tomas Jelinek" <tjelinek(a)redhat.com&gt; To: awels(a)redhat.com Cc: "Oved Ourfali" <ovedo(a)redhat.com&gt;, "Liran Zelkha" <lzelkha(a)redhat.com&gt;, devel(a)ovirt.org Sent: Wednesday, June 3, 2015 9:58:34 AM Subject: Re: [ovirt-devel] hibernate's internal PersistentBag sent to FE ----- Original Message ----- > From: "Alexander Wels" <awels(a)redhat.com&gt; > To: "Oved Ourfali" <ovedo(a)redhat.com&gt; > Cc: "Tomas Jelinek" <tjelinek(a)redhat.com&gt;, "Liran Zelkha" > <lzelkha(a)redhat.com&gt;, devel(a)ovirt.org > Sent: Tuesday, June 2, 2015 3:28:35 PM > Subject: Re: [ovirt-devel] hibernate's internal PersistentBag sent to FE > > On Tuesday, June 02, 2015 09:10:38 AM Oved Ourfali wrote: > > ----- Original Message ----- > > > > > From: "Tomas Jelinek" <tjelinek(a)redhat.com&gt; > > > To: awels(a)redhat.com > > > Cc: "Liran Zelkha" <lzelkha(a)redhat.com&gt;, "Oved Ourfali" > > > <ovedo(a)redhat.com&gt;, devel(a)ovirt.org Sent: Tuesday, June 2, 2015 4:08:05 > > > PM > > > Subject: Re: [ovirt-devel] hibernate's internal PersistentBag sent to > > > FE > > > > > > > > > > > > ----- Original Message ----- > > > > > > > From: "Alexander Wels" <awels(a)redhat.com&gt; > > > > To: "Liran Zelkha" <lzelkha(a)redhat.com&gt; > > > > Cc: "Tomas Jelinek" <tjelinek(a)redhat.com&gt;, "Oved Ourfali" > > > > <ovedo(a)redhat.com&gt;, devel(a)ovirt.org > > > > Sent: Tuesday, June 2, 2015 2:51:31 PM > > > > Subject: Re: [ovirt-devel] hibernate's internal PersistentBag sent to > > > > FE > > > > > > > > On Tuesday, June 02, 2015 03:05:11 AM Liran Zelkha wrote: > > > > > ----- Original Message ----- > > > > > > > > > > > From: "Tomas Jelinek" <tjelinek(a)redhat.com&gt; > > > > > > To: "Oved Ourfali" <ovedo(a)redhat.com&gt; > > > > > > Cc: awels(a)redhat.com, "Liran Zelkha" <lzelkha(a)redhat.com&gt;, > > > > > > devel(a)ovirt.org > > > > > > Sent: Tuesday, June 2, 2015 9:59:17 AM > > > > > > Subject: Re: [ovirt-devel] hibernate's internal PersistentBag > > > > > > sent > > > > > > to > > > > > > FE > > > > > > > > > > > > > > > > > > > > > > > > ----- Original Message ----- > > > > > > > > > > > > > From: "Oved Ourfali" <ovedo(a)redhat.com&gt; > > > > > > > To: awels(a)redhat.com > > > > > > > Cc: "Liran Zelkha" <lzelkha(a)redhat.com&gt;, devel(a)ovirt.org > > > > > > > Sent: Tuesday, June 2, 2015 8:35:56 AM > > > > > > > Subject: Re: [ovirt-devel] hibernate's internal PersistentBag > > > > > > > sent > > > > > > > to > > > > > > > FE > > > > > > > > > > > > > > top-posting: > > > > > > > Due to the fact that we'll see that all over the place, I > > > > > > > really > > > > > > > think > > > > > > > that > > > > > > > it would be best to support that at the frontend level, and not > > > > > > > the > > > > > > > backend > > > > > > > level. > > > > > > > > > > > > well, from philosophical perspective I think that sending > > > > > > org.hibernate.collection.**internal**.PersistentBag from backend > > > > > > to > > > > > > FE > > > > > > and > > > > > > than faking it's implementation on FE > > > > > > is a way to hell. It will bring lots of problems. For example the > > > > > > FE > > > > > > will > > > > > > be directly dependent on the internal structure of an internal > > > > > > hibernate > > > > > > class and if we update hibernate, FE can fail miserably. > > > > > > Also the FE patch (https://gerrit.ovirt.org/#/c/41682/1) seems to > > > > > > be > > > > > > more > > > > > > tricky than expected - it seems to be working but Omer had issues > > > > > > with > > > > > > it > > > > > > and in some cases (when the delegating also retainAll()) it > > > > > > caused > > > > > > my > > > > > > JRE > > > > > > to fail on SIGSEGV. > > > > > > It can be fixed on FE but it is a hack with all risks this kinds > > > > > > of > > > > > > hacks > > > > > > brings. > > > > > > > > > > I don't think it really matters where we do it, as long as server > > > > > code > > > > > will > > > > > still use the PersistentBag (otherwise we won't be able to use > > > > > attached > > > > > entities, and performance will suffer). I believe that serializing > > > > > these > > > > > objects to JSON/XML - will work, we won't need to do 2 level of > > > > > conversions > > > > > (PersistentBag-->List-->JSON/XML). So if the issue is only on the > > > > > GWT > > > > > side, > > > > > and we won't to write this code once, and not per entity - where is > > > > > the > > > > > best way to put it? > > > > > > > > The original problem comes from the fact that we are serializing an > > > > object > > > > from the backend to the frontend that the frontend cannot handle (The > > > > persistent bag). This is a backend implementation detail that the > > > > frontend > > > > really doesn't care about. In essence GWT is Javascript that is made > > > > to > > > > look > > > > like Java, but it has a bunch of limitations due to the fact it still > > > > ends > > > > up > > > > as javascript in the end. > > > > > > > > > > > Doing it in the backend level will cause a lot of overhead. > > > > > > > > > > > > Is it really lots of overhead? Comparing to all other layers the > > > > > > data > > > > > > has > > > > > > to pass anyway? When abandoning GWT FE and start to use REST we > > > > > > will > > > > > > anyway remove the GenericApiGWTServiceImpl which contains this > > > > > > cleaning > > > > > > (https://gerrit.ovirt.org/#/c/41810/). > > > > > > > > The overhead comes from having to scan through every single object > > > > that > > > > we > > > > are > > > > transferring to the front end, even though right now only one or two > > > > object > > > > types have this particular problem. The scanning only happens if we > > > > actually > > > > transfer the object to the frontend. If there was a way to determine > > > > if > > > > we > > > > need to scan the objects being returned to the front end that would > > > > cut > > > > down > > > > on the overhead. Another question to ask, is the overhead BAD enough > > > > to > > > > care? > > > > I honestly don't know without doing some profiling. But my gut is > > > > telling > > > > me > > > > no > > > > it is not. > > > > > > I agree this is the correct fix on the correct place. On the other > > > hand, > > > it > > > needs some optimizations and tests if it actually works and if it is > > > not > > > a > > > big bottleneck. > > > Also we need to consider if we want to have some annotation on classes > > > we > > > want to filter before sending back to frontend or we want this to be > > > done > > > all the time. > > > > > > Long story short, I think the patch which broke the frontend debugging > > > should be reverted and merged once the > > > https://gerrit.ovirt.org/#/c/41810/ is finished and tested. > > > > What's the ETA on finishing https://gerrit.ovirt.org/#/c/41810 ? > > > > I will be working on 41810 to make sure it is solid and is working as > expected. I am expecting it to take 1-2 days to fully complete, but I will > try > to do it asap. OK, Alexander have posted a new version of https://gerrit.ovirt.org/#/c/41810 and I think it is now good enough to be merged. I have tested it on my setup and everything seems to be working again. I think there is some room for enhancements but that enhancements can be discussed separately in a separate patch. Long story short - everyone who was stuck can now debug the FE again :) Thanx Alexander for the patch!

> > > > @Liran: could you please revert your patch? Because currently the FE > > > debugging is pretty much broken (and I don't trust the web mode > > > either...) > > > and the solution is not completely clear now. > > > > Let's see what the ETA is, as perhaps we can merge something soon. > > Liran's patch is a big change in all DB layers, so won't be easy to > > revert. > > > > > > > > > I'll leave the technical details to the UX experts here to see > > > > > > > what's > > > > > > > the > > > > > > > right approach to do it in the frontend side. > > > > > > > > > > > > > > Thanks, > > > > > > > Oved > > > > > > > > > > > > > > ----- Original Message ----- > > > > > > > > > > > > > > > From: "Alexander Wels" <awels(a)redhat.com&gt; > > > > > > > > To: "Martin Perina" <mperina(a)redhat.com&gt; > > > > > > > > Cc: "Liran Zelkha" <lzelkha(a)redhat.com&gt;, devel(a)ovirt.org > > > > > > > > Sent: Tuesday, June 2, 2015 4:16:31 AM > > > > > > > > Subject: Re: [ovirt-devel] hibernate's internal PersistentBag > > > > > > > > sent > > > > > > > > to > > > > > > > > FE > > > > > > > > > > > > > > > > On Monday, June 01, 2015 10:51:28 AM Martin Perina wrote: > > > > > > > > > ----- Original Message ----- > > > > > > > > > > > > > > > > > > > From: "Alexander Wels" <awels(a)redhat.com&gt; > > > > > > > > > > To: "Tomas Jelinek" <tjelinek(a)redhat.com&gt; > > > > > > > > > > Cc: "Liran Zelkha" <lzelkha(a)redhat.com&gt;, devel(a)ovirt.org > > > > > > > > > > Sent: Monday, June 1, 2015 4:19:47 PM > > > > > > > > > > Subject: Re: [ovirt-devel] hibernate's internal > > > > > > > > > > PersistentBag > > > > > > > > > > sent > > > > > > > > > > to > > > > > > > > > > FE > > > > > > > > > > > > > > > > > > > > On Monday, June 01, 2015 09:33:07 AM Tomas Jelinek wrote: > > > > > > > > > > > ----- Original Message ----- > > > > > > > > > > > > > > > > > > > > > > > From: "Alexander Wels" <awels(a)redhat.com&gt; > > > > > > > > > > > > To: devel(a)ovirt.org > > > > > > > > > > > > Cc: "Tomas Jelinek" <tjelinek(a)redhat.com&gt;, "Liran > > > > > > > > > > > > Zelkha" > > > > > > > > > > > > <lzelkha(a)redhat.com&gt; Sent: Monday, June 1, 2015 > > > > > > > > > > > > 3:16:34 > > > > > > > > > > > > PM > > > > > > > > > > > > Subject: Re: [ovirt-devel] hibernate's internal > > > > > > > > > > > > PersistentBag > > > > > > > > > > > > sent > > > > > > > > > > > > to > > > > > > > > > > > > FE > > > > > > > > > > > > > > > > > > > > > > > > On Monday, June 01, 2015 09:08:50 AM Tomas Jelinek > wrote: > > > > > > > > > > > > > Hey all, > > > > > > > > > > > > > > > > > > > > > > > > > > since the > > > > > > > > > > > > > org.ovirt.engine.core.common.job.Job/Step... > > > > > > > > > > > > > has > > > > > > > > > > > > > been > > > > > > > > > > > > > moved > > > > > > > > > > > > > to > > > > > > > > > > > > > use > > > > > > > > > > > > > the JPA we have a problem on frontend. The problem > > > > > > > > > > > > > is > > > > > > > > > > > > > that > > > > > > > > > > > > > the > > > > > > > > > > > > > @OneToMany > > > > > > > > > > > > > annotations results in a List which is of type > > > > > > > > > > > > > PersistentBag. > > > > > > > > > > > > > When > > > > > > > > > > > > > we > > > > > > > > > > > > > send > > > > > > > > > > > > > this to Frontend it fails during deserialization. > > > > > > > > > > > > > It > > > > > > > > > > > > > actually > > > > > > > > > > > > > fails > > > > > > > > > > > > > quite > > > > > > > > > > > > > bad because the FE already has an ui-override of it > > > > > > > > > > > > > which > > > > > > > > > > > > > is > > > > > > > > > > > > > not > > > > > > > > > > > > > correct > > > > > > > > > > > > > resulting in a ton of NPEs in development mode. > > > > > > > > > > > > > > > > > > > > > > > > > > So, there are 2 nasty fixes I have made where none > > > > > > > > > > > > > of > > > > > > > > > > > > > them > > > > > > > > > > > > > should > > > > > > > > > > > > > be > > > > > > > > > > > > > merged > > > > > > > > > > > > > but demonstrate the possibilities: 1: extend the FE > > > > > > > > > > > > > to > > > > > > > > > > > > > be > > > > > > > > > > > > > able > > > > > > > > > > > > > to > > > > > > > > > > > > > work > > > > > > > > > > > > > with > > > > > > > > > > > > > the PersistentBag > > > > > > > > > > > > > (https://gerrit.ovirt.org/#/c/41682/) > > > > > > > > > > > > > not > > > > > > > > > > > > > really > > > > > > > > > > > > > good > > > > > > > > > > > > > solution since the PersistenBag is an internal > > > > > > > > > > > > > Hibernate > > > > > > > > > > > > > class > > > > > > > > > > > > > which > > > > > > > > > > > > > is > > > > > > > > > > > > > really not meant to be passed around > > > > > > > > > > > > > > > > > > > > > > > > > > 2: fix on the backend to not send the PersistentBag > > > > > > > > > > > > > but > > > > > > > > > > > > > an > > > > > > > > > > > > > ArrayList. > > > > > > > > > > > > > This > > > > > > > > > > > > > is only a PoC fixed on a command we face the > > > > > > > > > > > > > problem > > > > > > > > > > > > > (https://gerrit.ovirt.org/#/c/41797/) Obviously > > > > > > > > > > > > > this > > > > > > > > > > > > > is > > > > > > > > > > > > > not > > > > > > > > > > > > > going > > > > > > > > > > > > > to > > > > > > > > > > > > > work > > > > > > > > > > > > > for other commands accessing the same Job nor for > > > > > > > > > > > > > other > > > > > > > > > > > > > entities. > > > > > > > > > > > > > > > > > > > > > > > > > > So, the first option is generic but very very bad. > > > > > > > > > > > > > The > > > > > > > > > > > > > second > > > > > > > > > > > > > option > > > > > > > > > > > > > should > > > > > > > > > > > > > be used but not sure how to do this in a cheep way > > > > > > > > > > > > > (e.g. > > > > > > > > > > > > > without > > > > > > > > > > > > > using > > > > > > > > > > > > > reflection to deep traverse everything sent back to > > > > > > > > > > > > > frontend > > > > > > > > > > > > > checking > > > > > > > > > > > > > if > > > > > > > > > > > > > it > > > > > > > > > > > > > does not have a PersistentBag in it. > > > > > > > > > > > > > > > > > > > > > > > > Tomas, > > > > > > > > > > > > > > > > > > > > > > > > Thanks, I was investigating the same issue, I noticed > > > > > > > > > > > > it > > > > > > > > > > > > last > > > > > > > > > > > > Friday > > > > > > > > > > > > just > > > > > > > > > > > > before leaving, so I was investigating the problem to > > > > > > > > > > > > see > > > > > > > > > > > > what > > > > > > > > > > > > was > > > > > > > > > > > > going > > > > > > > > > > > > on. You are right we should not be sending > > > > > > > > > > > > PersistentBag > > > > > > > > > > > > to > > > > > > > > > > > > the > > > > > > > > > > > > frontend > > > > > > > > > > > > at all. So how about we do a combination of [1] and > > > > > > > > > > > > [2], > > > > > > > > > > > > but > > > > > > > > > > > > instead > > > > > > > > > > > > of > > > > > > > > > > > > delegating in [1] we actually simple throw an > > > > > > > > > > > > exception > > > > > > > > > > > > stating > > > > > > > > > > > > don't > > > > > > > > > > > > sent PersistentBag to the front end. that way anyone > > > > > > > > > > > > inadvertently > > > > > > > > > > > > using > > > > > > > > > > > > it will be notified immediately (since their code > > > > > > > > > > > > won't > > > > > > > > > > > > work). > > > > > > > > > > > > > > > > > > > > > > Throwing the exception would help us in debugging but > > > > > > > > > > > the > > > > > > > > > > > main > > > > > > > > > > > question > > > > > > > > > > > is > > > > > > > > > > > how will we make it work? Since we are planning to move > > > > > > > > > > > more > > > > > > > > > > > and > > > > > > > > > > > more > > > > > > > > > > > to > > > > > > > > > > > JPA so we will face this issue more and more often. > > > > > > > > > > > Solving > > > > > > > > > > > it > > > > > > > > > > > one > > > > > > > > > > > by > > > > > > > > > > > one > > > > > > > > > > > on backend in each command is not going to work. > > > > > > > > > > > > > > > > > > > > How about something like this [1]? It appears to use some > > > > > > > > > > aspects > > > > > > > > > > to > > > > > > > > > > translate > > > > > > > > > > the hibernate internal classes into normal java util > > > > > > > > > > classes. > > > > > > > > > > > > > > > > > > > > [1] https://code.google.com/p/dehibernator/ > > > > > > > > > > > > > > > > > > Well, not sure about that from performance point of view. > > > > > > > > > > > > > > > > > > If we really move away from GWT completely in 4.0, then > > > > > > > > > implementing > > > > > > > > > all > > > > > > > > > hibernate inner collections in "uioverride" in a similar > > > > > > > > > way > > > > > > > > > as > > > > > > > > > Tomas > > > > > > > > > did > > > > > > > > > seems to me as the best solution from performance and > > > > > > > > > "easiest > > > > > > > > > to > > > > > > > > > remove > > > > > > > > > when not needed" point of view. > > > > > > > > > > > > > > > > I took some inspiration from that code and wrote my own > > > > > > > > fairly > > > > > > > > simple > > > > > > > > hibernate persistent collection replacer, and I put the patch > > > > > > > > up > > > > > > > > here > > > > > > > > [3]. > > > > > > > > It > > > > > > > > still uses reflection so it is probably not the fastest thing > > > > > > > > ever > > > > > > > > written. > > > > > > > > > > > > > > > > [3] https://gerrit.ovirt.org/#/c/41810/ > > > > > > > > > > > > > > > > > > > > Alexander > > > > > > > > > > > > > > > > > > > > > > > > > Any better ideas? > > > > > > > > > > > > > Thanx, > > > > > > > > > > > > > Tomas > > > > > > > > > > > > > _______________________________________________ > > > > > > > > > > > > > Devel mailing list > > > > > > > > > > > > > Devel(a)ovirt.org > > > > > > > > > > > > > http://lists.ovirt.org/mailman/listinfo/devel > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > > > > > > Devel mailing list > > > > > > > > > > Devel(a)ovirt.org > > > > > > > > > > http://lists.ovirt.org/mailman/listinfo/devel > > > > > > > > > > > > > > > > _______________________________________________ > > > > > > > > Devel mailing list > > > > > > > > Devel(a)ovirt.org > > > > > > > > http://lists.ovirt.org/mailman/listinfo/devel > > > > > > > > > > > > > > _______________________________________________ > > > > > > > Devel mailing list > > > > > > > Devel(a)ovirt.org > > > > > > > http://lists.ovirt.org/mailman/listinfo/devel > > _______________________________________________ Devel mailing list Devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/devel