Skip to content

cache OMERO sessions in level 2 Hibernate cache#5281

Merged
mtbc merged 4 commits intoome:rolesfrom
mtbc:roles-session-cache
May 11, 2017
Merged

cache OMERO sessions in level 2 Hibernate cache#5281
mtbc merged 4 commits intoome:rolesfrom
mtbc:roles-session-cache

Conversation

@mtbc
Copy link
Copy Markdown
Member

@mtbc mtbc commented May 5, 2017

What this PR does

Adjusts HQL queries of sessions in performance bottlenecks to enable caching.

Testing this PR

The server, especially integration tests, should in general run a little faster.

Related reading

https://trello.com/c/tYg2svfX/

@jburel jburel added the roles label May 7, 2017
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If merged then this would be a 5.3 deprecation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you want to open a 5.3 PR and mark it as deprecated?
so we can/cannot remove it later on depending on outcome

@joshmoore
Copy link
Copy Markdown
Member

I may have to go back and review the reasoning, but I'm remembering there being a reason (around stateful sessions or filters, etc) that we hid Hibernate's Session instance. There's also the LocalQuery service for nice server-internal items. Would it be an option to have a findByCachedQuery (or similar) there?

@mtbc
Copy link
Copy Markdown
Member Author

mtbc commented May 9, 2017

Add an API method to LocalQuery specifically? I could give it a try.

@mtbc mtbc force-pushed the roles-session-cache branch from 40fc4ea to 58fc1b3 Compare May 10, 2017 08:51
@joshmoore
Copy link
Copy Markdown
Member

This looks nice and simple! One research-y question (if you happened to look into it while preparing this): is there anything we should take into account when it comes to this API in terms permitting external users to also enable the cache for HQL queries? My hope was always that we could enable all queries (but I don't know what the impact of that would be)

@mtbc
Copy link
Copy Markdown
Member Author

mtbc commented May 11, 2017

Good question @joshmoore. I'd be hesitant to offer clients the choice of caching: the impression from my reading is that the query cache often does more harm than good (overhead in checking, invalidating, etc.), especially for data that might change, so the choice probably demands more subtlety and testing than we might expect from people writing scripts, etc., especially as this could impact other users of their server. I'm not even wholly convinced about this case here: VisualVM certainly suggests that this PR is a clear win in the expected places, but in my more real-world testing I don't see anything compelling either way emerging from the noise of varying times. In short: I doubt it's promising enough to be worth investigation / complication.

Could well be worth revisiting once we're using Hibernate 5: of course I've largely been reading Hibernate 3 stuff.

@mtbc mtbc merged commit 5d82e91 into ome:roles May 11, 2017
@mtbc mtbc deleted the roles-session-cache branch May 11, 2017 09:48
@sbesson sbesson added this to the 5.4.0 milestone Nov 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants