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

BroadcastSignal not work with Activiti Core API and Activiti Cloud Rest API #2229

Merged
merged 19 commits into from Dec 30, 2018

Conversation

Projects
None yet
6 participants
@daisuke-yoshimoto
Copy link
Member

daisuke-yoshimoto commented Dec 3, 2018

No description provided.

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

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Dec 3, 2018

CLA assistant check
All committers have signed the CLA.

@daisuke-yoshimoto daisuke-yoshimoto changed the title [【Not Ready]Add event publishing to signal api [Not Ready] Add event publishing to signal api Dec 3, 2018

@daisuke-yoshimoto daisuke-yoshimoto changed the title [Not Ready] Add event publishing to signal api [Not Ready] BroadcastSignal not work with Activiti Core API and Activiti Cloud Rest API Dec 3, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 4, 2018

Codecov Report

Merging #2229 into develop will decrease coverage by <.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2229      +/-   ##
=============================================
- Coverage      61.18%   61.18%   -0.01%     
  Complexity      1523     1523              
=============================================
  Files           1125     1125              
  Lines          39963    39967       +4     
  Branches        5818     5818              
=============================================
+ Hits           24453    24455       +2     
- Misses         12783    12785       +2     
  Partials        2727     2727
Impacted Files Coverage Δ Complexity Δ
...i/runtime/api/ProcessRuntimeAutoConfiguration.java 92.59% <ø> (ø) 0 <0> (ø) ⬇️
...viti/runtime/api/impl/ProcessAdminRuntimeImpl.java 7.52% <66.66%> (+0.93%) 0 <0> (ø) ⬇️
.../activiti/runtime/api/impl/ProcessRuntimeImpl.java 7.4% <66.66%> (+0.64%) 0 <0> (ø) ⬇️
.../entity/data/impl/MybatisExecutionDataManager.java 77.58% <0%> (-1.73%) 0% <0%> (ø)
...e/impl/persistence/entity/ExecutionEntityImpl.java 85.53% <0%> (+0.62%) 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 4a27b78...43c0788. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 4, 2018

Codecov Report

Merging #2229 into develop will increase coverage by 0.04%.
The diff coverage is 66.66%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2229      +/-   ##
=============================================
+ Coverage      60.93%   60.97%   +0.04%     
- Complexity      1564     1592      +28     
=============================================
  Files           1138     1141       +3     
  Lines          40382    40597     +215     
  Branches        5888     5907      +19     
=============================================
+ Hits           24605    24754     +149     
- Misses         13023    13081      +58     
- Partials        2754     2762       +8
Impacted Files Coverage Δ Complexity Δ
...i/runtime/api/ProcessRuntimeAutoConfiguration.java 94.54% <100%> (+5.25%) 0 <0> (ø) ⬇️
...me/api/impl/RuntimeSignalPayloadEventListener.java 50% <50%> (ø) 0 <0> (?)
...viti/runtime/api/impl/ProcessAdminRuntimeImpl.java 7.52% <66.66%> (+1.07%) 0 <0> (ø) ⬇️
.../activiti/runtime/api/impl/ProcessRuntimeImpl.java 12.4% <66.66%> (+0.72%) 0 <0> (ø) ⬇️
...autodeployment/AbstractAutoDeploymentStrategy.java 78.26% <0%> (-21.74%) 13% <0%> (+8%)
...xecutionsWithSameRootProcessInstanceIdMatcher.java 25% <0%> (-12.5%) 0% <0%> (ø)
...e/impl/persistence/entity/ExecutionEntityImpl.java 84.82% <0%> (-0.62%) 0% <0%> (ø)
...runtime/api/conf/TaskRuntimeAutoConfiguration.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...utodeployment/NeverFailAutoDeploymentStrategy.java 93.33% <0%> (ø) 6% <0%> (?)
... and 7 more

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 f1df4d3...31f1f61. Read the comment docs.

daisuke-yoshimoto added some commits Dec 5, 2018

daisuke-yoshimoto added some commits Dec 7, 2018

@daisuke-yoshimoto daisuke-yoshimoto changed the title [Not Ready] BroadcastSignal not work with Activiti Core API and Activiti Cloud Rest API BroadcastSignal not work with Activiti Core API and Activiti Cloud Rest API Dec 7, 2018

@daisuke-yoshimoto

This comment has been minimized.

Copy link
Member

daisuke-yoshimoto commented Dec 7, 2018

@salaboy @ryandawsonuk @erdemedeiros
I completed fixing. Please review this pr.

daisuke-yoshimoto added some commits Dec 13, 2018

@salaboy salaboy requested a review from igdianov Dec 13, 2018

@@ -187,10 +193,12 @@ public ProcessInstance delete(DeleteProcessPayload deleteProcessPayload) {
}

@Override
@Transactional
public void signal(SignalPayload signalPayload) {
//@TODO: define security policies for signalling
runtimeService.signalEventReceived(signalPayload.getName(),

This comment has been minimized.

@igdianov

igdianov Dec 18, 2018

Member

@daisuke-yoshimoto This code sends the same signal two times. The signalEventReceived method should not be invoked at all. Instead, the eventPublisher should simply call publishEvent(signalPayload) to broadcast the signal via Cloud Stream signalEvent destination

This comment has been minimized.

@daisuke-yoshimoto

daisuke-yoshimoto Dec 18, 2018

Member

@igdianov
Thanks for your comment.

I also think that it should be only once, but at the time of the first PR, it is the specification that salaboy decided.
Activiti/activiti-cloud-runtime-bundle-service#38

This comment has been minimized.

@daisuke-yoshimoto

daisuke-yoshimoto Dec 18, 2018

Member

@igdianov @salaboy
As a compromise, how about sending signals for local Runtime Bundle by only RuntimeService.signalEventReceived and sending signals for other Runtime Bundles via the Spring Cloud Stream?

This comment has been minimized.

@igdianov

igdianov Dec 18, 2018

Member

@daisuke-yoshimoto I do not understand the use case why do we need to send the signal via runtimeService Java Api and broadcast it via Cloud Stream. There will be side effects like a process instance with two signal catch events in your test. May be we do need to add a scope attribute to signal broadcast payload to distinguish between cloud and runtime bundle scopes?

@salaboy Can you clarify the requirements for signal broadcast in this use case?

This comment has been minimized.

@daisuke-yoshimoto

daisuke-yoshimoto Dec 18, 2018

Member

@igdianov
I think we needn't send the signal twice.
Simply, if it's a local Runtime Bundle, we can do it via runtimeService Java Api.
If it is another Runtime Bundle, it is necessary to propagate it via Spring Cloud Stream.

The problem is that it will run again on the local Runtime Bundle via the Spring Cloud Stream.

This comment has been minimized.

@daisuke-yoshimoto

daisuke-yoshimoto Dec 18, 2018

Member

@igdianov
Well, sorry, it certainly needs to consider process instance scope for Intermediate Signal ThrowEvent. Currently, signal of process instance scope also propagates to other runtime bundle.

https://github.com/Activiti/activiti-cloud-runtime-bundle-service/blob/develop/activiti-cloud-services-runtime-bundle/activiti-cloud-services-subscriptions/src/main/java/org/activiti/services/subscriptions/behavior/BroadcastSignalEventActivityBehavior.java#L31

This comment has been minimized.

@igdianov

igdianov Dec 18, 2018

Member

@daisuke-yoshimoto @salaboy This behaviour is not desirable. The Cloud signal event producer should not send process instance scoped signals outside of runtime bundle. Only globally scoped signals can be broadcasted via cloud stream.

Would it make sense to create and use unique destination for each signal name with runtime bundle and scope definition attributes? Signal event producers can resolve the destination for each signal to send the event message via cloud stream, while signal consumers can dynamically resolve and subscribe to signal sources to only receive signal events for process definitions signals inside runtime bundles.

Does it make any sense?

This comment has been minimized.

@salaboy

salaboy Dec 27, 2018

Member

@igdianov this does make sense.. in general what we were trying to achieve (and what we discussed with @daisuke-yoshimoto ) was:

  1. Signal method on the API should work with messages. meaning that you can send SignalPayload to the message driven APIs and it should work

  2. We discussed about scopes but we don't have that information right now, so we will need to define global (which before was "Engine" scope to mean multi Runtime Bundle. Local will now mean "inside a Runtime Bundle". finally Process Instance scope ... only to a process instance. But because we don't have this implemented, we tried to come up with some default behaviour for signals until we have these scopes defined somewhere.
    For now what I propose is that we provide a flag in the SignalPayload to mark the signal as global (or broadcast) to invoke the eventPublisher.publishEvent() to other runtime bundles. By default it should be local only.

  3. The Throw Event BPMN element should also provide the scope, we can do this as an extension, by default it should be local, but if the flag is true it should broadcast outside the runtime bundle.

@daisuke-yoshimoto @igdianov i think that is a simple change that we can merge now and then improve with a more concrete definition of the scopes.

This comment has been minimized.

@salaboy

salaboy Dec 27, 2018

Member

@igdianov regarding "unique destination for each signal name with runtime bundle" do we really need that? meaning that we can decide to which destination each RB is listening to for signals. Conceptually I was thinking about a single destination for all signals, but it is true that we might need to support a more flexible approach. The question is.. do we need that now?

This comment has been minimized.

@igdianov

igdianov Dec 27, 2018

Member

@salaboy We can deal with scopes later, I think. Please, see Activiti/activiti-cloud-runtime-bundle-service#171 (comment)

@@ -293,10 +298,12 @@ public void setVariables(SetProcessVariablesPayload setProcessVariablesPayload)
}

@Override
@Transactional
public void signal(SignalPayload signalPayload) {
//@TODO: define security policies for signalling
runtimeService.signalEventReceived(signalPayload.getName(),

This comment has been minimized.

@igdianov

igdianov Dec 18, 2018

Member

@daisuke-yoshimoto This code sends the same signal two times. The signalEventReceived method should not be invoked at all. Instead, the eventPublisher should simply call publishEvent(signalPayload) to broadcast the signal via Cloud Stream signalEvent destination

This comment has been minimized.

@daisuke-yoshimoto

daisuke-yoshimoto Dec 18, 2018

Member

@igdianov
Thanks for your comment.

I also think that it should be only once, but at the time of the first PR, it is the specification that salaboy decided.
Activiti/activiti-cloud-runtime-bundle-service#38

daisuke-yoshimoto added some commits Dec 22, 2018

@daisuke-yoshimoto

This comment has been minimized.

Copy link
Member

daisuke-yoshimoto commented Dec 23, 2018

@salaboy @igdianov Hi. Sorry I made you wait. Fixing signal 's PR is completed. Please review again.

igdianov and others added some commits Dec 24, 2018

Merge pull request #2309 from Activiti/igdianov-daisuke-2075-fix-broa…
…dcast-signal

Default SignalPayloadEventListener implemetation
@daisuke-yoshimoto

This comment has been minimized.

Copy link
Member

daisuke-yoshimoto commented Dec 29, 2018

@salaboy @igdianov
Since the fix is completed, could you review this pr again?

@salaboy salaboy merged commit 4204d92 into develop Dec 30, 2018

8 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch 66.66% of diff hit (target 60.93%)
Details
codecov/project 60.97% (+0.04%) compared to f1df4d3
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

@salaboy salaboy deleted the daisuke-2075-fix-broadcast-signal branch Dec 30, 2018

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