@mseaton I’m contracted to work on TRUNK-6507 by OpenMRS Inc. and complete by the end of June. I think most will be done in May. I’ll keep posting PRs for code-reviews for community and I’d appreciate your input.
I’ve tried to include things that we discussed here in TRUNK-6515, which is a subtask of TRUNK-6507. Feel free to comment or edit if there’s anything I missed.
@mseaton I see that the event module groups changes to all entities belonging to one transaction and issues them as a single event only just before and after commit. Is this kind of grouping before issuing an event used as a mean of ensuring that event is only published when transaction is about to commit and committed or you have some other use case for that?
I think the more flexible approach is to have separate events for each individual entity and register a @TransactionalEventListener(phase=AFTER_COMMIT or BEFORE_COMMIT) possibly making it even @Async or mark events to go into the above discussed outbox or listen in any other way Spring Events provide. This way you will also be able to subscribe to events for an entity that you are interested in and not having to go through different entities in a transaction. We are basically delegating handling of transactions to Spring Events instead of having to keep a stack of events ourselves. We could easily include a transaction or session id within an event for each entity so that it’s easy to group at a later point changes that happened in a particular transaction if there’s a use case for that…
A lot of this is informed by experiences with sync and it’s use cases. There, a sync record is stored and transmitted between systems, and that sync record is a serialized representation of all of the entities that were saved together in a single transaction.
In other cases, I have found that we want to, say, update an MPI / CR any time a patient’s demographics have changed. There are a few pieces to that:
a) If we detect a change to Person, PersonName, PersonAddress, 3 Person Attributes, 2 Patient Identifiers, etc. we don’t want this to result in sending 8+ separate messages to update an external system, we just want to detect this as a single update event.
b) If we just look for changes to, say, Patient - I am not 100% convinced that our uses of Hibernate and services and cascading entities and such will actually fire a Patient UPDATED event for associated patient data changes. For example, if a PersonAttribute is changed, and this is saved via a call to PersonService.savePersonAttribute, does this result in a dirty flush to the Person object that Hibernate detects and issues a Person updated event for? This is even more true in a Debezium-fed model. This issue doesn’t really go away with the design we have in place, but by not firing events strictly based on entity type, it makes it more of a business decision for a listener to listen for all events, and determine if any of the entity changes it contains should result in some sort of action.
If this works, then great! But I would want to make 100% sure it actually does what we think it will do. I assume AFTER_COMMIT only fires if the commit was successful, and not fired on rollback?
This is still already in place in the events module. I mean, there was always a Hibernate interceptor that listened for entity changes. Prior to version 4.0.0, this just always fired these out asynchronously on a JMS queue by type, and one would subscribe to a type. That still happens today in the 4.0.0+ world - it just happens in a TransactionEventListener that listends for a TransactionCommittedEvent, and iterates over the entities that were committed and fires the exact same events out at that stage.
So within the current event module design, we would simply create a TransactionEventListener that listens for a TransactionBeforeCompletionEvent, and either writes the individual entity changes or some serialized representation (like sync does) of all entity changes in the event, to the outbox table transactionally, so that it would either succeed or fail along with the source transaction. If there are cleaner and better ways to do this, fine by me as long as it works.
We should definitely do this, regardless of which approach we take.
This has also been the consistent issue we’ve needed to work around with Debezium. I think it’s a desideratum of any event publishing system we have to be able to produce higher-level events (e.g., a write the PersonName table cascades as an “patient update” message even if we also publish lower-level events for systems that only care about those).
Here’s a draft PR with proposed APIs for Events: TRUNK-6429: Create application events for service method calls and en… by rkorytkowski · Pull Request #6084 · openmrs/openmrs-core · GitHub
It’s still far from being mergeable, but I’m sharing to get your input as soon as possible.
Please have a look at the OutboxEventTest, TestEventListener, TestEventPublisherService on how to put events in the outbox table. Basically an event will only land in the outbox table, if there’s a listener annotated with @OutboxEventListener. Also outbox events are not dispatched with the standard Spring Events mechanism rather a dedicated OutboxEventRegistry so that we have full control over how they are handled. We want them to be processed by listeners in order, in separate transactions and await for results to confirm delivery. For now, if any listener fails, then the event is dispatched again for all listeners.
For polling the outbox we use the newly revamped scheduler service, which gives visibility into any failures (with stacktraces) and provides automated re-tries. We are also publishing application events on failures and success for outbox listeners so e.g. a system admin can be notified by some mechanism that the queue is stuck.
In RequiredDataAdvice there are entity events fired on service methods. If there are no listeners for these events, they are just ignored. You can have standard Spring Event listeners (@EventListener or @TransactionalEventListener) or our @OutboxEventListener for any of those events.
I’m going to publish in a similar fashion Hibernate interceptor events (still reasoning about @mseaton response).
Looking forward to your thoughts on this initial design.
@angshuonline @lingeswaran.s @mohant @mseaton flagging you for a review of interfaces. Thanks!
I’m finally happy with the code for outbox processing and aggregated events.
The PR is ready to be reviewed and merged at TRUNK-6429: Create application events for service method calls and en… by rkorytkowski · Pull Request #6084 · openmrs/openmrs-core · GitHub
I will be working on adding an optional broker and Debezium now.