2017-06-12 10:42 GMT+02:00 Svetlin Zarev <[email protected]>:
> 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 <[email protected]>: > > > 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 <[email protected]>: > > > > > > > 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 > > > > > > > > > > > > > > >
