-
Notifications
You must be signed in to change notification settings - Fork 556
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: support terminate end events #10442
Conversation
9cf4daa
to
f7aa26c
Compare
Note that the failing test case is not related to the changes. I can reproduce the failure also on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whew that was big one 😄 Nice work @saig0 🚀
I have a few questions and comments. But my main thing is what you mentioned in the PR.
The implementation doesn't follow the BPMN spec in one point: the flow scope that contains the terminate end event is not terminated but completed.
❌ I think we should reconsider this. In my opinion the downsides don't outweigh the positives by terminating instead of completing.
- You mention that this decision doesn't influence the core behavior. While this is true, it does cause a strange behavior for users willing to test their process using zeebe-process-test. For example, we have an assertion verifying if a process is terminated. For users it would be natural to verify this in the case of a terminate end event. However, this assertion will fail, because in reality Zeebe will complete the process.
- You mention it fits better with the existing implementation. I have no doubts that this is true and you will definitely know better than me 😄. The code will become more complex, but the behavior of the code would do what feels natural and is expected. I noticed I had a bit of trouble reviewing this PR already, because sometimes an element would be terminated, other times it would be completed. I understand how it works after the review, but it feels unpredictable, which is again something that consumers of the exported records may have trouble with.
- You mention it aligns better with C7. So let's improve it 😄
Please reconsider this approach.
❌ It would be cool if we can support this terminate end event in our randomised tests. This can be a separate issue which we can work on later.
...ne/src/test/java/io/camunda/zeebe/engine/processing/bpmn/activity/TerminateEndEventTest.java
Outdated
Show resolved
Hide resolved
engine/src/main/java/io/camunda/zeebe/engine/processing/bpmn/event/EndEventProcessor.java
Outdated
Show resolved
Hide resolved
engine/src/main/java/io/camunda/zeebe/engine/processing/bpmn/event/EndEventProcessor.java
Outdated
Show resolved
Hide resolved
engine/src/main/java/io/camunda/zeebe/engine/processing/bpmn/event/EndEventProcessor.java
Outdated
Show resolved
Hide resolved
engine/src/main/java/io/camunda/zeebe/engine/processing/bpmn/container/ProcessProcessor.java
Show resolved
Hide resolved
.multiInstance( | ||
multiInstance -> | ||
multiInstance | ||
.parallel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 I think we should also test the sequential multi instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍪
bors merge |
We discussed if the flow scope should be We decided to |
10442: feat: support terminate end events r=saig0 a=saig0 ## Description Add support for BPMN terminate end events. See #8789 (comment) on how the BPMN element should work. The implementation doesn't follow the BPMN spec in one point: the flow scope that contains the terminate end event is not terminated but completed. Reasoning: - The state of the flow scope is a detail that doesn't influence the core behavior. In both cases, the process instance should continue, for example, by taking the outgoing sequence flow. The difference is not visible to process participants but only when monitoring the process instance, for example, in Operate. - It fits better with the existing implementation. It would be a bigger effort to continue the process instance (e.g. taking the outgoing sequence flow) when the flow scope is terminated. As a result, we would end up in more complex code. - It aligns with the behavior of Camunda Platform 7. Side note: I implemented the major parts during a [Live Hacking session](https://www.twitch.tv/videos/1584245006). 🎥 ## Related issues closes #8789 Co-authored-by: Philipp Ossler <philipp.ossler@gmail.com>
Build failed: |
Bors merge |
10442: feat: support terminate end events r=saig0 a=saig0 ## Description Add support for BPMN terminate end events. See #8789 (comment) on how the BPMN element should work. The implementation doesn't follow the BPMN spec in one point: the flow scope that contains the terminate end event is not terminated but completed. Reasoning: - The state of the flow scope is a detail that doesn't influence the core behavior. In both cases, the process instance should continue, for example, by taking the outgoing sequence flow. The difference is not visible to process participants but only when monitoring the process instance, for example, in Operate. - It fits better with the existing implementation. It would be a bigger effort to continue the process instance (e.g. taking the outgoing sequence flow) when the flow scope is terminated. As a result, we would end up in more complex code. - It aligns with the behavior of Camunda Platform 7. Side note: I implemented the major parts during a [Live Hacking session](https://www.twitch.tv/videos/1584245006). 🎥 ## Related issues closes #8789 Co-authored-by: Philipp Ossler <philipp.ossler@gmail.com>
Build failed: |
bors merge |
10442: feat: support terminate end events r=saig0 a=saig0 ## Description Add support for BPMN terminate end events. See #8789 (comment) on how the BPMN element should work. The implementation doesn't follow the BPMN spec in one point: the flow scope that contains the terminate end event is not terminated but completed. Reasoning: - The state of the flow scope is a detail that doesn't influence the core behavior. In both cases, the process instance should continue, for example, by taking the outgoing sequence flow. The difference is not visible to process participants but only when monitoring the process instance, for example, in Operate. - It fits better with the existing implementation. It would be a bigger effort to continue the process instance (e.g. taking the outgoing sequence flow) when the flow scope is terminated. As a result, we would end up in more complex code. - It aligns with the behavior of Camunda Platform 7. Side note: I implemented the major parts during a [Live Hacking session](https://www.twitch.tv/videos/1584245006). 🎥 ## Related issues closes #8789 Co-authored-by: Philipp Ossler <philipp.ossler@gmail.com>
Build failed: |
bors merge |
10442: feat: support terminate end events r=saig0 a=saig0 ## Description Add support for BPMN terminate end events. See #8789 (comment) on how the BPMN element should work. The implementation doesn't follow the BPMN spec in one point: the flow scope that contains the terminate end event is not terminated but completed. Reasoning: - The state of the flow scope is a detail that doesn't influence the core behavior. In both cases, the process instance should continue, for example, by taking the outgoing sequence flow. The difference is not visible to process participants but only when monitoring the process instance, for example, in Operate. - It fits better with the existing implementation. It would be a bigger effort to continue the process instance (e.g. taking the outgoing sequence flow) when the flow scope is terminated. As a result, we would end up in more complex code. - It aligns with the behavior of Camunda Platform 7. Side note: I implemented the major parts during a [Live Hacking session](https://www.twitch.tv/videos/1584245006). 🎥 ## Related issues closes #8789 Co-authored-by: Philipp Ossler <philipp.ossler@gmail.com>
Build failed: |
bors merge |
10442: feat: support terminate end events r=saig0 a=saig0 ## Description Add support for BPMN terminate end events. See #8789 (comment) on how the BPMN element should work. The implementation doesn't follow the BPMN spec in one point: the flow scope that contains the terminate end event is not terminated but completed. Reasoning: - The state of the flow scope is a detail that doesn't influence the core behavior. In both cases, the process instance should continue, for example, by taking the outgoing sequence flow. The difference is not visible to process participants but only when monitoring the process instance, for example, in Operate. - It fits better with the existing implementation. It would be a bigger effort to continue the process instance (e.g. taking the outgoing sequence flow) when the flow scope is terminated. As a result, we would end up in more complex code. - It aligns with the behavior of Camunda Platform 7. Side note: I implemented the major parts during a [Live Hacking session](https://www.twitch.tv/videos/1584245006). 🎥 ## Related issues closes #8789 Co-authored-by: Philipp Ossler <philipp.ossler@gmail.com>
Build failed: |
bors merge |
10442: feat: support terminate end events r=saig0 a=saig0 ## Description Add support for BPMN terminate end events. See #8789 (comment) on how the BPMN element should work. The implementation doesn't follow the BPMN spec in one point: the flow scope that contains the terminate end event is not terminated but completed. Reasoning: - The state of the flow scope is a detail that doesn't influence the core behavior. In both cases, the process instance should continue, for example, by taking the outgoing sequence flow. The difference is not visible to process participants but only when monitoring the process instance, for example, in Operate. - It fits better with the existing implementation. It would be a bigger effort to continue the process instance (e.g. taking the outgoing sequence flow) when the flow scope is terminated. As a result, we would end up in more complex code. - It aligns with the behavior of Camunda Platform 7. Side note: I implemented the major parts during a [Live Hacking session](https://www.twitch.tv/videos/1584245006). 🎥 ## Related issues closes #8789 Co-authored-by: Philipp Ossler <philipp.ossler@gmail.com>
Build failed: |
bors merge |
10442: feat: support terminate end events r=saig0 a=saig0 ## Description Add support for BPMN terminate end events. See #8789 (comment) on how the BPMN element should work. The implementation doesn't follow the BPMN spec in one point: the flow scope that contains the terminate end event is not terminated but completed. Reasoning: - The state of the flow scope is a detail that doesn't influence the core behavior. In both cases, the process instance should continue, for example, by taking the outgoing sequence flow. The difference is not visible to process participants but only when monitoring the process instance, for example, in Operate. - It fits better with the existing implementation. It would be a bigger effort to continue the process instance (e.g. taking the outgoing sequence flow) when the flow scope is terminated. As a result, we would end up in more complex code. - It aligns with the behavior of Camunda Platform 7. Side note: I implemented the major parts during a [Live Hacking session](https://www.twitch.tv/videos/1584245006). 🎥 ## Related issues closes #8789 Co-authored-by: Philipp Ossler <philipp.ossler@gmail.com>
Build failed: |
Extend the end event builder to add a terminate event definition. The event definition morphs the event into a terminate end event.
If the process instance reach a terminate end event on the root scope then we should complete the process instance.
Extract the end event validation tests into a separate class. Morph the test cases in JUnit 5.
Morph the end event tests into parameterized tests for all supported and unsupported types of end events.
If the process instance reaches a terminate end event in the root scope then we should terminate all active element instances. After the element instances are terminated, we should complete the process instance.
Extend the executable model for end events to identify terminate end events. If the terminate end event is completed then we terminate all other element instances of the same flow scope.
The process instance reached a terminate end event in the process that terminated all element instances of the process. After all element instances are terminated, we complete the process instance. We check the current state of the process instance to distingulish the behavior of a regular cancallation of the process instance, or an interruption from a parent process instance.
If the process instance reaches a terminate end event in a subprocess then we should terminate all active element instances inside the subprocess. After the element instances are terminated, we should complete the subprocess and take the outgoing sequence flow. Other element instances outside the subprocess should not be terminated.
The process instance reached a terminate end event in the subprocess that terminated all element instances inside the subprocess. After all element instances are terminated, we complete the subprocess.
If the process instance reaches a terminate end event in an event subprocess then we should terminate all active element instances inside the event subprocess. After the element instances are terminated, we should complete the event subprocess. Other element instances outside the event subprocess should not be terminated.
The process instance reached a terminate end event in the event subprocess that terminated all element instances inside the event subprocess. After all element instances are terminated, we complete the event subprocess.
If the process instance reaches a terminate end event in a child process then we should terminate all active element instances of the child process. After the element instances are terminated, we should complete the child process instance and complete the call activity.
If the process instance reaches a terminate end event in a multi-instance subprocess then we should terminate all active element instances inside this subprocess. After the element instances are terminated, we should complete the subprocess. Other instances of the multi-instance should not be terminated.
186faea
to
1668ad1
Compare
bors merge |
10442: feat: support terminate end events r=saig0 a=saig0 ## Description Add support for BPMN terminate end events. See #8789 (comment) on how the BPMN element should work. The implementation doesn't follow the BPMN spec in one point: the flow scope that contains the terminate end event is not terminated but completed. Reasoning: - The state of the flow scope is a detail that doesn't influence the core behavior. In both cases, the process instance should continue, for example, by taking the outgoing sequence flow. The difference is not visible to process participants but only when monitoring the process instance, for example, in Operate. - It fits better with the existing implementation. It would be a bigger effort to continue the process instance (e.g. taking the outgoing sequence flow) when the flow scope is terminated. As a result, we would end up in more complex code. - It aligns with the behavior of Camunda Platform 7. Side note: I implemented the major parts during a [Live Hacking session](https://www.twitch.tv/videos/1584245006). 🎥 ## Related issues closes #8789 10443: Do not take a backup if it already exists r=deepthidevaki a=deepthidevaki ## Description After restore, the log is truncated to the checkpoint position. So the checkpoint record is processed again and will trigger a new backup with the same Id of the backup it restored from. With this PR, `BackupService` handles this case gracefully. In addition, we also do not take a new backup if existing backup is failed or in progress. Alternatively, we can delete this backup and take a new one. But chances of it happening (i.e triggering a new backup when one already is in progress/failed) is very low. So we can keep this simple. ## Related issues closes #10430 Co-authored-by: Philipp Ossler <philipp.ossler@gmail.com> Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>
Build failed (retrying...): |
Build succeeded: |
Description
Add support for BPMN terminate end events. See #8789 (comment) on how the BPMN element should work.
The implementation doesn't follow the BPMN spec in one point: the flow scope that contains the terminate end event is not terminated but completed. Reasoning:
Side note: I implemented the major parts during a Live Hacking session. 🎥
Related issues
closes #8789
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/1.3
) to the PR, in case that fails you need to create backports manually.Testing:
Documentation:
Please refer to our review guidelines.