automatically delete orphaned non-sharable annotations

[mtbc]

To assist http://trac.openmicroscopy.org/ome/ticket/2999 in preventing orphaned non-sharable annotations littering databases and in making the graph policy rules more consistent this PR ensures that when a non-sharable annotation is orphaned by being unlinked it is also deleted.

Testing: ensure that http://regions-ci.docker.openmicroscopy.org:8080/job/OMERO-test-integration/ is blue. Also, try deleting different things from different clients (e.g., comments, ratings, attachments, images, whatever, from Insight and Web) to watch out for regressions in deletion.

[mtbc]

DNS issues today. CI is at http://10.2.1.77:8080/ and OMERO.server at 10.2.1.85 4064.

[pwalczysko]

Unfortunately Insight does not connect for me today to regions server. @mtbc is also tellling me the server is not responding. Lets relist for tomorrow.

[mtbc]

@manics and @sbesson and @aleksandra-tarkowska have fixed regions devspace. 👍

[sbesson]

All integration tests passed successfully and changes look plausible (most of the diff is actually related to the using of the new builders). I have not tested the Web client extensively so this PR should be relisted for functional testing using both clients. /cc @pwalczysko Is there any code in Insight that could be simplified by this PR similarly to eab13c5 @dominikl?

[sbesson]

A more general question looking at the graph rules in more details: what is the rationale for treating comment and basic orphaned annotations (timestamp, numeric...) differently than other orphaned annotations?

[mtbc]

Yes, functional testing would be good indeed.

Good question about rationale. To some extent I simply appeal to the ticket, of course, combined with the mentions of non-sharable and sharable annotations around our code; in general there are special rules about different kinds of annotation that to some extent are suggested by patterns in both old documentation/tickets and integration tests. At the one end are things like comments that appear to be considered to be quite ephemeral and at the other are tags which are certainly not; this may be a spectrum of "sharability" or, oppositely, "dispensability", and is not explicit in the OME-XML schema. When I ask about intermediate kinds like MapAnnotation or XmlAnnotation people usually aren't very sure and the graph policy rules are thus commensurately in-between about those.

With very much of the graph policy stuff I've taken my cue from an impression accumulated from snippets from tickets, code, etc. and conversations with @jburel. I've tried to stimulate some clarification -- for example, relevant to your question, with https://trello.com/c/T3UX9YZe/340-annotations-and-permissions I capture the perhaps-more-recent opinion,

There is a thought that permissions policy on what should be done with annotations when permissions grow tighter should be more about the namespace that they are in than about what kind of annotation they are.

but I get the impression that the original rationale is captured in workflow and design discussions that rather predate me. They might be grounded more in how users were expected to use the graphical clients than in a server-side view.

In short: I'm not sure what the rationale is but I think there is, or at least was, one. Further, this is a specific facet that is fairly typical of many of the other graph policy decisions: the behavior "seems to fit" but if we had more time and energy and stepped back to rethink it all with the current UX team then that might well cause us to change aspects of how things work. In many cases I made precedent-based educated guesses about the edge cases while also trying to use what people were more certain of.

[pwalczysko]

sorry, did not manage any testing here today, #4579 takes lot of resources

Relisting for tomorrow

[pwalczysko]

Still occupied by #4579 . Started testing with insight on regions server, but I do not think I have a 5.3x web server anywhere to play with , @sbesson ?

[sbesson]

@pwalczysko: the Web client should be up and running - see standup links.

[pwalczysko]

No regressions observed, neither from Insight nor from Web.
Deleted P/D/I/Plates (with annotations), ROIs, comments, file attachments, tags, Map Annot (as far as these can be deleted from the UI). All went fine.

[sbesson]

Depending on the scoping on production DB annotations distribution, this deletion rule might need to be reviewed/adjusted - see conversation of #4608. How easy would it be to include namespace criteria for instance?

[mtbc]

It's certainly possible but not trivial. I have a checklist item for that on https://trello.com/c/zNzeSGlw/429-expand-graph-traversal-rules. So, we then have the overhead of querying every linked annotation's namespace to check if it's a match, and for each circumstance for each of the graph operations in which they delete an annotation (e.g., moving an image to a private group and somebody else made the comment) we need to decide what to do with the annotation, and adjust the policy rules accordingly. (Also is there any other special treatment required for certain annotations, e.g., regarding chgrp and chown?)

(If I adjust graph traversal so that namespace matches are possible at all, maybe somebody else can adjust the rules as required and add integration tests accordingly!)

[mtbc]

(Of course, 5.0 didn't do anything special for these either so this shouldn't affect many existing integration tests.)

[mtbc]

Obvious question is: Will such special annotations ever be affected by this? E.g., are they sometimes actually linked to something and then orphaned, or are they always orphans from the start?

[mtbc]

@aleksandra-tarkowska, @manics: this PR enables deletion of orphaned map annotations. Also see #4907.

[joshmoore]

Coming back to #4585 (comment) -- I wonder to what extent a sharability trigger would be appropriate.

[manics]

--rebased-to #5068

[manics]

I think this suffers from poor performance when one MapAnnotation is linked to several objects, and just one of those links is deleted: #5068 (comment)

[mtbc]

It's certainly going to be slower mostly because it now actually looks at those basic/comment annotations and their linkages at all rather than just ignoring them as it did previously. Should still perform far better than similar would have under OMERO 4 though. 😃

[joshmoore]

Not just slower as I understood it @mtbc: the server OOM'd.

[mtbc]

Ah, no hint of that in the linked comment: that would be surprising/disappointing given the lengths the graph traversal goes to to avoid actually hydrating Hibernate proxies.

[joshmoore]

Exception: object #0 (::omero::cmd::ERR)
{
    category = ::omero::cmd::Delete2
    name = graph-fail
    parameters = 
    {
        key = stacktrace
        value = java.lang.OutOfMemoryError: GC overhead limit exceeded
	at java.util.HashMap$KeySet.iterator(HashMap.java:916)
	at java.util.HashSet.iterator(HashSet.java:172)
	at com.google.common.collect.AbstractMapBasedMultimap$Itr.next(AbstractMapBasedMultimap.java:1148)
	at java.util.AbstractCollection.addAll(AbstractCollection.java:343)
	at java.util.HashSet.<init>(HashSet.java:119)
	at ome.services.graphs.GraphTraversal.getLinksToCache(GraphTraversal.java:884)
	at ome.services.graphs.GraphTraversal.cache(GraphTraversal.java:933)
	at ome.services.graphs.GraphTraversal.planOperation(GraphTraversal.java:616)
	at ome.services.graphs.GraphTraversal.planOperation(GraphTraversal.java:513)
	at omero.cmd.graphs.Delete2I.step(Delete2I.java:146)
	at omero.cmd.HandleI.steps(HandleI.java:438)
	at omero.cmd.HandleI$1.doWork(HandleI.java:366)
	at omero.cmd.HandleI$1.doWork(HandleI.java:362)
...     
        key = message
        value = GC overhead limit exceeded
    }
}

[joshmoore]

Moving conversation to https://trello.com/c/mfW0N1an/272-poor-performance-when-deleting-many-one-annotation-links