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-770 ORM 5 #571

Closed
wants to merge 32 commits into from
Closed

OGM-770 ORM 5 #571

wants to merge 32 commits into from

Conversation

DavideD
Copy link
Member

@DavideD DavideD commented Jul 30, 2015

https://hibernate.atlassian.net/browse/OGM-770

Rebase @gunnarmorling pull request (#562) to the latest changes and update ORM to CR3.

The integration test are not working at the moment because WildFly is still on ORM CR2

gunnarmorling and others added 26 commits July 31, 2015 08:41
* Fixing JavaDoc errors
* Enabling doc and dist build on JDK 9 as this is working now
* Updating exclusion of JConsole dependency
@gunnarmorling
Copy link
Member

@DavideD When upgrading the WF directory to ORM 5 CR 3 (I will provide a temp. fix for that until we can go to WF 10 Beta1), all integration tests pass except two in Neo4j:

  • Neo4jResourceLocalModuleMemberRegistrationIT#shouldFindPersistedMemberByIdWithQuery
  • Neo4jResourceLocalModuleMemberRegistrationIT#shouldFindPersistedMemberById()

Their commit fails, but I think the actual error happens before, causing the TX to be closed when trying to commit. Does this ring a bell?

@gunnarmorling
Copy link
Member

@DavideD Any success with analyzing the remaining test failures?

@DavideD
Copy link
Member Author

DavideD commented Aug 1, 2015

Yes, it works now. But I haven't sent a pull request yet.
I cannot do that now. I will send a pull request in the evening.
On 1 Aug 2015 10:36, "Gunnar Morling" notifications@github.com wrote:

@DavideD https://github.com/DavideD Any success with analyzing the
remaining test failures?


Reply to this email directly or view it on GitHub
#571 (comment)
.

@gunnarmorling
Copy link
Member

gunnarmorling commented Aug 1, 2015 via email

@DavideD
Copy link
Member Author

DavideD commented Aug 2, 2015

I've addded some commits to the existing pull request.
I've updated the Datastore so the the logic to select the correct TransactionCoordinatorBuilder is inside the BaseDatastoreProvider but can be overridden by specific datastore when the need arise.
It also allow the removal of the experimental method in the interface.

@gunnarmorling
Copy link
Member

Thanks, @DavideD. Rebased and applied.

The two TX coordinators look good. I've moved back the decision logic to TransactionCoordinatorBuilder, though I think it's better located there as a) not all providers necessarily extend BaseDatastoreProvider and b) implementors really should not be confronted with "JDBC"-named APIs. Hope that's alright, let me know in case you see any remaining issues.

@DavideD
Copy link
Member Author

DavideD commented Aug 3, 2015

I prefer my proposal, the builder received by the datastore is consistent with the real context and it removes the allowemulation method from the interface. the JDBC name in the API could have been easily renamed. The datastore provider still needs to provide a TransactionCoordinatorBuilder so I don't see why not to delegate to it the creation.

Anyway, I don';t mind to leave like it as it is now. We will probably need to change it anyway when/if we will fix https://hibernate.atlassian.net/browse/OGM-763

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