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

fix: (IntegrationContext) add process instance execution context attributes #2240

Merged
merged 6 commits into from Dec 18, 2018

Conversation

Projects
None yet
3 participants
@igdianov
Copy link
Member

igdianov commented Dec 5, 2018

This PR injects process instance execution context attributes into IntegrationContext holder class at runtime.

@igdianov igdianov self-assigned this Dec 5, 2018

@igdianov igdianov requested review from erdemedeiros and salaboy Dec 5, 2018

@igdianov igdianov added the enhancement label Dec 5, 2018

@salaboy salaboy added the in progress label Dec 5, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 5, 2018

Codecov Report

Merging #2240 into develop will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2240      +/-   ##
=============================================
+ Coverage      61.16%   61.17%   +<.01%     
  Complexity      1523     1523              
=============================================
  Files           1125     1125              
  Lines          39966    39968       +2     
  Branches        5818     5818              
=============================================
+ Hits           24447    24451       +4     
- Misses         12788    12789       +1     
+ Partials        2731     2728       -3
Impacted Files Coverage Δ Complexity Δ
...ntime/api/connector/IntegrationContextBuilder.java 13.63% <0%> (-1.37%) 0 <0> (ø)
...mpl/agenda/TakeOutgoingSequenceFlowsOperation.java 88.53% <0%> (-1.28%) 0% <0%> (ø)
.../entity/data/impl/MybatisExecutionDataManager.java 79.31% <0%> (+1.72%) 0% <0%> (ø) ⬇️
...mpl/asyncexecutor/AcquireAsyncJobsDueRunnable.java 70.17% <0%> (+3.5%) 0% <0%> (ø) ⬇️
...xecutionsWithSameRootProcessInstanceIdMatcher.java 37.5% <0%> (+12.5%) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c1c486...55d5929. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 5, 2018

Codecov Report

Merging #2240 into develop will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2240      +/-   ##
=============================================
- Coverage      61.16%   61.13%   -0.03%     
  Complexity      1521     1521              
=============================================
  Files           1126     1126              
  Lines          39977    39990      +13     
  Branches        5819     5822       +3     
=============================================
- Hits           24452    24448       -4     
- Misses         12794    12808      +14     
- Partials        2731     2734       +3
Impacted Files Coverage Δ Complexity Δ
...ntime/api/connector/IntegrationContextBuilder.java 9.09% <0%> (-5.91%) 0 <0> (ø)
...xecutionsWithSameRootProcessInstanceIdMatcher.java 25% <0%> (-12.5%) 0% <0%> (ø)
...mpl/asyncexecutor/AcquireAsyncJobsDueRunnable.java 66.66% <0%> (-3.51%) 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1253d37...6a6e3d1. Read the comment docs.

@igdianov igdianov changed the title fix: (IntegrationContextBuilder) add process instance business key fix: (IntegrationContextBuilder) add process instance execution context Dec 12, 2018

@igdianov igdianov changed the title fix: (IntegrationContextBuilder) add process instance execution context fix: (IntegrationContext) add process instance execution context attributes Dec 12, 2018

@erdemedeiros
Copy link
Member

erdemedeiros left a comment

Sounds good to me.

Optional.ofNullable(processInstance.getSuperExecution())
.ifPresent(superExecution -> integrationContext
.setParentProcessInstanceId(superExecution.getProcessInstanceId()));
} catch (NullPointerException e) {

This comment has been minimized.

@igdianov

igdianov Dec 13, 2018

Member

@salaboy @erdemedeiros Does it make sense to ignore the use case when using IntegrationContextBuilder outside of Activiti transaction?

This comment has been minimized.

@erdemedeiros

erdemedeiros Dec 17, 2018

Member

@igdianov I think it makes sense to ignore it in this case. I'm just wondering if can avoid this catch NPE. Maybe by updating the if block to if(processInstance.getSuperExecutionId() != null && Context.getCommandContext() != null ) {?

This comment has been minimized.

@igdianov

igdianov Dec 18, 2018

Member

@erdemedeiros I was thinking that if IntegrationContextBuilder intended use is only inside Activiti transaction internally, we should catch that NPE when trying to extract parent process Id and fail by throwing an ActivitiException to signal the problem. Ignoring NPE does not seem right to me at this moment.

This comment has been minimized.

@erdemedeiros

erdemedeiros Dec 18, 2018

Member

@igdianov as far as I know this code will always be called from inside an Activiti transaction as it's used by service tasks behaviours. The question is how capital the the information about parent process id is. Shaw we fail if we are not able to retrieve it or shaw we just ignore it?
But I see your point, maybe it's safer to throw an error so we know if this code get invoked outside of an Activiti transaction in the future.

This comment has been minimized.

@igdianov

igdianov Dec 18, 2018

Member

@erdemedeiros It seems to me that parent process id is important part of the IntegrationContext contract. We should be able to to resolve and provide it to connectors if it exists. If we cannot resolve superExecution due to improper usage outside of engine context then it is better to fail fast. If you agree, I can make a test for this case.

This comment has been minimized.

@erdemedeiros

erdemedeiros Dec 18, 2018

Member

@igdianov yes, it makes sense. I'm ok with this approach.

@salaboy salaboy merged commit 6ecc0ac into develop Dec 18, 2018

6 of 8 checks passed

codecov/patch 0% of diff hit (target 61.16%)
Details
codecov/project 61.13% (-0.03%) compared to 1253d37
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
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 added a commit to Activiti/activiti-cloud-runtime-bundle-service that referenced this pull request Dec 21, 2018

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

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](https://user-images.githubusercontent.com/20428629/48309101-0b3f8e80-e527-11e8-8ac7-cce1ec96a691.png)

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`

4. 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](https://user-images.githubusercontent.com/20428629/49545925-ea532a80-f893-11e8-96d2-6880fd82b39a.png)

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

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

7. 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment