Skip to content
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

Document new process instance migration restrictions #4194

Closed

Conversation

korthout
Copy link
Member

Description

In 8.6, process instance migration supports the mapping of boundary events. While this adds a whole new level of migration capabilities, we've also had to add some additional restrictions to avoid the migration from adjusting the process instance to a state where it cannot continue execution correctly.

See:

This Pull Request adjusts both the gRPC API documentation as well as the Concept documentation to allow mapping boundary events.

No example has been added to the Concept documentation yet that showcases the behavior and use case of mapping a boundary event during process instance migration. This will be done later, probably together with the addition of the timer boundary event support.

closes #4186

When should this change go live?

  • This is a bug fix, security concern, or something that needs urgent release support.
  • This is already available but undocumented and should be released within a week.
  • This on a specific schedule and the assignee will coordinate a release with the DevEx team. (apply hold label or convert to draft PR)
  • This is part of a scheduled alpha or minor. (apply alpha or minor label)
  • There is no urgency with this change and can be released at any time.

PR Checklist

  • My changes are for an already released minor and are in /versioned_docs directory.
  • My changes are for the next minor and are in /docs directory (aka /next/).

With 8.6, instance migration will have a few more error cases to ensure
process instances are always migrated correctly.
With 8.6, instance migration will have a few more limitations to ensure
process instances are always migrated correctly.
Boundary events can now be mapped in instance migrations. This results
in migrating the associated subscription.

While this change adjusts the limitations and notes that this mapping
was not supported, it does not yet provide an example to showcase the
behavior.
@korthout korthout added the 8.6.0-alpha5 September 2024 alpha release label Aug 26, 2024
@korthout korthout requested a review from berkaycanbc August 26, 2024 10:28
Copy link
Contributor

👋 🤖 🤔 Hello! Did you make your changes in all the right places?

These files were changed only in docs/. You might want to duplicate these changes in versioned_docs/version-8.5/.

  • docs/apis-tools/zeebe-api/gateway-service.md
  • docs/components/concepts/process-instance-migration.md

You may have done this intentionally, but we wanted to point it out in case you didn't. You can read more about the versioning within our docs in our documentation guidelines.

@akeller akeller requested a review from a team August 26, 2024 15:33
Copy link
Contributor

@berkaycanbc berkaycanbc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@korthout this is a very valuable update in our migration docs, thanks! 🚀

I requested some changes to better clarify current limitations, please take a look. 👀

@@ -227,6 +223,8 @@ In the following cases, the process instance can't apply the migration plan and
- The migration plan can only map each `sourceElementId` once.
- A mapping instruction's `sourceElementId` must refer to an element existing in the process instance's process definition.
- A mapping instruction's `targetElementId` must refer to an element existing in the target process definition.
- A mapping instruction cannot detach a boundary event from an active element.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 We can be more explicit here: "A mapping instruction provided between boundary events cannot detach a boundary event from an active element."

@@ -227,6 +223,8 @@ In the following cases, the process instance can't apply the migration plan and
- The migration plan can only map each `sourceElementId` once.
- A mapping instruction's `sourceElementId` must refer to an element existing in the process instance's process definition.
- A mapping instruction's `targetElementId` must refer to an element existing in the target process definition.
- A mapping instruction cannot detach a boundary event from an active element.
- Each boundary event can only be the target of a mapping instruction once.
Copy link
Contributor

@berkaycanbc berkaycanbc Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ This sentence is too implicit to understand. An alternative would be: Multiple boundary events in the source cannot be mapped to the same boundary event in the target.


First migrate the process instance to a process model where the element is without a message boundary event.
Once the process instance is successfully migrated, you can migrate the process instance to a process model where the message boundary event is attached to the element again.
Currently, you cannot migrate an active element with a message boundary event attached to an element that also has a message boundary event attached if both the boundary events rely on the same message name and no mapping is provided between these boundary events.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ This sentence is hard to follow to the end. I think, we can simply add "without providing mapping instructions between boundary events" at the end of the old version. Then it becomes: Currently, you cannot migrate an active element with a message boundary event attached to an element that also has a message boundary event attached with the same message name without providing mapping instructions between boundary events.. I am also open for other simpler explanations.

@korthout korthout added the hold This issue is parked, do not merge. label Aug 29, 2024
@korthout
Copy link
Member Author

Thanks all for the reviews. I've come the conclusion that the restrictions need to be generalized anyways. They don't just apply to boundary events. I've marked the PR on hold for now.

@christinaausley
Copy link
Contributor

@korthout I will be out for two weeks after tomorrow, so feel free to loop in @camunda/tech-writers for additional review when you're ready 👍

@mesellings
Copy link
Contributor

@korthout Did you need this reviewed and merged for the alpha release next week, or is this on hold beyond that?

@korthout
Copy link
Member Author

korthout commented Sep 9, 2024

Hi @mesellings 👋

This is on hold as we were discovering that the rule is wider than just boundary events. Rather they concern all catch events that active elements are subscribed to. While the functionality is now available in alpha5, I don't see a use in getting this merged as is. So, let's hold off on merging until we've adjusted the documentation accordingly.

cc @berkaycanbc

@mesellings
Copy link
Contributor

@korthout Thanks for the clarification - we'll hold off for alpha 5, and will perform a review once the docs are adjusted 👍

@conceptualshark conceptualshark added target:8.6 Issues included in the 8.6 release and removed 8.6.0-alpha5 September 2024 alpha release labels Sep 10, 2024
@christinaausley
Copy link
Contributor

@korthout would you like this to go out with release next week, or is this still on hold?

@korthout
Copy link
Member Author

korthout commented Oct 4, 2024

@christinaausley Thanks for reminding me. This is superseded by:

@korthout korthout closed this Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hold This issue is parked, do not merge. target:8.6 Issues included in the 8.6 release
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Document new process instance migration restrictions for boundary events
5 participants