WCP 2/X: Thread batchID through HistoryBuilder AddEvent methods#10412
WCP 2/X: Thread batchID through HistoryBuilder AddEvent methods#10412simvlad wants to merge 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb28d6b904
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if len(b.memLatestBatch) > 0 && b.memLatestBatchSize+eventSize > limit { | ||
| b.FlushAndCreateNewBatch() |
There was a problem hiding this comment.
When MaximumEventBatchSizeInBytes is enabled, this rollover can put a close event into a new batch after the caller has already guessed the batch's first event ID. The commit threads the returned batch ID for completed/failed/canceled workflows, but AddContinueAsNewEvent, AddTimeoutWorkflowEvent, and AddWorkflowExecutionTerminatedEvent still write CompletionEventBatchId from the precomputed firstEventID/WFT-failed ID; if the close event is split into its own batch, later completion-event loads use the wrong batch ID and can fail to find the close event. Please have those history-builder methods return the EventStore.add batch ID and apply that value too.
Useful? React with 👍 / 👎.
simvlad
changed the title
1 participant
What changed?
HistoryBuilder.Add*methods that previously returned just the event now also return thebatchID.MutableStateImplthreads thatbatchIDinto the correspondingApply*calls, which use it to populate the batchID back reference set on the event fields.This is stacked on top of #10357.
Why?
#10357 adds batch splitting in EventStore based on the size, which results in looser guarantees as to what batchId the event falls into. Previously these fields were set to
workflowTaskCompletedEventID, which only happens to equal the new event's batch ID when the new event ends up in the same batch as the WorkflowTaskCompleted event.How did you test it?
Potential risks
[Very low risk] The risk with this particular change is that in some cases we actually split the event with the WorkflowTaskCompleted event batches and we happen to expect WorkflowTaskCompleted present there instead of the event. This is very unlikely, since WorkflowTaskCompleted event ID is already set on some events and the naming such as ScheduledEventBatchId suggests the opposite.