On Tue, Jun 2, 2015 at 3:10 PM, Oved Ourfali <[email protected]> wrote: > > > ----- Original Message ----- >> From: "Tomas Jelinek" <[email protected]> >> To: [email protected] >> Cc: "Liran Zelkha" <[email protected]>, "Oved Ourfali" <[email protected]>, >> [email protected] >> 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" <[email protected]> >> > To: "Liran Zelkha" <[email protected]> >> > Cc: "Tomas Jelinek" <[email protected]>, "Oved Ourfali" >> > <[email protected]>, [email protected] >> > 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" <[email protected]> >> > > > To: "Oved Ourfali" <[email protected]> >> > > > Cc: [email protected], "Liran Zelkha" <[email protected]>, >> > > > [email protected] >> > > > 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" <[email protected]> >> > > > > To: [email protected] >> > > > > Cc: "Liran Zelkha" <[email protected]>, [email protected] >> > > > > 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 ? > >> @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 suggest to use DTO [1] concept to solve this issue. We should not marshal entities and send them anywhere. [1] http://en.wikipedia.org/wiki/Data_transfer_object >> >> > >> > > > > 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" <[email protected]> >> > > > > > To: "Martin Perina" <[email protected]> >> > > > > > Cc: "Liran Zelkha" <[email protected]>, [email protected] >> > > > > > 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" <[email protected]> >> > > > > > > > To: "Tomas Jelinek" <[email protected]> >> > > > > > > > Cc: "Liran Zelkha" <[email protected]>, [email protected] >> > > > > > > > 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" <[email protected]> >> > > > > > > > > > To: [email protected] >> > > > > > > > > > Cc: "Tomas Jelinek" <[email protected]>, "Liran Zelkha" >> > > > > > > > > > <[email protected]> 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 >> > > > > > > > > > > [email protected] >> > > > > > > > > > > http://lists.ovirt.org/mailman/listinfo/devel >> > > > > > > > >> > > > > > > > _______________________________________________ >> > > > > > > > Devel mailing list >> > > > > > > > [email protected] >> > > > > > > > http://lists.ovirt.org/mailman/listinfo/devel >> > > > > > >> > > > > > _______________________________________________ >> > > > > > Devel mailing list >> > > > > > [email protected] >> > > > > > http://lists.ovirt.org/mailman/listinfo/devel >> > > > > >> > > > > _______________________________________________ >> > > > > Devel mailing list >> > > > > [email protected] >> > > > > http://lists.ovirt.org/mailman/listinfo/devel >> > >> > >> > _______________________________________________ > Devel mailing list > [email protected] > http://lists.ovirt.org/mailman/listinfo/devel _______________________________________________ Devel mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/devel
