2017-06-12 11:02 GMT+02:00 Svetlin Zarev <svetlin.angelov.za...@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 <rmannibu...@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 <rmannibu...@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 < > rmannibu...@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 > > > > > > > > > > > > > > > > > > > > > > > > > > > >
