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

Tue Jun 2 13:19:42 UTC 2015

On Tue, Jun 2, 2015 at 3:10 PM, Oved Ourfali <ovedo at redhat.com> wrote:
>
>
> ----- Original Message -----
>> From: "Tomas Jelinek" <tjelinek at redhat.com>
>> To: awels at redhat.com
>> Cc: "Liran Zelkha" <lzelkha at redhat.com>, "Oved Ourfali" <ovedo at redhat.com>, devel at 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 at redhat.com>
>> > To: "Liran Zelkha" <lzelkha at redhat.com>
>> > Cc: "Tomas Jelinek" <tjelinek at redhat.com>, "Oved Ourfali"
>> > <ovedo at redhat.com>, devel at 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 at redhat.com>
>> > > > To: "Oved Ourfali" <ovedo at redhat.com>
>> > > > Cc: awels at redhat.com, "Liran Zelkha" <lzelkha at redhat.com>,
>> > > > devel at 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 at redhat.com>
>> > > > > To: awels at redhat.com
>> > > > > Cc: "Liran Zelkha" <lzelkha at redhat.com>, devel at 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 ?
>
>> @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" <awels at redhat.com>
>> > > > > > To: "Martin Perina" <mperina at redhat.com>
>> > > > > > Cc: "Liran Zelkha" <lzelkha at redhat.com>, devel at 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 at redhat.com>
>> > > > > > > > To: "Tomas Jelinek" <tjelinek at redhat.com>
>> > > > > > > > Cc: "Liran Zelkha" <lzelkha at redhat.com>, devel at 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 at redhat.com>
>> > > > > > > > > > To: devel at ovirt.org
>> > > > > > > > > > Cc: "Tomas Jelinek" <tjelinek at redhat.com>, "Liran Zelkha"
>> > > > > > > > > > <lzelkha at redhat.com> 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 at ovirt.org
>> > > > > > > > > > > http://lists.ovirt.org/mailman/listinfo/devel
>> > > > > > > >
>> > > > > > > > _______________________________________________
>> > > > > > > > Devel mailing list
>> > > > > > > > Devel at ovirt.org
>> > > > > > > > http://lists.ovirt.org/mailman/listinfo/devel
>> > > > > >
>> > > > > > _______________________________________________
>> > > > > > Devel mailing list
>> > > > > > Devel at ovirt.org
>> > > > > > http://lists.ovirt.org/mailman/listinfo/devel
>> > > > >
>> > > > > _______________________________________________
>> > > > > Devel mailing list
>> > > > > Devel at ovirt.org
>> > > > > http://lists.ovirt.org/mailman/listinfo/devel
>> >
>> >
>>
> _______________________________________________
> Devel mailing list
> Devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/devel