New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Inject process run-time message headers for aggregated events #143

Merged
merged 53 commits into from Dec 21, 2018

Conversation

Projects
None yet
2 participants
@igdianov
Copy link
Member

igdianov commented Nov 10, 2018

This PR fixes Activiti/Activiti#2069 via injecting Runtime Bundle configuration property values and execution context attributes derived from execution bound to entities aggregated in Activiti engine CommandContext as Spring Message headers, so that these headers can be used by consumers to correlate events based with business context using business key, process definition key, process definition id, runtime bundle attributes, etc.

Depends on Activiti/Activiti#2240 and Activiti/Activiti#2251. Part of Activiti/activiti-cloud-api#44

image

There are a few gotchas that need to be discussed:

  1. ExecutionContext can only be resolved within process engine transaction boundary. It relies on existing CommandContext to find execution, processInstance and deployment entities using ExecutionEntityManager instance. All runtime execution context key attributes can be discovered and added as message headers with minimum overhead.

  2. IntegrationContext has only a few attributes such as processInstanceId, processDefinitionId that can be resolved only from event itself. It is would be nice to have businessKey attribute. The only option is to actually inject it into event attributes directly.

  3. We need to decide what key attributes set is required to support in messages for events:

  • RuntimeBundleInfo.*
  • businessKey
  • processDefinitionKey
  • processDefinitionVersion
  • processDefinitionId
  • processInstanceId
  1. The message destination (routing key or topic) could be very useful for selectively consuming events in other micro-services. The destination could represent a hierarchy of key attributes that could be subscribed to using mask binding on the consumer side, i.e.: for message with destination rb-app.app-name.proc-def-key.proc-id.biz-key, consumers could create bindings to receive only messages with business-key: #.biz-key or rb-app.#.biz-key.

image

  1. Resolving Execution Context attributes for all events once per transaction offers the best performance.

  2. It is possible to apply sending strategies inside MessageProducerCommandContextCloseListener, i.e. dis-aggregating events into single messages.

  3. It would be best practice to resolve and apply execution context from events aggregated inside transaction to ensure consistent event handling to be able to inject context attributes for all events in one place.

@igdianov igdianov self-assigned this Nov 10, 2018

@igdianov igdianov requested review from erdemedeiros and salaboy Nov 10, 2018

@igdianov igdianov changed the title feat: Inject process run-time message headers in for aggregated events feat: Inject process run-time message headers for aggregated events Nov 10, 2018

@igdianov igdianov added the wip label Nov 12, 2018

Not sure mergify will pick up the do-not-merge label and the PR is not ready to merge yet.

@igdianov igdianov added ready-for-review and removed wip labels Dec 12, 2018

@erdemedeiros

This comment has been minimized.

Copy link
Member

erdemedeiros commented Dec 13, 2018

@igdianov after discussion with @CTI777 yesterday, I'm wondering if we are setting the context information (processInstanceId, etc) in the right place. I mean we are setting it for cloud events only, and that would be great to have them for non cloud events as well, meaning that we'd need to set them in the converters from internal model to api model. I.e. https://github.com/Activiti/Activiti/blob/develop/activiti-api-impl/activiti-api-process-runtime-impl/src/main/java/org/activiti/runtime/api/event/impl/ToAPIProcessStartedEventConverter.java#L35. This could be done in another PR tough. Thoughts?

@igdianov

This comment has been minimized.

Copy link
Member

igdianov commented Dec 13, 2018

@erdemedeiros It is better to create another issue with description about what we already know and resolve it in a separate PR to address the problem of lazy loading super execution entities inside the engine in order to be able to use parent information in converters running outside of execution context.

igdianov and others added some commits Dec 11, 2018

Revert Mergify configuration back to `merge` method
so we can see which set of version updates were grouped together

@erdemedeiros erdemedeiros force-pushed the igdianov-message-headers branch from 6f744e9 to 1871359 Dec 21, 2018

@erdemedeiros erdemedeiros merged commit 450f89b into develop Dec 21, 2018

5 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
security/snyk - pom.xml (Activiti) No new issues
Details

@erdemedeiros erdemedeiros deleted the igdianov-message-headers branch Dec 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment