Migrate interrupting event sub marker #6813
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Zelldon
changed the title
|
Hey @saig0 I need your help here. I migrated the interrupting marker from element instance key to element id, this was something we discussed some weeks ago. This is necessary for migrating the event sub processes and to avoid state changes on the event triggering. My current issue is one failing test, which expects a incident instead of a rejection. I'm not sure whether it would be also fine to just have the rejection instead. The test in question is Related LOG |
|
@npepinpe question regarding the ProcessEvent, is it planned to write that event also for the other triggers ? Like message and timer? P.S.: This would reduce my duplication of code in the appliers. P.S.S: maybe makes sense to have for different catch event type a different intent instead of one. So instead of TRIGGERED we have, BOUNDARY_TRIGGERED, EVENT_SUB_TRIGGERED, START_EVENT_TRIGGERED, INTERMEDIATE_TRIGGERED then we could have different implementations of appliers IF we need them, like for the event sub process. |
I guess that the failure test is In the test case, an interrupting event-subprocess is triggered and the process instance waits in a service task inside the subprocess. It throws an error and expects that an incident is created because the error is not caught. In this case, we must create an incident. A rejection would not be visible to the user and would behave differently (i.e. endless retry because the job is not completed or failed). The behavior changed because of these changes: https://github.com/camunda-cloud/zeebe/pull/6813/files#diff-f7a8bbe8884d202edc2b5f5cad1912bd9c4018191689cea3ec8a167986ad6998L65-R75 |
|
Hey @saig0 thanks for your input. Yeah sorry I copied the wrong name😅🙈 yes I needed to change that because the marker is now set before the triggering and not after😅 Any idea on this? Do you have later sometime for this? |
It is not planned yet. Currently, it is not necessary because we have the data in the other records. If this would increase the maintainability then we could also introduce it for the other events. Another option would be to extract the common behavior into a class that is used by the different event appliers. For now, I suggest trying it without using the record for other events and keep it simple. We can revisit it when doing the clean up. |
Delete unused code.
Zelldon
force-pushed
the
zell-migrate-interrupting-event-sub
branch
from
e7d3542
to
c795ddf
Compare
Zelldon
commented
Apr 19, 2021
Comment on lines
+114
to
+117
| if (flowScopeElementInstance.isInterrupted() | ||
| && !flowScopeElementInstance | ||
| .getInterruptingElementId() | ||
| .equals(startEvent.getEventSubProcess())) { |
There was a problem hiding this comment.
This is now necessary because we mark the flow scope before the triggering
saig0
approved these changes
Apr 19, 2021
engine/src/main/java/io/zeebe/engine/state/appliers/EventSubProcessInterruptionMarker.java
Show resolved
Hide resolved
Moves the event sub process interruption marker to related EventAppliers, in order to have no longer state changes in the processing (updating the flow scope element instance). The interruption marker is changed from the element instance key to the corresponding element id, since this the available information we have in the applier. This change allows to migrate the event sub processes.
Zelldon
force-pushed
the
zell-migrate-interrupting-event-sub
branch
from
c795ddf
to
3f8a069
Compare
|
bors r+ |
|
Build succeeded: |
ghost
merged commit
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Who ever has time first @npepinpe @saig0 We need that for continuing on migration of event and embedded sub process
Related issues
related #6195
related #6196
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/0.25) to the PR, in case that fails you need to create backports manually.Testing:
Documentation: