Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OGM-1229 ORM 5.2 upgrade #872

Merged
merged 22 commits into from Feb 6, 2018
Merged

OGM-1229 ORM 5.2 upgrade #872

merged 22 commits into from Feb 6, 2018

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Aug 30, 2017

Upgrade to ORM 5.2 and Search 5.8.

One thing that we might need to discuss is the removal of NoSQLQuery. It might cause some issues on the user side but relying on NativeQuery (which now has a good name - it's not called SQLQuery anymore) makes it far easier for the integration. If we do want to keep NoSQLQuery, we will need to add a QueryFactory facility to the ORM Session.

@gsmet gsmet force-pushed the orm-5.2-upgrade branch 3 times, most recently from 640dbd7 to 8d5884f Compare September 6, 2017 10:07
@Sanne
Copy link
Member

Sanne commented Sep 17, 2017

@gsmet the JTA failure on Neo4j should be solved with the ORM uprade right?

@gsmet
Copy link
Member Author

gsmet commented Sep 17, 2017

@Sanne yes, the JTA issue is fixed. But I have another one with the WF 11 upgrade.

@Sanne
Copy link
Member

Sanne commented Sep 18, 2017

Not urgent but for me to have a look it would be useful to rebase it first.

Let me know if I can help with the WF11 upgrade? I tried the current branch but it's attempting to load a non-existing org.hibernate:hibernate-orm-modules:zip:5.2.11.Final with classifier wildfly-10-dist so I guess we're not fully in synch

@gsmet
Copy link
Member Author

gsmet commented Sep 18, 2017

Yes, I upgraded to Final, then noticed the WF11 upgrade, then had to upgrade to Search 5.8 and so on :).

I'm working on fixing (hopefully) last issue.

Anyway, we shouldn't merge it for now, I think. Better apply it when we initialize 5.3.

@Sanne
Copy link
Member

Sanne commented Sep 18, 2017

I'm working on fixing (hopefully) last issue.

cool

Anyway, we shouldn't merge it for now, I think. Better apply it when we initialize 5.3.

Sure, I'd still like to have a peek though :) Also feel free to identify some improvements which could be extracted and merged already.

@gsmet
Copy link
Member Author

gsmet commented Sep 18, 2017

@Sanne rebased!

@gsmet gsmet force-pushed the orm-5.2-upgrade branch 2 times, most recently from 23d22d3 to b6ad0f3 Compare September 18, 2017 15:27
@Sanne
Copy link
Member

Sanne commented Sep 18, 2017

Assigning to @emmanuelbernard as it's just some maintenance, a trivial minor upgrade :D

@gsmet
Copy link
Member Author

gsmet commented Sep 18, 2017

Jenkins, retest this please

* @return a new set of entities
*/
private static Set<Class<?>> toRootEntities(ExtendedSearchIntegrator searchFactoryImplementor, Class<?>... selection) {
private static IndexedTypeSet toRootEntities(ExtendedSearchIntegrator extendedIntegrator, Class<?>... selection) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to let you know I noticed this, and yes this is going to be much better with the next round of the Search SPI

@gsmet
Copy link
Member Author

gsmet commented Oct 2, 2017

Tried to align the versions with WildFly 11 but I had an issue with Narayana: https://developer.jboss.org/message/976535#976535 .

Worst case scenario, we can keep an older version for the tests but let's see if we can figure it out with the help of the Narayana people.

@gsmet
Copy link
Member Author

gsmet commented Oct 2, 2017

The issue with Narayana was fixed thanks to Tom Jenkinson. I rebased the branch onto latest master.

@gsmet
Copy link
Member Author

gsmet commented Oct 10, 2017

Rebased and upgraded Search to 5.8.1.Final.

DavideD and others added 22 commits February 6, 2018 19:49
…onImpl

is now an EntityManager

Also removes a lot of copied code to be able to determine what is
strictly necessary. Note that this code will be reintroduced later (and
then in the end removed due thanks to some changes in ORM).

Note that this commit removes some APIs (NoSQLQuery and
NoSQLQueryImplementor) as they are not necessary anymore and requires us
to maintain more things.

We discussed it with Sanne and, considering the fact that ORM 5.2 breaks
a lot of things anyway, it would be considered acceptable.
This is due to the fact that Session and EntityManager are now a same
thing so Session has to follow JPA requirements.
…ntityManager

and it is created as an OGM session
…n class

It was already the case but as Session is also an EntityManager now, we
have more methods in it (and the code in ORM has more private/protected
methods we need to copy).

Note that the methods were deleted in a previous commit to know exactly
what was required.

They will be removed in a subsequent commit relying on the new
CustomLoaderFactory mechanism introduced in ORM.
…n() in a JTA environment anymore

A lot of our shared tests rely on this and we want to keep them working for Neo4j.
It was required for a moment in my journey and I think it's more future
proof to have it there and rely on ORM mechanisms.
Note that it is not an incompatibility in ORM: the method was introduced
during the 5.2.11 release cycle and the name changed before the release.
ORM used to initialize a TransactionImpl when initializing the
SessionImpl but it now only calls pulse() on the TransactionCoordinator.
Thus, the OperationCollector initialization must be moved there.
@DavideD
Copy link
Member

DavideD commented Feb 6, 2018

Jenkins, rebuild this please

@gsmet
Copy link
Member Author

gsmet commented Feb 6, 2018

@DavideD just force pushed a fix. I didn't have the infinispan-embedded project in my workspace so I didn't see the compilation issue with the new added test. Should be fine now.

@DavideD DavideD merged commit a97bb73 into hibernate:master Feb 6, 2018
@DavideD
Copy link
Member

DavideD commented Feb 6, 2018

Merged #872

Thanks

@gsmet
Copy link
Member Author

gsmet commented Feb 6, 2018

@DavideD thanks! Glad to see this one finally in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants