Rework RenderingEngine

[jamoore]

Several times there have been difficulties with the RenderingEngine. See:

• #194
• #206
• #330

This stems partly if not largely from the fact that the RE API was not designed for remote access, especially not for remote access to Hibernate. Taking this into mind, it may make sense to either split the API into stateful and stateless portions or to reintroduce DTOs in some capacity to prevent further troubles. Another alternative would be to implement a DeepCopy similar to ShallowCopy. (This could also be useful for: #80 if done properly, i.e. by passing in the paths for what should be copied/fetched. )

[jmoore]

(10:24:29) chris: Mornin'.
(10:24:36) Josh: the same to ya.
(10:24:42) chris: Issue:
(10:24:48) chris: getPixels() in the RE.
(10:25:09) chris: I need it to have channels, logical channels, etc. linked when the call is made from Shoola.
...
(10:25:21) chris: I have the data retrieval happening with a custom "fetch" iQuery.
(10:25:28) Josh: ok. 
(10:25:43) chris: But when the call returns data to the client, all the persistant lists appear to be null'd out.
(10:27:12) Josh: you've updated PixelsImpl with the fetches?
(10:27:18) chris: Correct.
(10:27:23) Josh: can you show me that?
(10:27:29) chris: Sure, sec.
(10:28:19) chris:     @RolesAllowed("user") 
        public Pixels retrievePixDescription(long pixId) {
                Pixels p = iQuery.findByQuery(
                        "select p from Pixels as p join fetch p.channels as c join " +
                        "fetch p.pixelsDimensions join fetch c.logicalChannel as lc " +
                        "join fetch c.statsInfo where p.id = :id",
                        new Parameters().addId(pixId));
                System.err.println(p.getChannels());
                return p;
        }
...[cut] here we go into debugging madness
... and then chris finds it.
(12:28:49) chris: *LOL*
(12:28:57) chris: return (Pixels) new ShallowCopy().copy(pix); ? :)
(12:29:10) Josh: that'd do it.
(12:29:55) chris: Basically modify/add to ShallowCopy, yeah?
(12:30:03) Josh: Where is this?
(12:30:11) chris: RenderingBean dewd.
(12:30:19) Josh: i can't remember everything ;)
(12:30:36) chris: This was our work around for something.
(12:30:50) Josh: exactly. forgotten what (probably lazy initialization or similar)
(12:31:10) Josh: but yeah, there are several copy methods way down bottom. you need one of those for pixels. 
(12:31:12) chris: Potentially. Might be able to strip it out now since we've got things initializing properly.
(12:31:31) Josh: um,....ok. *shrugs* doesn't hurt to try. 
(12:31:50) Josh: i remember us saying that basically the stateful services were barely going to be be allowed to let any ome.model.* instance pass their borders.
(12:31:54) Josh: again, don't remember why.
(12:32:04) Josh: i'm sure the ticket or changeset which added that schtuff would tell us. 
...
(12:32:59) Josh: ok. whatever it is write this s*!t down. let's not go through this again. #330 fyi. 
(12:33:40) chris: Laters.
(12:36:01) chris: https://trac.openmicroscopy.org.uk/omero/ticket/194
...
(13:34:48) Josh: awesome. sorry that took so long. some of our hackathons have apparently slipped through my mental cracks.
(13:35:15) chris: :)
...[later that day] a discussion about how to prevent this stuff
(16:33:18) chris: z0r.
(17:27:57) Josh: need something?
(17:28:05) chris: Hey.
(17:28:15) chris: Just wanted to discuss this RE metadata thing.
(17:28:22) Josh: ok. what's up?
(17:28:51) chris: So, does IQuery also have to deal with ShallowCopy?
(17:29:31) chris: Or is the RenderingBean's usage of it because it's a stateful service?
(17:30:14) Josh: exactly. it's the statefulness, because of the requirement that the session not be flushed.
(17:31:01) chris: Okay, but does the cleanup filter deal with this somehow?
(17:31:41) Josh: No. The cleanup filter happens too late to take care of this. The stack is:
    <value>serviceHandler</value>
    <value>proxyHandler</value>
    <value>transactionHandler</value>
    <value>sessionHandler</value>
    <value>eventHandler</value>
(17:32:15) Josh: So The cleaning up happens after the transaction is over and the session is closed <em>for stateless services</em>. But for the stateful service, those things don't happen and so ProxyCleanupFilter can't work it's magic. 
(17:33:01) Josh: Specifically because the entities that are in the stateful service are in the session and if you <censored> do anything to 'em, it'll get persisted.
(17:34:01) chris: Makes sense.
(17:34:18) chris: Basically you've got to copy out anything that you want to return client side.
(17:34:38) chris: Is there any way for us to make a sort of aggregate service where one interface has a stateful portion and a stateless portion?
(17:34:57) Josh: from stateful, unfortunately. now I can imagine writing a CopyingInterceptor, basically ShallowCopy but not so shallow. Useful?
(17:35:19) Josh: aggregate service....hmmm....probably. what exactly do you want?
(17:35:56) chris: Well, the big issue is that J-M wants an interface that gives the API user all he/she needs to make a viewer.
(17:36:11) Josh: k...
(17:36:16) chris: Totally understood since you don't want to have to fiddle around with IQuery and the RE in order to get things done in a viewer.
(17:36:32) chris: Kinda silly to put that burden on the client developer when you don't have to.
(17:36:42) Josh: Sure. I'm all for more specific interfaces.
(17:37:07) chris: The logical place is to extend the RE, which is what we've done. (getPixels()) being a prime example.
(17:37:17) Josh: There just haven't been that many use cases up until now. Well, importer, but you guys never seemed really needy.
(17:37:42) Josh: Right, but then there's this crazy constraint on Stateful services, eh?
(17:37:44) chris: However, those metadata retrieval elements are not stateful at all.
(17:37:48) chris: Exactly.
(17:37:55) chris: Makes our job on the server that much harder.
(17:38:13) Josh: Well, in the case of RE it *is* because you *cough* wanted the pixels to be active so you could do lazy initialization. 
(17:38:16) chris: More error/bug prone, etc.
(17:39:39) Josh: What may be easiest is to just use two interfaces for your aggregate (at least this is how I do it):
  RE.doStuff()
  IPixels.getPixels()

RE can then internally use <bean id="internal:ome.api.IPixels"> so that the "activeness" of the objects still holds.
(17:39:40) chris: Well I'm of the opinion that any server developer should not have to be concerned about how he/she deals with the data, it's why we have the model API there, right?
(17:40:12) Josh: Sure. I'm just saying that that effectively makes the RE.getPixels() call stateful because of your use of the Pixels instance.
(17:40:18) chris: Otherwise we may as well throw Hibernate in the bin and just have the JDBC going in and out because it gets much easier that way.
(17:40:34) Josh: you don't know how many times I've asked my self that. 
(17:40:59) chris: :)
(17:42:15) chris: I guess the big problem is there's no way to get around the persistance issues of the RE.
(17:42:21) chris: It's really quite nasty.
(17:43:01) chris: I'm almost getting to the point now where it should be a client all together.
(17:43:04) Josh: Very nasty. It's actually been one of the most complicated things I've seen.
(17:43:10) Josh: :) Super. 
(17:43:44) chris: Which probably means wrapping up data in RE objects.
(17:43:59) chris: Basically an IQuery proxy like IPojos.
(17:45:09) Josh: sorry, i don't follow.
(17:45:37) chris: To give the RE developer the "ease of use" factor it'd be helpful to have DTOs essentially.
(17:45:52) chris: Pixels + Blah + Blah = REPixels.
(17:45:57) Josh: Those objects existed, right? And we deleted them?
(17:46:08) chris: Sort of, yep.
(17:46:19) chris: They mainly existed due to the C bindings.
(17:46:44) chris: And it was a standalone application that was retrieving the data via XML-RPC.
(17:47:28) chris: Basically, having model objects flying around sucks.
(17:47:33) chris: ome.model anyway. :)
(17:49:24) Josh: in the stateful case, yes. That is, when the state is ome.model. You can really only have one or the other: state is ome.model and you send DTOs, or state is DTOs and you send ome.model. 
(17:49:59) chris: Bingo.
(17:50:03) Josh: The second gets you the "aggregate" service you were asking about.
(17:50:30) chris: I think a lot of the complication with the RE comes from the fact that it's in a transaction/Hibernate session.
(17:51:35) chris: Gets really unwieldy to control since it's not particularly well setup for dealing with it and Hibernate sessions are inherently tailored to the method call == HTTP page request problem.
(17:51:46) chris: Which is fine.
(17:52:32) Josh: i disagree with the last part a bit, but in general, yep, RE wasn't made for this. it was made for in VM work. We could refactor the API, actually we probably SHOULD refactor the API, to save us a lot of maintenance headache.
(17:53:28) chris: Well perhaps my understanding of the way the JBoss/J2EE architecture is setup is more in line with the method call == HTTP page request problem.
(17:55:55) chris: Anyway, I think we all agree that this API is not tailored to the use case now and we need to improve it otherwise the headaches that come from this are going to be ridiculously frequent and increasingly large.
(17:57:39) Josh: Ok. I've got a Rework SecuritySystem ticket. Let's add a Rework RE ticket. Combine REImpl and RBean. And we can think about what to do with the stateless calls. The DeepCopy() may suffice. 
(17:57:57) chris: Possibly, yep.
...
(17:58:56) Josh: Alright, I'll be popping in and out too. You take it easy.
(17:59:04) chris: Ciao.

[jmoore]

Includes #175.

[jmoore]

r1024 merges RenderingEngineImpl into RenderingBean (under /server). Also adds RenderingEngine.getCurrentEventContext()

[jmoore]

r1027 cleans up from previous changeset.

[jmoore]

r1030 fixes test references to REImpl and refactors the getCurrentEventContext call to the StatefulServiceInterface

[jmoore]

Do we get rid of the AOP wrappers altogether? Moving to milestone:3.0-Beta4 and pushing to you, Chris.

[cxallan]

Pushing to milestone:OMERO-Beta4.1.

[cxallan]

Closing with implementations using executors in released OMERO 4.x versions.