Brief summary: the Atom Feed module built for Sync 2 copy/pasted code from the Event module, because the asynchronous-after-the-transaction event delivery provided by that module wasn’t sufficient. Therefore we’ve been trying to figure out how to make the Event module support the Atom Feed use case.
Taking a step back, I think there are two general use cases that we should be seeking to address:
Immediate notification of database changes, so that a listener can also take some action within the same DB transaction (and possibly roll it back).
this is for low-level sorts of activities, and probably has performance implications too
Basically this is for use cases where you want a hibernate interceptor, but it would be nice if the hibernate interceptor logic could be written just once and others could just get callbacks from one place.
Notification after application events have happened (without the ability to cancel that event)
This is the traditional Event paradigm
Example: get notified every time a new patient is registered; get notified when a new order is created
(wearing my ThoughtWorks hat we tend to say that you should listen on an atom feed instead of doing this paradigm, but I won’t go into that now)
I think there’s a simple solution, based on our current state:
Introduce a new module named “dbchangenotification”, which has a hibernate interceptor, and gives a mechanism to get a callback just before the transaction is closed. This callback mechanism would have much simpler semantics than a hibernate interceptor.
obviously the module needs a better name than this
really this ought to go in openmrs-core, but that would happen later
the event module basically stays as-is, but it could depend on this new module instead of defining its own hibernate interceptor
Pro: have a standard but simpler mechanism for triggering on changes to domain objects; it’s possible to build a lighter-weight OpenMRS distro without the event module and ActiveMQ while still using this.
Con: the RefApp (and/or anything that depends on registrationcore) would have one more module
@wyclif, you seem to think that there are additional meaningful use cases beyond the two I mentioned above. Can you say more about that, and what the real-world scenario is for them?
The alternatives approaches I see are:
Say it’s okay for atomfeed to have a hibernate interceptor, as it’s pretty integral to what that module does. (I.e. the atomfeed and event modules can both stay exactly as they are today.)
The new module that I describe above is instead made a part of the Event module (thus making that module a bit larger)
Irrespective of any of this, we should update the event module to have the latest version of ActiveMQ rather than the very old one that’s there today.
@darius I believe you just echoed the things I tried to bring up in my last comment on EVNT-36, any use case I can imagine would typically fall under those 2 general categories.
I personally think the firing of events in the atom feed module is happening in the wrong place in the interceptor.
@darius, thanks for posting this - this is a very helpful summary. I don’t think I’m free for the design all on Wednesday (@jthomas FYI), but I like your approach. I’m not sure we need yet another module. What if we have a single module that contains both the synchronous, transactional event notifications (with the interceptor) and the atomfeed implementation.
For argument’s sake, let’s say we make the atomfeed module into this (since it already has all of the atom feed code and also the copied interceptor). Then we move the small number of classes from the Event module into the atom feed module so that they can be used there and maintain backwards-compatibility (eg. they would still be org.openmrs.event). Then we re-write the Event module to depend on the atomfeed module.
I’d suggest that then the atomfeed module would become a part of the reference application, and the event module would no longer be included, and registrationcore would be updated to depend on the atomfeed module as described above.
Interesting idea to make the atomfeed module the primary one, and to add this to the Refapp.
We ended up discussing this on today’s design call earlier today (since people didn’t show up for the planned topic) and we eventually settled on this:
(top priority) First, fix the atomfeed module (Wyclif to provide details). At this point it will still be a copy-paste-modify of event, with duplicated code.
TODO: Wyclif will comment on Talk thread and create ticket for this work
(medium priority) extract the tricky-but-useful pattern that’s in the hibernate interceptors for event and atomfeed, give it a simple callback API, and put this in openmrs-core.
After this we could refactor event and atomfeed to remove the duplication according to desire. (This might not happen for years, because these modules will care about backwards-compatibility with earlier openmrs-core versions, though maybe we can provide a backwards-compatibility shim for this)
TODO: Darius will create ticket for this (closing EVNT-36 and pointing to it)
(low priority, if anyone cares) make improvements to the Event module (upgrade to latest ActiveMQ, try to ensure in-order delivery)
One thing I noticed during the call is that openmrs-core has multiple hibernate interceptors of its own, so I now care less about the concern of having one redundant/duplicated interceptor in a module.
The openmrs-core interceptors are:
Chaining (actually this allows for multiple hibernate interceptors)
DropMilliseconds
Auditable
ImmutableObs
ImmutableOrder
@mseaton does that new approach sound good? I guess we’d want a separate ticket (also low priority) to switch registrationcore from depending on Event to Atom Feed.
@darius, I think this sounds OK but am not 100% sure I’m understanding entirely. Will the atomfeed module just have an Interceptor that is concerned with it’s own use case (recording an entry on the feed), or will the atom feed module also contain the code needed to subscribe to events with a callback that can be executed within the current transaction?
If we are going to add to core, but still need to maintain some duplicate code in a module for those who cannot upgrade, can we do this across the board with the atomfeed module?
Step 1:
Copy Event API over to Atom Feed module (essentially the API classes that exists in the Event module, with key differences that the EventEngine does not depend on ActiveMQ, and the Interceptor executes listeners before-transaction-completion, and not after-transaction-completion)
Step 2:
Move all of the code from Step 1 into OpenMRS core (including atom feed implementation)
Then, any implementation or distribution not on openmrs-core version X (wherever it is added in step 2), can install the atomfeed module to achieve the same functionality.
Step 3:
Event module is changed to remove all of the classes that were extracted into atomfeed/core above, depend on atom-feed conditionally based on core version), and simply add in the ActiveMQ component.
I don’t think the atomfeed module can be that module with the core functionality because it has other baggage, the new module needs to have just the interceptor and a simple API that provides a mechanism for listeners to subscribe to events so that other modules like atomfeed don’t need to duplicate the interceptor code and this is something we intend to do separately at a later point and possibly should be in openmrs-core, the reason for having it in a new module initially is so that those running older versions of core can benefit from it.
In the short term, all we are going to do is update the atomfeed module to create feeds before transaction completion so that they don’t have to have some extra code around managing new transactions.
atomfeed has an interceptor only concerned with its own use case, with minor changes per Wyclif
event module stays as-is (i.e. keeps it own interceptor)
openmrs-core introduces a new callback-on-db-change API, but this is only available from 2.2.0.
Later
we have a module like “callbackondbchange” which is a compatibility module providing this functionality for versions of openmrs core < 2.2.0
atomfeed is aware-of callbackondbchange, and will conditionally use either openmrs-core or this
even module is aware-of callbackondbchange, and will conditionally use either openmrs-core or this
Now that I have written this, it seems like it actually makes sense to write the callbackondbchange module first and then move its code to openmrs-core after some testing.
(Mike I think you’re suggesting that we should just put this new API in atomfeed for now to avoid having yet another module and because atomfeed seems to be becoming a recommended module. I understand that motivation but I’d still vote for putting in in a single clean module, that will no longer be required after the next core release.)
Also, note that the Event API is actually based on JMS, and we wouldn’t actually want this exact API for the “callbacks within hibernate interceptor” module.
For the atomfeed module, the changes we need to make are to move the calls that create the feeds to be made before transaction completion, this has 2 benefits
The module doesn’t need to create a new transaction to be able to commit the created feed instances to the DB.
In the event of failure of creation of the feed, the entire transaction fails as a single unit.
If there is a compelling reason to make the call backs after transaction completion then I’d think the work around would be to fix the service method that saves the feeds to be annotated with @Transactional and the propagation attribute set to REQUIRES_NEW and let spring deal with the transaction management as opposed to doing it manually like the current code.
Yes (and this applies to @wyclif’s comment earlier to me) - the reason I’m wondering if we can just do this in atomfeed is if we think that atomfeed is becoming a standard approach that also warrants it’s inclusion in core. If so, we could just use the atomfeed module as this compatibility module and move all of it into core in 2.2.0.
