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

[DS-3656] DOIOrganiser CLI does not change the database anymore #1812

Closed
wants to merge 2 commits into from
Closed

[DS-3656] DOIOrganiser CLI does not change the database anymore #1812

wants to merge 2 commits into from

Conversation

ssolim
Copy link
Contributor

@ssolim ssolim commented Jul 20, 2017

HibernateDBConnection add session.flush in uncacheEntity 6_x

@ssolim ssolim changed the title HibernateDBConnection add session.flush in uncacheEntity HibernateDBConnection add session.flush in uncacheEntity 6_x Jul 20, 2017
@ssolim ssolim changed the title HibernateDBConnection add session.flush in uncacheEntity 6_x [DS-3656] DOIOrganiser CLI does not change the database anymore Jul 20, 2017
@mwoodiupui
Copy link
Member

Just a note: we may be coming back to this code later for performance reasons. uncacheEntity is mildly recursive and can (now) result in multiple flushes (up to eight, for a Collection). If this is a problem, then probably the logic needs to move to an internalUncacheEntity() that does no flushing, wrapped in uncacheEntity which just delegates to internalUncacheEntity() and then does one flush. All the recursion would be confined to internalUncacheEntity(). Likely it is best to treat this as premature optimization, for now, but I noticed it and wanted to capture the thought.

@tomdesair
Copy link
Contributor

This is (I think) prevented by the getSession().isDirty() check. This test will only return "true" is there are "unflushed" statements. Once you call getSession().flush() your session is "clean" again and it will not be executed again by the next recursive call. But it's best that we double check this by adding some debug/trace logging maybe.

@tdonohue
Copy link
Member

tdonohue commented Aug 9, 2017

On Slack #dev channel today, @ssolim recommended reverting the changes in this PR, and instead fixing this issue using newly added commit() messages like this: ssolim/DSpace@ac3ecbf

@ssolim
Copy link
Contributor Author

ssolim commented Aug 10, 2017

I will close this as https://github.com/DSpace/DSpace/pull/1823/files is preferred solution

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

Successfully merging this pull request may close these issues.

None yet

4 participants