It passed with " all-adapters" as well :) 2017-06-12 16:37 GMT+03:00 Romain Manni-Bucau <rmannibu...@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.za...@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 <rmannibu...@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 <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 > 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 < > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
