Support large input collections for Multi-instance #12692
Conversation
remcowesterhoud
force-pushed
the
2890_large_input_collection
branch
from
bd361e6 to
44755c8
Compare
Verifies that a multi instance with a large input collection can activate all it's children without running out of batch size. The test also verifies that batching is used for the activating.
Improves the assertion message by explaining what the next steps are if it fails.
remcowesterhoud
force-pushed
the
2890_large_input_collection
branch
from
44755c8 to
b0b1264
Compare
There was a problem hiding this comment.
👍 Awesome reuse of the ProcessInstanceBatch record @remcowesterhoud.
The changes look good. I just left some optional feedback that shouldn't block you from merging this PR.
engine/src/main/java/io/camunda/zeebe/engine/processing/ProcessEventProcessors.java
Outdated
Show resolved
Hide resolved
| private void writeFollowupBatchCommand( | ||
| final ProcessInstanceBatchRecord recordValue, final long index) { | ||
| final var nextBatchRecord = | ||
| new ProcessInstanceBatchRecord() | ||
| .setProcessInstanceKey(recordValue.getProcessInstanceKey()) | ||
| .setBatchElementInstanceKey(recordValue.getBatchElementInstanceKey()) | ||
| .setIndex(index); | ||
| final long key = keyGenerator.nextKey(); | ||
| commandWriter.appendFollowUpCommand(key, ProcessInstanceBatchIntent.ACTIVATE, nextBatchRecord); | ||
| } | ||
|
|
||
| private boolean canWriteCommands( | ||
| final TypedRecord<ProcessInstanceBatchRecord> record, | ||
| final ProcessInstanceRecord childInstanceRecord) { | ||
| // We must have space in the batch to write both the ACTIVATE command as the potential | ||
| // follow-up batch command. An excessive 8Kb is added to account for metadata. This is way | ||
| // more than will be necessary. | ||
| final var expectedCommandLength = | ||
| record.getLength() + childInstanceRecord.getLength() + (1024 * 8); | ||
| return commandWriter.canWriteCommandOfLength(expectedCommandLength); | ||
| } | ||
| } |
There was a problem hiding this comment.
💭 I wonder if it makes sense to extract these two methods in a *Behavior class since there are two somewhat similar methods in the TerminateProcessInstanceBatchProcessor, and I assume we might have further use of the ProcessInstanceBatch record in the future, so we might duplicate these methods even more.
There was a problem hiding this comment.
I was thinking about this as well. Thing is, in the end it's only 2 lines of code. One of which is used to determine the expectedCommandLength. This is different for both of the processors we have now. Moving this to a behavior class would mean that we require a parameter getting the expectedCommandLength. So in the end all the behavior method will do is make a call to the command writer.
💭 I guess we could argue that the + 8KB is duplicated and could be extracted from the calculation. To be honest, at this time I don't really care. Let's see if we start using this command more often and how similar the other checks will become.
Adds the ActivateProcessInstanceBatchProcessor and registers it with the Engine. This processor is responsible to handle the ProcessInstanceBatch.ACTIVATE commands. It is used for activating child element instance of a multi instance element in a batch-like manner. The index in the command is used to indicate how much children need to be activated. If the amount of children will exceed the max message size a followup batch command is written. The index of the followup command is decremented for each of the activate commands that have been send.
A batch ACTIVATE operation works different from a TERMINATE batch operation. During a TERMINATE the index is the key of a child element instance. For ACTIVATE it is a simple counter, keeping track of how many ACTIVATE commands we still need to write.
Instead of writing all the ACTIVATE commands for all children at once, we now use the ProcessInstanceBatch.ACTIVATE command instead. This command takes an index, which in this case is the amount of children that an ACTIVATE command will be written for.
remcowesterhoud
force-pushed
the
2890_large_input_collection
branch
from
b0b1264 to
65988a2
Compare
|
bors merge |
|
Build succeeded: |
ghost
merged commit 
|
Successfully created backport PR for |
|
Successfully created backport PR for |
lenaschoenburg
added
the
version:8.2.5
lenaschoenburg
added
the
version:8.1.13
* chore(deps): update patch dependencies * Do not upgrade netty --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Matt Bear <[email protected]>
Description
This PR makes it so the child elements of a multi-instance are activated in batches. This will prevent the activation from exceeding the batch size.
The way it works is by writing
ProcessInstanceBatchRecord.ACTIVATEcommand. This command contains an index, which is the total amount of child elements that need to be activated.During the processing of this command we will check if another
ProcessInstance.ACTIVATEcommand will still fit in the batch. If it does we write and continue with the next one.If the command doesn't fit we will write a followup
ProcessInstanceBatchRecord.ACTIVATEcommand instead. In this command the index will be thetotal amount of children - the already activated amount of children.This keeps repeating until the index reaches 0. At this point we'll have written an
ACTIVATEcommand for all element of the input collection.Only back porting to 8.1 and 8.2, as these records are not available on 8.0
Related issues
closes #2890
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:
Other teams:
If the change impacts another team an issue has been created for this team, explaining what they need to do to support this change.
Please refer to our review guidelines.