You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomee.apache.org by Svetlin Zarev <sv...@gmail.com> on 2017/06/12 08:13:57 UTC
TOMEE-2057 fix proposal
Hi Everyone, Romain,
As JIRA is down, I'm writing to the mailing list.
Recently I reported TOMEE-2057 -> modification to
entities done in the @BeforeCompletion callback were
not getting persisted. The root cause was that
eclipselink's synchronization was executed before
openejb's one. The same case works as expected
with OpenJPA.
Hence I want to propose the following fix for the
eclipselink's integration with tomee: [1] OpenJPA
does the same, but it's implemented inside OpenJPA.
[1] https://github.com/apache/tomee/pull/70
PS: Is there really need for reflection ? Why don't we add
dependency to OpenEJB-Core and remove the reflection ?
Best regards,
Svetlin
Re: TOMEE-2057 fix proposal
Posted by Romain Manni-Bucau <rm...@gmail.com>.
:), so looks like you did it ;). Thanks a lot! Really appreciated the
effort to test it.
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://blog-rmannibucau.rhcloud.com> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
<https://javaeefactory-rmannibucau.rhcloud.com>
2017-06-12 16:26 GMT+02:00 Svetlin Zarev <sv...@gmail.com>:
> It passed with " all-adapters" as well :)
>
> 2017-06-12 16:37 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
>
> > if you run it with the profile all-adapters from maven command line it
> will
> > run with all tomee flavors (embedded, webprofile, plus, plume) so plume
> run
> > will check with eclipselinks.
> >
> >
> > Romain Manni-Bucau
> > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > <https://blog-rmannibucau.rhcloud.com> | Old Blog
> > <http://rmannibucau.wordpress.com> | Github <https://github.com/
> > rmannibucau> |
> > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
> > <https://javaeefactory-rmannibucau.rhcloud.com>
> >
> > 2017-06-12 15:34 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail.
> com
> > >:
> >
> > > I've added Arquilian test in arquillian-tomee-webprofile-tests.
> > > It passes on my machine :) but I'm pretty sure it runs only with
> > > OpenJPA. How can I force it to run with eclipselink ?
> > >
> > > https://github.com/apache/tomee/pull/70
> > >
> > > 2017-06-12 12:04 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
> > >
> > > > 2017-06-12 11:02 GMT+02:00 Svetlin Zarev
> <svetlin.angelov.zarev@gmail.
> > > com
> > > > >:
> > > >
> > > > > OK, I'll update my PR.
> > > > >
> > > > > I want to write a test as well. In which project
> > > > > should I add it, so it's executed with both
> > > > > OpenJPA & Eclipse link ?
> > > > >
> > > > >
> > > > you have a few options here:
> > > >
> > > > 1. openejb-core
> > > > 2. add an example in examples/ (we use them as itest sometimes)
> > > > 3. arquillian/*tests
> > > >
> > > > The easiest is the example option probably but take the approach
> making
> > > you
> > > > comfortable. Only constraints are 1. don't break the whole build ;),
> 2.
> > > > ensure it is covered :)
> > > >
> > > >
> > > > >
> > > > > 2017-06-12 11:43 GMT+03:00 Romain Manni-Bucau <
> rmannibucau@gmail.com
> > >:
> > > > >
> > > > > > 2017-06-12 10:42 GMT+02:00 Svetlin Zarev
> > > <svetlin.angelov.zarev@gmail.
> > > > > com
> > > > > > >:
> > > > > >
> > > > > > > I didn't think of that, but still I prefer to do it via the
> > proper
> > > > > > > interface,
> > > > > > > because javax.transaction.Transaction does not expose the
> > > interposed
> > > > > > list.
> > > > > > > So it would be transaction-manager implementation dependent and
> > it
> > > > > might
> > > > > > > stop working if geronimo changes its impl.
> > > > > > >
> > > > > >
> > > > > > Hmm, this is true...and if openejb removes SystemInstance too so
> > > guess
> > > > it
> > > > > > is better to decrease the reflection. Also don't forget the test
> > > would
> > > > > > break if it changes
> > > > > > and we would fix it ;).
> > > > > >
> > > > > > Side note: we are committer on this geronimo component too so see
> > it
> > > as
> > > > > > part of tomee in term of risk.
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > But I can modify my PR to use txn if you insist.
> > > > > > >
> > > > > > > 2017-06-12 11:36 GMT+03:00 Romain Manni-Bucau <
> > > rmannibucau@gmail.com
> > > > >:
> > > > > > >
> > > > > > > > 2017-06-12 10:29 GMT+02:00 Svetlin Zarev
> > > > > <svetlin.angelov.zarev@gmail.
> > > > > > > com
> > > > > > > > >:
> > > > > > > >
> > > > > > > > > > why not relying on the
> > > > > > > > > default? javax.transaction.Transaction.class.cast(txn).
> > > > > > > > > registerSynchronization(listener
> > > > > > > > >
> > > > > > > > > Because it registers the transaction in the "regular"
> > > > > > synchronizations
> > > > > > > > > list. But
> > > > > > > > > if the synchronization is registered via the
> > > > > > > > > TransactionSynchronizationRegistry,
> > > > > > > > > the synchronization will be registered in the "interposed"
> > > > > > > > synchronization
> > > > > > > > > list.
> > > > > > > > >
> > > > > > > > > The synchronizations from the interposed list are always
> > > executed
> > > > > > after
> > > > > > > > the
> > > > > > > > > synchronizations from the regular list.
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Makes sense. Note you can just get
> > registerInterposedSynchronizat
> > > > ion
> > > > > > > from
> > > > > > > > the txn directly, it would make less reflection and do
> exactly
> > > the
> > > > > > same.
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > OpenEJB's SessionSynchronizationCoordinator is registered
> > into
> > > > the
> > > > > > > > > interposed list, so if we want for @BeforeCompletion to
> work
> > > > > > > > > correctly with eclipselink , then either openejb's
> > > > > > > > > SessionSynchronizationCoordinator
> > > > > > > > > must be registered in the regular list, or eclipselink's
> > > > > > > synchronization
> > > > > > > > > must be registered in the interposed list. I think that
> the
> > > > second
> > > > > > > > option
> > > > > > > > > is less invasive.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Your patch (adapted with the previous reflection enhancement
> > > > > probably)
> > > > > > > > looks good, just miss a test to ensure it works (surely in
> > > > > openejb-core
> > > > > > > > with application composer)
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > 2017-06-12 11:22 GMT+03:00 Romain Manni-Bucau <
> > > > > rmannibucau@gmail.com
> > > > > > >:
> > > > > > > > >
> > > > > > > > > > Hi Svetlin,
> > > > > > > > > >
> > > > > > > > > > why not relying on the
> > > > > > > > > > default? javax.transaction.Transaction.class.cast(txn).
> > > > > > > > > > registerSynchronization(listener)?
> > > > > > > > > > This is what your code does except it goes through the
> > > registry
> > > > > to
> > > > > > > find
> > > > > > > > > the
> > > > > > > > > > current transaction instead of using the one already
> passed
> > > and
> > > > > > bound
> > > > > > > > to
> > > > > > > > > > the entity manager - should lead to the same if the
> > > application
> > > > > > > doesnt
> > > > > > > > > > abuse of transactions.
> > > > > > > > > >
> > > > > > > > > > The reflection is here cause openejb-core depends on
> > > > > > > > > > openejb-jpa-integration so if you add openejb-core as a
> > > > > dependency
> > > > > > it
> > > > > > > > > > wouldn't build. This is done cause jpa-integration is
> added
> > > to
> > > > > > > > > applications
> > > > > > > > > > for the ones providing the jpa provider instead of using
> > the
> > > > > > > container
> > > > > > > > > one.
> > > > > > > > > >
> > > > > > > > > > Side note: would be good to ensure any PR has some
> test(s)
> > > when
> > > > > > > > possible
> > > > > > > > > > otherwise it is easy to break it before next release
> > without
> > > > even
> > > > > > > > > noticing
> > > > > > > > > > it.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Romain Manni-Bucau
> > > > > > > > > > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > > > > > > > > > <https://blog-rmannibucau.rhcloud.com> | Old Blog
> > > > > > > > > > <http://rmannibucau.wordpress.com> | Github <
> > > > https://github.com/
> > > > > > > > > > rmannibucau> |
> > > > > > > > > > LinkedIn <https://www.linkedin.com/in/rmannibucau> |
> > JavaEE
> > > > > > Factory
> > > > > > > > > > <https://javaeefactory-rmannibucau.rhcloud.com>
> > > > > > > > > >
> > > > > > > > > > 2017-06-12 10:13 GMT+02:00 Svetlin Zarev
> > > > > > > <svetlin.angelov.zarev@gmail.
> > > > > > > > > com
> > > > > > > > > > >:
> > > > > > > > > >
> > > > > > > > > > > Hi Everyone, Romain,
> > > > > > > > > > >
> > > > > > > > > > > As JIRA is down, I'm writing to the mailing list.
> > > > > > > > > > >
> > > > > > > > > > > Recently I reported TOMEE-2057 -> modification to
> > > > > > > > > > > entities done in the @BeforeCompletion callback were
> > > > > > > > > > > not getting persisted. The root cause was that
> > > > > > > > > > > eclipselink's synchronization was executed before
> > > > > > > > > > > openejb's one. The same case works as expected
> > > > > > > > > > > with OpenJPA.
> > > > > > > > > > >
> > > > > > > > > > > Hence I want to propose the following fix for the
> > > > > > > > > > > eclipselink's integration with tomee: [1] OpenJPA
> > > > > > > > > > > does the same, but it's implemented inside OpenJPA.
> > > > > > > > > > >
> > > > > > > > > > > [1] https://github.com/apache/tomee/pull/70
> > > > > > > > > > >
> > > > > > > > > > > PS: Is there really need for reflection ? Why don't we
> > add
> > > > > > > > > > > dependency to OpenEJB-Core and remove the reflection ?
> > > > > > > > > > >
> > > > > > > > > > > Best regards,
> > > > > > > > > > > Svetlin
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: TOMEE-2057 fix proposal
Posted by Svetlin Zarev <sv...@gmail.com>.
It passed with " all-adapters" as well :)
2017-06-12 16:37 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
> if you run it with the profile all-adapters from maven command line it will
> run with all tomee flavors (embedded, webprofile, plus, plume) so plume run
> will check with eclipselinks.
>
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> <https://blog-rmannibucau.rhcloud.com> | Old Blog
> <http://rmannibucau.wordpress.com> | Github <https://github.com/
> rmannibucau> |
> LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
> <https://javaeefactory-rmannibucau.rhcloud.com>
>
> 2017-06-12 15:34 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail.com
> >:
>
> > I've added Arquilian test in arquillian-tomee-webprofile-tests.
> > It passes on my machine :) but I'm pretty sure it runs only with
> > OpenJPA. How can I force it to run with eclipselink ?
> >
> > https://github.com/apache/tomee/pull/70
> >
> > 2017-06-12 12:04 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
> >
> > > 2017-06-12 11:02 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail.
> > com
> > > >:
> > >
> > > > OK, I'll update my PR.
> > > >
> > > > I want to write a test as well. In which project
> > > > should I add it, so it's executed with both
> > > > OpenJPA & Eclipse link ?
> > > >
> > > >
> > > you have a few options here:
> > >
> > > 1. openejb-core
> > > 2. add an example in examples/ (we use them as itest sometimes)
> > > 3. arquillian/*tests
> > >
> > > The easiest is the example option probably but take the approach making
> > you
> > > comfortable. Only constraints are 1. don't break the whole build ;), 2.
> > > ensure it is covered :)
> > >
> > >
> > > >
> > > > 2017-06-12 11:43 GMT+03:00 Romain Manni-Bucau <rmannibucau@gmail.com
> >:
> > > >
> > > > > 2017-06-12 10:42 GMT+02:00 Svetlin Zarev
> > <svetlin.angelov.zarev@gmail.
> > > > com
> > > > > >:
> > > > >
> > > > > > I didn't think of that, but still I prefer to do it via the
> proper
> > > > > > interface,
> > > > > > because javax.transaction.Transaction does not expose the
> > interposed
> > > > > list.
> > > > > > So it would be transaction-manager implementation dependent and
> it
> > > > might
> > > > > > stop working if geronimo changes its impl.
> > > > > >
> > > > >
> > > > > Hmm, this is true...and if openejb removes SystemInstance too so
> > guess
> > > it
> > > > > is better to decrease the reflection. Also don't forget the test
> > would
> > > > > break if it changes
> > > > > and we would fix it ;).
> > > > >
> > > > > Side note: we are committer on this geronimo component too so see
> it
> > as
> > > > > part of tomee in term of risk.
> > > > >
> > > > >
> > > > > >
> > > > > > But I can modify my PR to use txn if you insist.
> > > > > >
> > > > > > 2017-06-12 11:36 GMT+03:00 Romain Manni-Bucau <
> > rmannibucau@gmail.com
> > > >:
> > > > > >
> > > > > > > 2017-06-12 10:29 GMT+02:00 Svetlin Zarev
> > > > <svetlin.angelov.zarev@gmail.
> > > > > > com
> > > > > > > >:
> > > > > > >
> > > > > > > > > why not relying on the
> > > > > > > > default? javax.transaction.Transaction.class.cast(txn).
> > > > > > > > registerSynchronization(listener
> > > > > > > >
> > > > > > > > Because it registers the transaction in the "regular"
> > > > > synchronizations
> > > > > > > > list. But
> > > > > > > > if the synchronization is registered via the
> > > > > > > > TransactionSynchronizationRegistry,
> > > > > > > > the synchronization will be registered in the "interposed"
> > > > > > > synchronization
> > > > > > > > list.
> > > > > > > >
> > > > > > > > The synchronizations from the interposed list are always
> > executed
> > > > > after
> > > > > > > the
> > > > > > > > synchronizations from the regular list.
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Makes sense. Note you can just get
> registerInterposedSynchronizat
> > > ion
> > > > > > from
> > > > > > > the txn directly, it would make less reflection and do exactly
> > the
> > > > > same.
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > OpenEJB's SessionSynchronizationCoordinator is registered
> into
> > > the
> > > > > > > > interposed list, so if we want for @BeforeCompletion to work
> > > > > > > > correctly with eclipselink , then either openejb's
> > > > > > > > SessionSynchronizationCoordinator
> > > > > > > > must be registered in the regular list, or eclipselink's
> > > > > > synchronization
> > > > > > > > must be registered in the interposed list. I think that the
> > > second
> > > > > > > option
> > > > > > > > is less invasive.
> > > > > > > >
> > > > > > >
> > > > > > > Your patch (adapted with the previous reflection enhancement
> > > > probably)
> > > > > > > looks good, just miss a test to ensure it works (surely in
> > > > openejb-core
> > > > > > > with application composer)
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > 2017-06-12 11:22 GMT+03:00 Romain Manni-Bucau <
> > > > rmannibucau@gmail.com
> > > > > >:
> > > > > > > >
> > > > > > > > > Hi Svetlin,
> > > > > > > > >
> > > > > > > > > why not relying on the
> > > > > > > > > default? javax.transaction.Transaction.class.cast(txn).
> > > > > > > > > registerSynchronization(listener)?
> > > > > > > > > This is what your code does except it goes through the
> > registry
> > > > to
> > > > > > find
> > > > > > > > the
> > > > > > > > > current transaction instead of using the one already passed
> > and
> > > > > bound
> > > > > > > to
> > > > > > > > > the entity manager - should lead to the same if the
> > application
> > > > > > doesnt
> > > > > > > > > abuse of transactions.
> > > > > > > > >
> > > > > > > > > The reflection is here cause openejb-core depends on
> > > > > > > > > openejb-jpa-integration so if you add openejb-core as a
> > > > dependency
> > > > > it
> > > > > > > > > wouldn't build. This is done cause jpa-integration is added
> > to
> > > > > > > > applications
> > > > > > > > > for the ones providing the jpa provider instead of using
> the
> > > > > > container
> > > > > > > > one.
> > > > > > > > >
> > > > > > > > > Side note: would be good to ensure any PR has some test(s)
> > when
> > > > > > > possible
> > > > > > > > > otherwise it is easy to break it before next release
> without
> > > even
> > > > > > > > noticing
> > > > > > > > > it.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Romain Manni-Bucau
> > > > > > > > > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > > > > > > > > <https://blog-rmannibucau.rhcloud.com> | Old Blog
> > > > > > > > > <http://rmannibucau.wordpress.com> | Github <
> > > https://github.com/
> > > > > > > > > rmannibucau> |
> > > > > > > > > LinkedIn <https://www.linkedin.com/in/rmannibucau> |
> JavaEE
> > > > > Factory
> > > > > > > > > <https://javaeefactory-rmannibucau.rhcloud.com>
> > > > > > > > >
> > > > > > > > > 2017-06-12 10:13 GMT+02:00 Svetlin Zarev
> > > > > > <svetlin.angelov.zarev@gmail.
> > > > > > > > com
> > > > > > > > > >:
> > > > > > > > >
> > > > > > > > > > Hi Everyone, Romain,
> > > > > > > > > >
> > > > > > > > > > As JIRA is down, I'm writing to the mailing list.
> > > > > > > > > >
> > > > > > > > > > Recently I reported TOMEE-2057 -> modification to
> > > > > > > > > > entities done in the @BeforeCompletion callback were
> > > > > > > > > > not getting persisted. The root cause was that
> > > > > > > > > > eclipselink's synchronization was executed before
> > > > > > > > > > openejb's one. The same case works as expected
> > > > > > > > > > with OpenJPA.
> > > > > > > > > >
> > > > > > > > > > Hence I want to propose the following fix for the
> > > > > > > > > > eclipselink's integration with tomee: [1] OpenJPA
> > > > > > > > > > does the same, but it's implemented inside OpenJPA.
> > > > > > > > > >
> > > > > > > > > > [1] https://github.com/apache/tomee/pull/70
> > > > > > > > > >
> > > > > > > > > > PS: Is there really need for reflection ? Why don't we
> add
> > > > > > > > > > dependency to OpenEJB-Core and remove the reflection ?
> > > > > > > > > >
> > > > > > > > > > Best regards,
> > > > > > > > > > Svetlin
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: TOMEE-2057 fix proposal
Posted by Romain Manni-Bucau <rm...@gmail.com>.
if you run it with the profile all-adapters from maven command line it will
run with all tomee flavors (embedded, webprofile, plus, plume) so plume run
will check with eclipselinks.
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://blog-rmannibucau.rhcloud.com> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
<https://javaeefactory-rmannibucau.rhcloud.com>
2017-06-12 15:34 GMT+02:00 Svetlin Zarev <sv...@gmail.com>:
> I've added Arquilian test in arquillian-tomee-webprofile-tests.
> It passes on my machine :) but I'm pretty sure it runs only with
> OpenJPA. How can I force it to run with eclipselink ?
>
> https://github.com/apache/tomee/pull/70
>
> 2017-06-12 12:04 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
>
> > 2017-06-12 11:02 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail.
> com
> > >:
> >
> > > OK, I'll update my PR.
> > >
> > > I want to write a test as well. In which project
> > > should I add it, so it's executed with both
> > > OpenJPA & Eclipse link ?
> > >
> > >
> > you have a few options here:
> >
> > 1. openejb-core
> > 2. add an example in examples/ (we use them as itest sometimes)
> > 3. arquillian/*tests
> >
> > The easiest is the example option probably but take the approach making
> you
> > comfortable. Only constraints are 1. don't break the whole build ;), 2.
> > ensure it is covered :)
> >
> >
> > >
> > > 2017-06-12 11:43 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
> > >
> > > > 2017-06-12 10:42 GMT+02:00 Svetlin Zarev
> <svetlin.angelov.zarev@gmail.
> > > com
> > > > >:
> > > >
> > > > > I didn't think of that, but still I prefer to do it via the proper
> > > > > interface,
> > > > > because javax.transaction.Transaction does not expose the
> interposed
> > > > list.
> > > > > So it would be transaction-manager implementation dependent and it
> > > might
> > > > > stop working if geronimo changes its impl.
> > > > >
> > > >
> > > > Hmm, this is true...and if openejb removes SystemInstance too so
> guess
> > it
> > > > is better to decrease the reflection. Also don't forget the test
> would
> > > > break if it changes
> > > > and we would fix it ;).
> > > >
> > > > Side note: we are committer on this geronimo component too so see it
> as
> > > > part of tomee in term of risk.
> > > >
> > > >
> > > > >
> > > > > But I can modify my PR to use txn if you insist.
> > > > >
> > > > > 2017-06-12 11:36 GMT+03:00 Romain Manni-Bucau <
> rmannibucau@gmail.com
> > >:
> > > > >
> > > > > > 2017-06-12 10:29 GMT+02:00 Svetlin Zarev
> > > <svetlin.angelov.zarev@gmail.
> > > > > com
> > > > > > >:
> > > > > >
> > > > > > > > why not relying on the
> > > > > > > default? javax.transaction.Transaction.class.cast(txn).
> > > > > > > registerSynchronization(listener
> > > > > > >
> > > > > > > Because it registers the transaction in the "regular"
> > > > synchronizations
> > > > > > > list. But
> > > > > > > if the synchronization is registered via the
> > > > > > > TransactionSynchronizationRegistry,
> > > > > > > the synchronization will be registered in the "interposed"
> > > > > > synchronization
> > > > > > > list.
> > > > > > >
> > > > > > > The synchronizations from the interposed list are always
> executed
> > > > after
> > > > > > the
> > > > > > > synchronizations from the regular list.
> > > > > > >
> > > > > >
> > > > > >
> > > > > > Makes sense. Note you can just get registerInterposedSynchronizat
> > ion
> > > > > from
> > > > > > the txn directly, it would make less reflection and do exactly
> the
> > > > same.
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > OpenEJB's SessionSynchronizationCoordinator is registered into
> > the
> > > > > > > interposed list, so if we want for @BeforeCompletion to work
> > > > > > > correctly with eclipselink , then either openejb's
> > > > > > > SessionSynchronizationCoordinator
> > > > > > > must be registered in the regular list, or eclipselink's
> > > > > synchronization
> > > > > > > must be registered in the interposed list. I think that the
> > second
> > > > > > option
> > > > > > > is less invasive.
> > > > > > >
> > > > > >
> > > > > > Your patch (adapted with the previous reflection enhancement
> > > probably)
> > > > > > looks good, just miss a test to ensure it works (surely in
> > > openejb-core
> > > > > > with application composer)
> > > > > >
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > 2017-06-12 11:22 GMT+03:00 Romain Manni-Bucau <
> > > rmannibucau@gmail.com
> > > > >:
> > > > > > >
> > > > > > > > Hi Svetlin,
> > > > > > > >
> > > > > > > > why not relying on the
> > > > > > > > default? javax.transaction.Transaction.class.cast(txn).
> > > > > > > > registerSynchronization(listener)?
> > > > > > > > This is what your code does except it goes through the
> registry
> > > to
> > > > > find
> > > > > > > the
> > > > > > > > current transaction instead of using the one already passed
> and
> > > > bound
> > > > > > to
> > > > > > > > the entity manager - should lead to the same if the
> application
> > > > > doesnt
> > > > > > > > abuse of transactions.
> > > > > > > >
> > > > > > > > The reflection is here cause openejb-core depends on
> > > > > > > > openejb-jpa-integration so if you add openejb-core as a
> > > dependency
> > > > it
> > > > > > > > wouldn't build. This is done cause jpa-integration is added
> to
> > > > > > > applications
> > > > > > > > for the ones providing the jpa provider instead of using the
> > > > > container
> > > > > > > one.
> > > > > > > >
> > > > > > > > Side note: would be good to ensure any PR has some test(s)
> when
> > > > > > possible
> > > > > > > > otherwise it is easy to break it before next release without
> > even
> > > > > > > noticing
> > > > > > > > it.
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Romain Manni-Bucau
> > > > > > > > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > > > > > > > <https://blog-rmannibucau.rhcloud.com> | Old Blog
> > > > > > > > <http://rmannibucau.wordpress.com> | Github <
> > https://github.com/
> > > > > > > > rmannibucau> |
> > > > > > > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE
> > > > Factory
> > > > > > > > <https://javaeefactory-rmannibucau.rhcloud.com>
> > > > > > > >
> > > > > > > > 2017-06-12 10:13 GMT+02:00 Svetlin Zarev
> > > > > <svetlin.angelov.zarev@gmail.
> > > > > > > com
> > > > > > > > >:
> > > > > > > >
> > > > > > > > > Hi Everyone, Romain,
> > > > > > > > >
> > > > > > > > > As JIRA is down, I'm writing to the mailing list.
> > > > > > > > >
> > > > > > > > > Recently I reported TOMEE-2057 -> modification to
> > > > > > > > > entities done in the @BeforeCompletion callback were
> > > > > > > > > not getting persisted. The root cause was that
> > > > > > > > > eclipselink's synchronization was executed before
> > > > > > > > > openejb's one. The same case works as expected
> > > > > > > > > with OpenJPA.
> > > > > > > > >
> > > > > > > > > Hence I want to propose the following fix for the
> > > > > > > > > eclipselink's integration with tomee: [1] OpenJPA
> > > > > > > > > does the same, but it's implemented inside OpenJPA.
> > > > > > > > >
> > > > > > > > > [1] https://github.com/apache/tomee/pull/70
> > > > > > > > >
> > > > > > > > > PS: Is there really need for reflection ? Why don't we add
> > > > > > > > > dependency to OpenEJB-Core and remove the reflection ?
> > > > > > > > >
> > > > > > > > > Best regards,
> > > > > > > > > Svetlin
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: TOMEE-2057 fix proposal
Posted by Svetlin Zarev <sv...@gmail.com>.
I've added Arquilian test in arquillian-tomee-webprofile-tests.
It passes on my machine :) but I'm pretty sure it runs only with
OpenJPA. How can I force it to run with eclipselink ?
https://github.com/apache/tomee/pull/70
2017-06-12 12:04 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
> 2017-06-12 11:02 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail.com
> >:
>
> > OK, I'll update my PR.
> >
> > I want to write a test as well. In which project
> > should I add it, so it's executed with both
> > OpenJPA & Eclipse link ?
> >
> >
> you have a few options here:
>
> 1. openejb-core
> 2. add an example in examples/ (we use them as itest sometimes)
> 3. arquillian/*tests
>
> The easiest is the example option probably but take the approach making you
> comfortable. Only constraints are 1. don't break the whole build ;), 2.
> ensure it is covered :)
>
>
> >
> > 2017-06-12 11:43 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
> >
> > > 2017-06-12 10:42 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail.
> > com
> > > >:
> > >
> > > > I didn't think of that, but still I prefer to do it via the proper
> > > > interface,
> > > > because javax.transaction.Transaction does not expose the interposed
> > > list.
> > > > So it would be transaction-manager implementation dependent and it
> > might
> > > > stop working if geronimo changes its impl.
> > > >
> > >
> > > Hmm, this is true...and if openejb removes SystemInstance too so guess
> it
> > > is better to decrease the reflection. Also don't forget the test would
> > > break if it changes
> > > and we would fix it ;).
> > >
> > > Side note: we are committer on this geronimo component too so see it as
> > > part of tomee in term of risk.
> > >
> > >
> > > >
> > > > But I can modify my PR to use txn if you insist.
> > > >
> > > > 2017-06-12 11:36 GMT+03:00 Romain Manni-Bucau <rmannibucau@gmail.com
> >:
> > > >
> > > > > 2017-06-12 10:29 GMT+02:00 Svetlin Zarev
> > <svetlin.angelov.zarev@gmail.
> > > > com
> > > > > >:
> > > > >
> > > > > > > why not relying on the
> > > > > > default? javax.transaction.Transaction.class.cast(txn).
> > > > > > registerSynchronization(listener
> > > > > >
> > > > > > Because it registers the transaction in the "regular"
> > > synchronizations
> > > > > > list. But
> > > > > > if the synchronization is registered via the
> > > > > > TransactionSynchronizationRegistry,
> > > > > > the synchronization will be registered in the "interposed"
> > > > > synchronization
> > > > > > list.
> > > > > >
> > > > > > The synchronizations from the interposed list are always executed
> > > after
> > > > > the
> > > > > > synchronizations from the regular list.
> > > > > >
> > > > >
> > > > >
> > > > > Makes sense. Note you can just get registerInterposedSynchronizat
> ion
> > > > from
> > > > > the txn directly, it would make less reflection and do exactly the
> > > same.
> > > > >
> > > > >
> > > > > >
> > > > > > OpenEJB's SessionSynchronizationCoordinator is registered into
> the
> > > > > > interposed list, so if we want for @BeforeCompletion to work
> > > > > > correctly with eclipselink , then either openejb's
> > > > > > SessionSynchronizationCoordinator
> > > > > > must be registered in the regular list, or eclipselink's
> > > > synchronization
> > > > > > must be registered in the interposed list. I think that the
> second
> > > > > option
> > > > > > is less invasive.
> > > > > >
> > > > >
> > > > > Your patch (adapted with the previous reflection enhancement
> > probably)
> > > > > looks good, just miss a test to ensure it works (surely in
> > openejb-core
> > > > > with application composer)
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > 2017-06-12 11:22 GMT+03:00 Romain Manni-Bucau <
> > rmannibucau@gmail.com
> > > >:
> > > > > >
> > > > > > > Hi Svetlin,
> > > > > > >
> > > > > > > why not relying on the
> > > > > > > default? javax.transaction.Transaction.class.cast(txn).
> > > > > > > registerSynchronization(listener)?
> > > > > > > This is what your code does except it goes through the registry
> > to
> > > > find
> > > > > > the
> > > > > > > current transaction instead of using the one already passed and
> > > bound
> > > > > to
> > > > > > > the entity manager - should lead to the same if the application
> > > > doesnt
> > > > > > > abuse of transactions.
> > > > > > >
> > > > > > > The reflection is here cause openejb-core depends on
> > > > > > > openejb-jpa-integration so if you add openejb-core as a
> > dependency
> > > it
> > > > > > > wouldn't build. This is done cause jpa-integration is added to
> > > > > > applications
> > > > > > > for the ones providing the jpa provider instead of using the
> > > > container
> > > > > > one.
> > > > > > >
> > > > > > > Side note: would be good to ensure any PR has some test(s) when
> > > > > possible
> > > > > > > otherwise it is easy to break it before next release without
> even
> > > > > > noticing
> > > > > > > it.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Romain Manni-Bucau
> > > > > > > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > > > > > > <https://blog-rmannibucau.rhcloud.com> | Old Blog
> > > > > > > <http://rmannibucau.wordpress.com> | Github <
> https://github.com/
> > > > > > > rmannibucau> |
> > > > > > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE
> > > Factory
> > > > > > > <https://javaeefactory-rmannibucau.rhcloud.com>
> > > > > > >
> > > > > > > 2017-06-12 10:13 GMT+02:00 Svetlin Zarev
> > > > <svetlin.angelov.zarev@gmail.
> > > > > > com
> > > > > > > >:
> > > > > > >
> > > > > > > > Hi Everyone, Romain,
> > > > > > > >
> > > > > > > > As JIRA is down, I'm writing to the mailing list.
> > > > > > > >
> > > > > > > > Recently I reported TOMEE-2057 -> modification to
> > > > > > > > entities done in the @BeforeCompletion callback were
> > > > > > > > not getting persisted. The root cause was that
> > > > > > > > eclipselink's synchronization was executed before
> > > > > > > > openejb's one. The same case works as expected
> > > > > > > > with OpenJPA.
> > > > > > > >
> > > > > > > > Hence I want to propose the following fix for the
> > > > > > > > eclipselink's integration with tomee: [1] OpenJPA
> > > > > > > > does the same, but it's implemented inside OpenJPA.
> > > > > > > >
> > > > > > > > [1] https://github.com/apache/tomee/pull/70
> > > > > > > >
> > > > > > > > PS: Is there really need for reflection ? Why don't we add
> > > > > > > > dependency to OpenEJB-Core and remove the reflection ?
> > > > > > > >
> > > > > > > > Best regards,
> > > > > > > > Svetlin
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: TOMEE-2057 fix proposal
Posted by Romain Manni-Bucau <rm...@gmail.com>.
2017-06-12 11:02 GMT+02:00 Svetlin Zarev <sv...@gmail.com>:
> OK, I'll update my PR.
>
> I want to write a test as well. In which project
> should I add it, so it's executed with both
> OpenJPA & Eclipse link ?
>
>
you have a few options here:
1. openejb-core
2. add an example in examples/ (we use them as itest sometimes)
3. arquillian/*tests
The easiest is the example option probably but take the approach making you
comfortable. Only constraints are 1. don't break the whole build ;), 2.
ensure it is covered :)
>
> 2017-06-12 11:43 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
>
> > 2017-06-12 10:42 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail.
> com
> > >:
> >
> > > I didn't think of that, but still I prefer to do it via the proper
> > > interface,
> > > because javax.transaction.Transaction does not expose the interposed
> > list.
> > > So it would be transaction-manager implementation dependent and it
> might
> > > stop working if geronimo changes its impl.
> > >
> >
> > Hmm, this is true...and if openejb removes SystemInstance too so guess it
> > is better to decrease the reflection. Also don't forget the test would
> > break if it changes
> > and we would fix it ;).
> >
> > Side note: we are committer on this geronimo component too so see it as
> > part of tomee in term of risk.
> >
> >
> > >
> > > But I can modify my PR to use txn if you insist.
> > >
> > > 2017-06-12 11:36 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
> > >
> > > > 2017-06-12 10:29 GMT+02:00 Svetlin Zarev
> <svetlin.angelov.zarev@gmail.
> > > com
> > > > >:
> > > >
> > > > > > why not relying on the
> > > > > default? javax.transaction.Transaction.class.cast(txn).
> > > > > registerSynchronization(listener
> > > > >
> > > > > Because it registers the transaction in the "regular"
> > synchronizations
> > > > > list. But
> > > > > if the synchronization is registered via the
> > > > > TransactionSynchronizationRegistry,
> > > > > the synchronization will be registered in the "interposed"
> > > > synchronization
> > > > > list.
> > > > >
> > > > > The synchronizations from the interposed list are always executed
> > after
> > > > the
> > > > > synchronizations from the regular list.
> > > > >
> > > >
> > > >
> > > > Makes sense. Note you can just get registerInterposedSynchronization
> > > from
> > > > the txn directly, it would make less reflection and do exactly the
> > same.
> > > >
> > > >
> > > > >
> > > > > OpenEJB's SessionSynchronizationCoordinator is registered into the
> > > > > interposed list, so if we want for @BeforeCompletion to work
> > > > > correctly with eclipselink , then either openejb's
> > > > > SessionSynchronizationCoordinator
> > > > > must be registered in the regular list, or eclipselink's
> > > synchronization
> > > > > must be registered in the interposed list. I think that the second
> > > > option
> > > > > is less invasive.
> > > > >
> > > >
> > > > Your patch (adapted with the previous reflection enhancement
> probably)
> > > > looks good, just miss a test to ensure it works (surely in
> openejb-core
> > > > with application composer)
> > > >
> > > >
> > > > >
> > > > >
> > > > > 2017-06-12 11:22 GMT+03:00 Romain Manni-Bucau <
> rmannibucau@gmail.com
> > >:
> > > > >
> > > > > > Hi Svetlin,
> > > > > >
> > > > > > why not relying on the
> > > > > > default? javax.transaction.Transaction.class.cast(txn).
> > > > > > registerSynchronization(listener)?
> > > > > > This is what your code does except it goes through the registry
> to
> > > find
> > > > > the
> > > > > > current transaction instead of using the one already passed and
> > bound
> > > > to
> > > > > > the entity manager - should lead to the same if the application
> > > doesnt
> > > > > > abuse of transactions.
> > > > > >
> > > > > > The reflection is here cause openejb-core depends on
> > > > > > openejb-jpa-integration so if you add openejb-core as a
> dependency
> > it
> > > > > > wouldn't build. This is done cause jpa-integration is added to
> > > > > applications
> > > > > > for the ones providing the jpa provider instead of using the
> > > container
> > > > > one.
> > > > > >
> > > > > > Side note: would be good to ensure any PR has some test(s) when
> > > > possible
> > > > > > otherwise it is easy to break it before next release without even
> > > > > noticing
> > > > > > it.
> > > > > >
> > > > > >
> > > > > >
> > > > > > Romain Manni-Bucau
> > > > > > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > > > > > <https://blog-rmannibucau.rhcloud.com> | Old Blog
> > > > > > <http://rmannibucau.wordpress.com> | Github <https://github.com/
> > > > > > rmannibucau> |
> > > > > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE
> > Factory
> > > > > > <https://javaeefactory-rmannibucau.rhcloud.com>
> > > > > >
> > > > > > 2017-06-12 10:13 GMT+02:00 Svetlin Zarev
> > > <svetlin.angelov.zarev@gmail.
> > > > > com
> > > > > > >:
> > > > > >
> > > > > > > Hi Everyone, Romain,
> > > > > > >
> > > > > > > As JIRA is down, I'm writing to the mailing list.
> > > > > > >
> > > > > > > Recently I reported TOMEE-2057 -> modification to
> > > > > > > entities done in the @BeforeCompletion callback were
> > > > > > > not getting persisted. The root cause was that
> > > > > > > eclipselink's synchronization was executed before
> > > > > > > openejb's one. The same case works as expected
> > > > > > > with OpenJPA.
> > > > > > >
> > > > > > > Hence I want to propose the following fix for the
> > > > > > > eclipselink's integration with tomee: [1] OpenJPA
> > > > > > > does the same, but it's implemented inside OpenJPA.
> > > > > > >
> > > > > > > [1] https://github.com/apache/tomee/pull/70
> > > > > > >
> > > > > > > PS: Is there really need for reflection ? Why don't we add
> > > > > > > dependency to OpenEJB-Core and remove the reflection ?
> > > > > > >
> > > > > > > Best regards,
> > > > > > > Svetlin
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: TOMEE-2057 fix proposal
Posted by Svetlin Zarev <sv...@gmail.com>.
OK, I'll update my PR.
I want to write a test as well. In which project
should I add it, so it's executed with both
OpenJPA & Eclipse link ?
2017-06-12 11:43 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
> 2017-06-12 10:42 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail.com
> >:
>
> > I didn't think of that, but still I prefer to do it via the proper
> > interface,
> > because javax.transaction.Transaction does not expose the interposed
> list.
> > So it would be transaction-manager implementation dependent and it might
> > stop working if geronimo changes its impl.
> >
>
> Hmm, this is true...and if openejb removes SystemInstance too so guess it
> is better to decrease the reflection. Also don't forget the test would
> break if it changes
> and we would fix it ;).
>
> Side note: we are committer on this geronimo component too so see it as
> part of tomee in term of risk.
>
>
> >
> > But I can modify my PR to use txn if you insist.
> >
> > 2017-06-12 11:36 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
> >
> > > 2017-06-12 10:29 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail.
> > com
> > > >:
> > >
> > > > > why not relying on the
> > > > default? javax.transaction.Transaction.class.cast(txn).
> > > > registerSynchronization(listener
> > > >
> > > > Because it registers the transaction in the "regular"
> synchronizations
> > > > list. But
> > > > if the synchronization is registered via the
> > > > TransactionSynchronizationRegistry,
> > > > the synchronization will be registered in the "interposed"
> > > synchronization
> > > > list.
> > > >
> > > > The synchronizations from the interposed list are always executed
> after
> > > the
> > > > synchronizations from the regular list.
> > > >
> > >
> > >
> > > Makes sense. Note you can just get registerInterposedSynchronization
> > from
> > > the txn directly, it would make less reflection and do exactly the
> same.
> > >
> > >
> > > >
> > > > OpenEJB's SessionSynchronizationCoordinator is registered into the
> > > > interposed list, so if we want for @BeforeCompletion to work
> > > > correctly with eclipselink , then either openejb's
> > > > SessionSynchronizationCoordinator
> > > > must be registered in the regular list, or eclipselink's
> > synchronization
> > > > must be registered in the interposed list. I think that the second
> > > option
> > > > is less invasive.
> > > >
> > >
> > > Your patch (adapted with the previous reflection enhancement probably)
> > > looks good, just miss a test to ensure it works (surely in openejb-core
> > > with application composer)
> > >
> > >
> > > >
> > > >
> > > > 2017-06-12 11:22 GMT+03:00 Romain Manni-Bucau <rmannibucau@gmail.com
> >:
> > > >
> > > > > Hi Svetlin,
> > > > >
> > > > > why not relying on the
> > > > > default? javax.transaction.Transaction.class.cast(txn).
> > > > > registerSynchronization(listener)?
> > > > > This is what your code does except it goes through the registry to
> > find
> > > > the
> > > > > current transaction instead of using the one already passed and
> bound
> > > to
> > > > > the entity manager - should lead to the same if the application
> > doesnt
> > > > > abuse of transactions.
> > > > >
> > > > > The reflection is here cause openejb-core depends on
> > > > > openejb-jpa-integration so if you add openejb-core as a dependency
> it
> > > > > wouldn't build. This is done cause jpa-integration is added to
> > > > applications
> > > > > for the ones providing the jpa provider instead of using the
> > container
> > > > one.
> > > > >
> > > > > Side note: would be good to ensure any PR has some test(s) when
> > > possible
> > > > > otherwise it is easy to break it before next release without even
> > > > noticing
> > > > > it.
> > > > >
> > > > >
> > > > >
> > > > > Romain Manni-Bucau
> > > > > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > > > > <https://blog-rmannibucau.rhcloud.com> | Old Blog
> > > > > <http://rmannibucau.wordpress.com> | Github <https://github.com/
> > > > > rmannibucau> |
> > > > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE
> Factory
> > > > > <https://javaeefactory-rmannibucau.rhcloud.com>
> > > > >
> > > > > 2017-06-12 10:13 GMT+02:00 Svetlin Zarev
> > <svetlin.angelov.zarev@gmail.
> > > > com
> > > > > >:
> > > > >
> > > > > > Hi Everyone, Romain,
> > > > > >
> > > > > > As JIRA is down, I'm writing to the mailing list.
> > > > > >
> > > > > > Recently I reported TOMEE-2057 -> modification to
> > > > > > entities done in the @BeforeCompletion callback were
> > > > > > not getting persisted. The root cause was that
> > > > > > eclipselink's synchronization was executed before
> > > > > > openejb's one. The same case works as expected
> > > > > > with OpenJPA.
> > > > > >
> > > > > > Hence I want to propose the following fix for the
> > > > > > eclipselink's integration with tomee: [1] OpenJPA
> > > > > > does the same, but it's implemented inside OpenJPA.
> > > > > >
> > > > > > [1] https://github.com/apache/tomee/pull/70
> > > > > >
> > > > > > PS: Is there really need for reflection ? Why don't we add
> > > > > > dependency to OpenEJB-Core and remove the reflection ?
> > > > > >
> > > > > > Best regards,
> > > > > > Svetlin
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: TOMEE-2057 fix proposal
Posted by Romain Manni-Bucau <rm...@gmail.com>.
2017-06-12 10:42 GMT+02:00 Svetlin Zarev <sv...@gmail.com>:
> I didn't think of that, but still I prefer to do it via the proper
> interface,
> because javax.transaction.Transaction does not expose the interposed list.
> So it would be transaction-manager implementation dependent and it might
> stop working if geronimo changes its impl.
>
Hmm, this is true...and if openejb removes SystemInstance too so guess it
is better to decrease the reflection. Also don't forget the test would
break if it changes
and we would fix it ;).
Side note: we are committer on this geronimo component too so see it as
part of tomee in term of risk.
>
> But I can modify my PR to use txn if you insist.
>
> 2017-06-12 11:36 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
>
> > 2017-06-12 10:29 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail.
> com
> > >:
> >
> > > > why not relying on the
> > > default? javax.transaction.Transaction.class.cast(txn).
> > > registerSynchronization(listener
> > >
> > > Because it registers the transaction in the "regular" synchronizations
> > > list. But
> > > if the synchronization is registered via the
> > > TransactionSynchronizationRegistry,
> > > the synchronization will be registered in the "interposed"
> > synchronization
> > > list.
> > >
> > > The synchronizations from the interposed list are always executed after
> > the
> > > synchronizations from the regular list.
> > >
> >
> >
> > Makes sense. Note you can just get registerInterposedSynchronization
> from
> > the txn directly, it would make less reflection and do exactly the same.
> >
> >
> > >
> > > OpenEJB's SessionSynchronizationCoordinator is registered into the
> > > interposed list, so if we want for @BeforeCompletion to work
> > > correctly with eclipselink , then either openejb's
> > > SessionSynchronizationCoordinator
> > > must be registered in the regular list, or eclipselink's
> synchronization
> > > must be registered in the interposed list. I think that the second
> > option
> > > is less invasive.
> > >
> >
> > Your patch (adapted with the previous reflection enhancement probably)
> > looks good, just miss a test to ensure it works (surely in openejb-core
> > with application composer)
> >
> >
> > >
> > >
> > > 2017-06-12 11:22 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
> > >
> > > > Hi Svetlin,
> > > >
> > > > why not relying on the
> > > > default? javax.transaction.Transaction.class.cast(txn).
> > > > registerSynchronization(listener)?
> > > > This is what your code does except it goes through the registry to
> find
> > > the
> > > > current transaction instead of using the one already passed and bound
> > to
> > > > the entity manager - should lead to the same if the application
> doesnt
> > > > abuse of transactions.
> > > >
> > > > The reflection is here cause openejb-core depends on
> > > > openejb-jpa-integration so if you add openejb-core as a dependency it
> > > > wouldn't build. This is done cause jpa-integration is added to
> > > applications
> > > > for the ones providing the jpa provider instead of using the
> container
> > > one.
> > > >
> > > > Side note: would be good to ensure any PR has some test(s) when
> > possible
> > > > otherwise it is easy to break it before next release without even
> > > noticing
> > > > it.
> > > >
> > > >
> > > >
> > > > Romain Manni-Bucau
> > > > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > > > <https://blog-rmannibucau.rhcloud.com> | Old Blog
> > > > <http://rmannibucau.wordpress.com> | Github <https://github.com/
> > > > rmannibucau> |
> > > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
> > > > <https://javaeefactory-rmannibucau.rhcloud.com>
> > > >
> > > > 2017-06-12 10:13 GMT+02:00 Svetlin Zarev
> <svetlin.angelov.zarev@gmail.
> > > com
> > > > >:
> > > >
> > > > > Hi Everyone, Romain,
> > > > >
> > > > > As JIRA is down, I'm writing to the mailing list.
> > > > >
> > > > > Recently I reported TOMEE-2057 -> modification to
> > > > > entities done in the @BeforeCompletion callback were
> > > > > not getting persisted. The root cause was that
> > > > > eclipselink's synchronization was executed before
> > > > > openejb's one. The same case works as expected
> > > > > with OpenJPA.
> > > > >
> > > > > Hence I want to propose the following fix for the
> > > > > eclipselink's integration with tomee: [1] OpenJPA
> > > > > does the same, but it's implemented inside OpenJPA.
> > > > >
> > > > > [1] https://github.com/apache/tomee/pull/70
> > > > >
> > > > > PS: Is there really need for reflection ? Why don't we add
> > > > > dependency to OpenEJB-Core and remove the reflection ?
> > > > >
> > > > > Best regards,
> > > > > Svetlin
> > > > >
> > > >
> > >
> >
>
Re: TOMEE-2057 fix proposal
Posted by Svetlin Zarev <sv...@gmail.com>.
I didn't think of that, but still I prefer to do it via the proper
interface,
because javax.transaction.Transaction does not expose the interposed list.
So it would be transaction-manager implementation dependent and it might
stop working if geronimo changes its impl.
But I can modify my PR to use txn if you insist.
2017-06-12 11:36 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
> 2017-06-12 10:29 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail.com
> >:
>
> > > why not relying on the
> > default? javax.transaction.Transaction.class.cast(txn).
> > registerSynchronization(listener
> >
> > Because it registers the transaction in the "regular" synchronizations
> > list. But
> > if the synchronization is registered via the
> > TransactionSynchronizationRegistry,
> > the synchronization will be registered in the "interposed"
> synchronization
> > list.
> >
> > The synchronizations from the interposed list are always executed after
> the
> > synchronizations from the regular list.
> >
>
>
> Makes sense. Note you can just get registerInterposedSynchronization from
> the txn directly, it would make less reflection and do exactly the same.
>
>
> >
> > OpenEJB's SessionSynchronizationCoordinator is registered into the
> > interposed list, so if we want for @BeforeCompletion to work
> > correctly with eclipselink , then either openejb's
> > SessionSynchronizationCoordinator
> > must be registered in the regular list, or eclipselink's synchronization
> > must be registered in the interposed list. I think that the second
> option
> > is less invasive.
> >
>
> Your patch (adapted with the previous reflection enhancement probably)
> looks good, just miss a test to ensure it works (surely in openejb-core
> with application composer)
>
>
> >
> >
> > 2017-06-12 11:22 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
> >
> > > Hi Svetlin,
> > >
> > > why not relying on the
> > > default? javax.transaction.Transaction.class.cast(txn).
> > > registerSynchronization(listener)?
> > > This is what your code does except it goes through the registry to find
> > the
> > > current transaction instead of using the one already passed and bound
> to
> > > the entity manager - should lead to the same if the application doesnt
> > > abuse of transactions.
> > >
> > > The reflection is here cause openejb-core depends on
> > > openejb-jpa-integration so if you add openejb-core as a dependency it
> > > wouldn't build. This is done cause jpa-integration is added to
> > applications
> > > for the ones providing the jpa provider instead of using the container
> > one.
> > >
> > > Side note: would be good to ensure any PR has some test(s) when
> possible
> > > otherwise it is easy to break it before next release without even
> > noticing
> > > it.
> > >
> > >
> > >
> > > Romain Manni-Bucau
> > > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > > <https://blog-rmannibucau.rhcloud.com> | Old Blog
> > > <http://rmannibucau.wordpress.com> | Github <https://github.com/
> > > rmannibucau> |
> > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
> > > <https://javaeefactory-rmannibucau.rhcloud.com>
> > >
> > > 2017-06-12 10:13 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail.
> > com
> > > >:
> > >
> > > > Hi Everyone, Romain,
> > > >
> > > > As JIRA is down, I'm writing to the mailing list.
> > > >
> > > > Recently I reported TOMEE-2057 -> modification to
> > > > entities done in the @BeforeCompletion callback were
> > > > not getting persisted. The root cause was that
> > > > eclipselink's synchronization was executed before
> > > > openejb's one. The same case works as expected
> > > > with OpenJPA.
> > > >
> > > > Hence I want to propose the following fix for the
> > > > eclipselink's integration with tomee: [1] OpenJPA
> > > > does the same, but it's implemented inside OpenJPA.
> > > >
> > > > [1] https://github.com/apache/tomee/pull/70
> > > >
> > > > PS: Is there really need for reflection ? Why don't we add
> > > > dependency to OpenEJB-Core and remove the reflection ?
> > > >
> > > > Best regards,
> > > > Svetlin
> > > >
> > >
> >
>
Re: TOMEE-2057 fix proposal
Posted by Romain Manni-Bucau <rm...@gmail.com>.
2017-06-12 10:29 GMT+02:00 Svetlin Zarev <sv...@gmail.com>:
> > why not relying on the
> default? javax.transaction.Transaction.class.cast(txn).
> registerSynchronization(listener
>
> Because it registers the transaction in the "regular" synchronizations
> list. But
> if the synchronization is registered via the
> TransactionSynchronizationRegistry,
> the synchronization will be registered in the "interposed" synchronization
> list.
>
> The synchronizations from the interposed list are always executed after the
> synchronizations from the regular list.
>
Makes sense. Note you can just get registerInterposedSynchronization from
the txn directly, it would make less reflection and do exactly the same.
>
> OpenEJB's SessionSynchronizationCoordinator is registered into the
> interposed list, so if we want for @BeforeCompletion to work
> correctly with eclipselink , then either openejb's
> SessionSynchronizationCoordinator
> must be registered in the regular list, or eclipselink's synchronization
> must be registered in the interposed list. I think that the second option
> is less invasive.
>
Your patch (adapted with the previous reflection enhancement probably)
looks good, just miss a test to ensure it works (surely in openejb-core
with application composer)
>
>
> 2017-06-12 11:22 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
>
> > Hi Svetlin,
> >
> > why not relying on the
> > default? javax.transaction.Transaction.class.cast(txn).
> > registerSynchronization(listener)?
> > This is what your code does except it goes through the registry to find
> the
> > current transaction instead of using the one already passed and bound to
> > the entity manager - should lead to the same if the application doesnt
> > abuse of transactions.
> >
> > The reflection is here cause openejb-core depends on
> > openejb-jpa-integration so if you add openejb-core as a dependency it
> > wouldn't build. This is done cause jpa-integration is added to
> applications
> > for the ones providing the jpa provider instead of using the container
> one.
> >
> > Side note: would be good to ensure any PR has some test(s) when possible
> > otherwise it is easy to break it before next release without even
> noticing
> > it.
> >
> >
> >
> > Romain Manni-Bucau
> > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > <https://blog-rmannibucau.rhcloud.com> | Old Blog
> > <http://rmannibucau.wordpress.com> | Github <https://github.com/
> > rmannibucau> |
> > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
> > <https://javaeefactory-rmannibucau.rhcloud.com>
> >
> > 2017-06-12 10:13 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail.
> com
> > >:
> >
> > > Hi Everyone, Romain,
> > >
> > > As JIRA is down, I'm writing to the mailing list.
> > >
> > > Recently I reported TOMEE-2057 -> modification to
> > > entities done in the @BeforeCompletion callback were
> > > not getting persisted. The root cause was that
> > > eclipselink's synchronization was executed before
> > > openejb's one. The same case works as expected
> > > with OpenJPA.
> > >
> > > Hence I want to propose the following fix for the
> > > eclipselink's integration with tomee: [1] OpenJPA
> > > does the same, but it's implemented inside OpenJPA.
> > >
> > > [1] https://github.com/apache/tomee/pull/70
> > >
> > > PS: Is there really need for reflection ? Why don't we add
> > > dependency to OpenEJB-Core and remove the reflection ?
> > >
> > > Best regards,
> > > Svetlin
> > >
> >
>
Re: TOMEE-2057 fix proposal
Posted by Svetlin Zarev <sv...@gmail.com>.
> why not relying on the
default? javax.transaction.Transaction.class.cast(txn).
registerSynchronization(listener
Because it registers the transaction in the "regular" synchronizations
list. But
if the synchronization is registered via the
TransactionSynchronizationRegistry,
the synchronization will be registered in the "interposed" synchronization
list.
The synchronizations from the interposed list are always executed after the
synchronizations from the regular list.
OpenEJB's SessionSynchronizationCoordinator is registered into the
interposed list, so if we want for @BeforeCompletion to work
correctly with eclipselink , then either openejb's
SessionSynchronizationCoordinator
must be registered in the regular list, or eclipselink's synchronization
must be registered in the interposed list. I think that the second option
is less invasive.
2017-06-12 11:22 GMT+03:00 Romain Manni-Bucau <rm...@gmail.com>:
> Hi Svetlin,
>
> why not relying on the
> default? javax.transaction.Transaction.class.cast(txn).
> registerSynchronization(listener)?
> This is what your code does except it goes through the registry to find the
> current transaction instead of using the one already passed and bound to
> the entity manager - should lead to the same if the application doesnt
> abuse of transactions.
>
> The reflection is here cause openejb-core depends on
> openejb-jpa-integration so if you add openejb-core as a dependency it
> wouldn't build. This is done cause jpa-integration is added to applications
> for the ones providing the jpa provider instead of using the container one.
>
> Side note: would be good to ensure any PR has some test(s) when possible
> otherwise it is easy to break it before next release without even noticing
> it.
>
>
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> <https://blog-rmannibucau.rhcloud.com> | Old Blog
> <http://rmannibucau.wordpress.com> | Github <https://github.com/
> rmannibucau> |
> LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
> <https://javaeefactory-rmannibucau.rhcloud.com>
>
> 2017-06-12 10:13 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail.com
> >:
>
> > Hi Everyone, Romain,
> >
> > As JIRA is down, I'm writing to the mailing list.
> >
> > Recently I reported TOMEE-2057 -> modification to
> > entities done in the @BeforeCompletion callback were
> > not getting persisted. The root cause was that
> > eclipselink's synchronization was executed before
> > openejb's one. The same case works as expected
> > with OpenJPA.
> >
> > Hence I want to propose the following fix for the
> > eclipselink's integration with tomee: [1] OpenJPA
> > does the same, but it's implemented inside OpenJPA.
> >
> > [1] https://github.com/apache/tomee/pull/70
> >
> > PS: Is there really need for reflection ? Why don't we add
> > dependency to OpenEJB-Core and remove the reflection ?
> >
> > Best regards,
> > Svetlin
> >
>
Re: TOMEE-2057 fix proposal
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hi Svetlin,
why not relying on the
default? javax.transaction.Transaction.class.cast(txn).registerSynchronization(listener)?
This is what your code does except it goes through the registry to find the
current transaction instead of using the one already passed and bound to
the entity manager - should lead to the same if the application doesnt
abuse of transactions.
The reflection is here cause openejb-core depends on
openejb-jpa-integration so if you add openejb-core as a dependency it
wouldn't build. This is done cause jpa-integration is added to applications
for the ones providing the jpa provider instead of using the container one.
Side note: would be good to ensure any PR has some test(s) when possible
otherwise it is easy to break it before next release without even noticing
it.
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://blog-rmannibucau.rhcloud.com> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
<https://javaeefactory-rmannibucau.rhcloud.com>
2017-06-12 10:13 GMT+02:00 Svetlin Zarev <sv...@gmail.com>:
> Hi Everyone, Romain,
>
> As JIRA is down, I'm writing to the mailing list.
>
> Recently I reported TOMEE-2057 -> modification to
> entities done in the @BeforeCompletion callback were
> not getting persisted. The root cause was that
> eclipselink's synchronization was executed before
> openejb's one. The same case works as expected
> with OpenJPA.
>
> Hence I want to propose the following fix for the
> eclipselink's integration with tomee: [1] OpenJPA
> does the same, but it's implemented inside OpenJPA.
>
> [1] https://github.com/apache/tomee/pull/70
>
> PS: Is there really need for reflection ? Why don't we add
> dependency to OpenEJB-Core and remove the reflection ?
>
> Best regards,
> Svetlin
>