Support inside properties for multi-instance completion condition

[lzgabel]

Description

The BPMN 2.0 specification defined the following properties of a multi-instance body instance:

• numberOfInstances
• numberOfActiveInstances
• numberOfCompletedInstances
• numberOfTerminatedInstances

This PR makes numberOfInstances and numberOfActiveInstances available for use in the completion condition expression. Although they are available in that expression, they do not exist as process variables. These properties take precedence over process variables with the same name.

Out of scope: numberOfCompletedInstances, numberOfTerminatedInstances
Out of scope: using these properties in other expressions

Related issues

closes #2893

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

• The changes are backwards compatibility with previous versions
• If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/0.25) to the PR, in case that fails you need to create backports manually.

Testing:

• There are unit/integration tests that verify all acceptance criterias of the issue
• New tests are written to ensure backwards compatibility with further versions
• The behavior is tested manually
• The change has been verified by a QA run
• The impact of the changes is verified by a benchmark

Documentation:

• The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
• New content is added to the release announcement

[lzgabel]

Hi @korthout , Can you take the time to review the code? :)

[korthout]

Hi @lzgabel Thanks for giving this a go 🚀 . The first steps are already great, but there are some problems specifically with the numberOfCompletedElementInstances and numberOfTerminatedElementInstances, which will require some effort to resolve.

Therefore, I recommend to extract the changes for those properties into a separate PR again. The properties numberOfElementInstances and numberOfActiveElementInstances are much more straightforward and we might be able to get those merged much quicker.

The main thing that is missing for those 2 properties are some test cases. Please have a look at MultiInstanceActivityTest.shouldSetLoopCounterVariable. We'll need similar tests for the new properties.

I've also tried to explain the problems with the completed and terminated instances, to give you the chance to already think a bit about how to solve those problems. I think we'll need to do some back and forth discussing of solution ideas there before actually implementing it.

[lzgabel]

Hi @korthout , I've removed the numberOfCompletedInstances and the numberOfTerminatedInstances from this PR. Please check this out. 🙇

[korthout]

Thanks for the changes! Starting to look great @lzgabel 🏗️

Please have a look at my comments. Like before, I'm using the emoji code again.

💭 I struggle with 1 part and that is the combination of completion condition and the numberOfActiveInstances. In the current form the numberOfActiveInstances variable is updated after the completion condition expression is evaluated. It seems better if we could do it the other way around. That would mean moving the updating of the numberOfActiveInstances variable to just before evaluation of the completion condition expression, but that leads to wrong changes if the instance doesn't fully complete (i.e. when an incident is raised in the meantime). I don't see ways to solve that problem yet, but I wanted to raise it. Perhaps you see any solutions here.

[lzgabel]

Hi @korthout , I've fixed all review comments, please check this out. 🙇

[korthout]

Thanks @lzgabel. I'm currently out sick but I'll have a new look at it soon.

[korthout]

Hi @lzgabel 🎖️ I've finally had some time to review this and I'm happy to see you've found a solution and improved the tests.

However, in the case of an incident the numberOfActiveInstances variable is now updated twice (first lowered and then reset back to its previous value), which becomes confusing.

After a discussion with @saig0, we came to the conclusion that our completion logic does not currently allow us to use variables for these properties. Instead, we would like to propose to change the implementation to use "hidden" variables:

• currently, expressions are evaluated in the ExpressionProcessor
• it uses an evaluation context (VariableStateEvaluationContext), which in turn uses the variables stored in the state
• instead of storing these properties as variables in the state, we can try to find a way to provide additional variables to the VariableStateEvaluationContext
• these "hidden" variables should then be controlled by the MultiInstanceBodyProcessor: numberOfInstances and numberOfActiveInstances.
• this would mean these variables don't actually exist as variables in zeebe, but are available to expressions when they are evaluated. This way, the completion condition expression can still access these variables.

🙇 I understand that this requires quite a rewrite of the changes you've made here, but it would improve things. We actually think that we'll need to add more such "hidden" variables in the future, and these can be first ones. 🚀

As always, please let me know if you have any questions or if I can help you in any way.

[korthout]

Hi @lzgabel Can I help unblock you? Please let me know if there's anything I can do.

[lzgabel]

Hi @korthout, Sorry for the delay caused by celebrating the Chinese New Year. I will continue this feature next week. 🙇

Happy Lunar New Year!

[korthout]

@lzgabel No worries whatsoever. Hope you enjoy the celebrations and happy lunar new year! 🎉

[lzgabel]

Hi @korthout, I'm very sorry for the delay, please check this out. 🙇

[korthout]

As always @lzgabel ❤️ Thanks so much for this.

I'm really liking this approach for the multi-instance properties and I think it's almost ready. There's 1 larger problem still left to resolve and then we need an additional test case for numberOfActiveInstances.

This will mark the start of an entirely new way of providing values to expressions (i.e. hidden variables). And once this is done, we should consider if there are other such variables we want to make available (e.g. processInstanceKey is a desired value to be used as the message event correlation key), and whether the multi-instance properties should also be available in input-output mappings of the multi-instance element. In any case, I think this is becoming an awesome feature! Thanks again for taking the time to build this 🚀

[CLAassistant]

All committers have signed the CLA.

[lzgabel]

Hi @korthout. I've fixed all comments, please check this out. 🙇

BTW. Seems we're having some problems with our CLA bot.

[korthout]

👍 Thanks @lzgabel I like the new changes and we're getting closer.

❌ I have a single change request because existing zeebe clusters should be able to update to a version with these changes in it. So we'll need to find a solution to avoid the breaking change.

💭 This has made me think about supporting existing process instances. Next to the change I've requested, we'll probably need to implement a MigrationTask. The task should provide a value for the childActivatedCounter for the existing ElementInstances of type Multi-Instance. If we would not do this, then the value would be incorrect for those multi-instances. The implemented MigrationTask should then be added to the DbMigrationImpl. Please let me know if you need help with that. If you feel comfortable with making those changes, then please do. Otherwise, I'd also be happy to contribute the implementation for this MigrationTask.

EDIT: please ignore the CLA assistant for now.

[korthout]

BTW. Seems we're having some problems with our CLA bot.

@lzgabel It seems our team has installed a new version that lost all previous signatures 😕 You'll have to sign it again. Sorry about that.

[lzgabel]

Hi @korthout. I've fixed all comments, please check this out. 🙇

[korthout]

Hi @lzgabel I'm sorry that I haven't been able to review this yet. We've been busy with the upcoming major release. I also have a few days off coming up, so I'll need to postpone this review to next week. I hope that's okay with you and again sorry for keeping you waiting.

[korthout]

Hi @lzgabel I finally got around to reviewing this. Please accept my apology for the delays 🙇 After my previous message I was on sick leave for some time. But I'm happy to be back 🎉

👏 Thanks again for these changes. It's really looking good and you've done a great job with the migration code.

❌ I've highlighted a few things that I encountered that we should fix before merging. Please have a look at them.

🔧 I also want to propose that you rebase your PR on top of #9118 (or wait until it is merged). This PR adds a similar construct as you've introduced that allows adding additional variable lookups to the expression processor. I would like to see that this PR uses it.

❓ Lastly, I imagine that it must be frustrating that this PR has been open for so long. I'm very happy to receive your contribution and I want to encourage you to continue. But I would also understand it if you'd like me to take this PR over from you. Once it's merged, we could focus on the next steps like the numberOfCompletedInstances and numberOfTerminatedInstances. Please let me know, whether you want to continue with it yourself or would prefer me to take it over.

As always thanks a lot for your efforts. They're greatly appreciated 🙇

[lzgabel]

Hi @korthout. Thanks for your reply. I want to continue with it.🧑🏻‍💻

[lzgabel]

Hi @korthout. Sorry for the long wait, i've rebased this changes on top of main, it may require you to re-review the code, sorry for the inconvenience, please check this out. 🙇

[korthout]

🚀 🚀 🚀 Great work @lzgabel! This looks awesome!

I have one final request and then we can merge. Last round, I promise 🙈 Please also consider my suggestions.

[lzgabel]

Hi @korthout. I've fixed all comments, please check this out. 🙇

[korthout]

LGTM 🚀

Thanks for all your efforts! We really appreciate it ❤️

PS: I've updated the PR description and title to reflect the current state

[korthout]

@lzgabel there appears to be a formatting error, please run mvn spotless:apply and just squash it.

[korthout]

bors merge

[zeebe-bors-camunda]

Build succeeded:

• Smoke tests on ubuntu-latest
• continuous-integration/jenkins/branch
• Smoke tests on windows-latest
• Smoke tests on macos-latest