ome / omero-server

Gradle project containing main server logic for OMERO
https://www.openmicroscopy.org/omero
GNU General Public License v2.0
5 stars 14 forks source link

Experimenter group cache #166

Open kkoz opened 1 year ago

kkoz commented 1 year ago

Fixes #165. Creates a simple in-memory cache which contains all the relevant data in the ExperimenterGroup and ExperimenterGroupMap tables to perform the isRelatedUser check. Some things that still need to be implemented/fixed:

Testing:

There are a few different scenarios which need to be tested:

chris-allan commented 1 year ago

Updating a group's permissions (e.g. from Private to ReadOnly) does not seem to create an event with entity of type ExperimenterGroup as I expected. It does create one with an entity of Session, but I don't think we want to update the cache every time we get a Session event. I haven't figured out why this is or what to do about it.

This is because modifying a group's permissions is performed by the Chmod2 infrastructure ^1 which does not use Hibernate entities to perform changes to the object graph. When a Chmod2 request completes processing of an object it publishes an EventLogMessage:

https://github.com/ome/omero-blitz/blob/master/src/main/java/omero/cmd/graphs/Chmod2I.java#L344-L352

https://github.com/ome/omero-blitz/blob/master/src/main/java/omero/cmd/graphs/GraphHelper.java#L232-L240

This can be picked up if the PermissionsCacheEventListener were to implement ApplicationListener<EventLogMessage>.

I'm not entirely sure what to do if the cache fails to update and becomes invalid. Currently, I just flag it as invalid and it is ignored until it updates successfully. It's probably better than the server going down, but it also means that it's possible your server will experience a sudden decrease in performance and you have to go to the logs to figure out why.

Thinking about this a little more I'm now of the opinion that we need to react to changes by making incremental updates that synchronize database state with in memory state. Invalidating the entire cache whenever any of these objects are modified is not a viable solution. If 50 users are added to a group we cannot be refreshing the entire cache 50 times; that's not even an extreme use case.

At least we have the entities in hand for everything but the permissions change so should be able to update things very quickly without roundtripping to the database.

I noticed that if you change a user to or from a group owner, it takes awhile for the event context to be updated and until then you may see incorrect responses when getting user data.

Based on this and what I've mentioned above I don't think the EventContext is a good source of information. It should likely not be trusted for any of this work and instead the correct queries performed in its place.