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.

Sign up for GitHub

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

Jump to bottom

fix: never drop responses during batch processing #11865

Merged
4 commits merged into from
Mar 1, 2023
Merged

fix: never drop responses during batch processing #11865

4 commits merged into from
Mar 1, 2023

Conversation

lenaschoenburg
Copy link
Member

Description

During batch processing the stream processor now accumulates multiple responses and sends them out together.
Keeping only one response was not correct in some situations, for example when using CreateProcessInstanceWithResponse when the process ends with a service task or message event.

Related issues

closes #11848

@lenaschoenburg
test: should send multiple responses during batch processing
10b9a53
@lenaschoenburg
refactor: drop fake return value from response sending
b4e0e05 
Since we never returned 'false' from `tryWriteResponse`, the retry
logic was useless. This changes the return type to void to indicate
that we can't tell if sending the response was successful or not.
@lenaschoenburg
fix: buffer multiple responses on batch processing
64bd01c 
During batch processing, multiple responses can accumulate, for example
when using create process instance with result.
Previously, this was not handled correctly. The last response was the
only one being sent out. Now we buffer all responses and send them all
out together.
@ChrisKujawa ChrisKujawa self-requested a review March 1, 2023 10:11
@lenaschoenburg lenaschoenburg force-pushed the oz-fiz-response-dropping branch from 28cec90 to 11cf8b4 Compare March 1, 2023 10:20
ChrisKujawa
ChrisKujawa approved these changes Mar 1, 2023
View reviewed changes
Copy link
Member

@ChrisKujawa ChrisKujawa left a comment

Choose a reason for hiding this comment

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

🚄

bors r+

@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2023

Test Results

   997 files  ±    0     997 suites  ±0   1h 48m 15s ⏱️ + 9m 37s
7 953 tests +208  7 943 ✔️ +208  10 💤 ±0  0 ❌ ±0 
8 149 runs  +208  8 139 ✔️ +208  10 💤 ±0  0 ❌ ±0 

Results for commit 11cf8b4. ± Comparison against base commit c757d7b.

This pull request removes 412 and adds 620 tests. Note that renamed tests count towards both. 
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[56] collection=[Left[[B@24c66ec5]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[57] collection=[Right[[B@24c66ec5], Left[[B@24c66ec5]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[58] collection=[Left[[B@24c66ec5], Right[[B@24c66ec5]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[59] collection=[Left[[B@24c66ec5], Left[[B@24c66ec5]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[60] collection=[Right[[B@24c66ec5], Right[[B@24c66ec5], Left[[B@24c66ec5]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[61] collection=[Right[[B@24c66ec5], Left[[B@24c66ec5], Right[[B@24c66ec5]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[62] collection=[Right[[B@24c66ec5], Left[[B@24c66ec5], Left[[B@24c66ec5]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[63] collection=[Left[[B@24c66ec5], Right[[B@24c66ec5], Right[[B@24c66ec5]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[64] collection=[Left[[B@24c66ec5], Right[[B@24c66ec5], Left[[B@24c66ec5]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[65] collection=[Left[[B@24c66ec5], Left[[B@24c66ec5], Right[[B@24c66ec5]]
…

EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[56] collection=[Left[[B@24839124]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[57] collection=[Right[[B@24839124], Left[[B@24839124]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[58] collection=[Left[[B@24839124], Right[[B@24839124]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[59] collection=[Left[[B@24839124], Left[[B@24839124]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[60] collection=[Right[[B@24839124], Right[[B@24839124], Left[[B@24839124]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[61] collection=[Right[[B@24839124], Left[[B@24839124], Right[[B@24839124]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[62] collection=[Right[[B@24839124], Left[[B@24839124], Left[[B@24839124]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[63] collection=[Left[[B@24839124], Right[[B@24839124], Right[[B@24839124]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[64] collection=[Left[[B@24839124], Right[[B@24839124], Left[[B@24839124]]
EitherTest Streams of Eithers can be collected using .collector() ‑ Only Streams with Lefts are collected into a Left[65] collection=[Left[[B@24839124], Left[[B@24839124], Right[[B@24839124]]
…

@ghost
Copy link

ghost commented Mar 1, 2023

Build succeeded:

@ghost ghost merged commit 42773d6 into main Mar 1, 2023
@ghost ghost deleted the oz-fiz-response-dropping branch March 1, 2023 10:43
@backport-action
Copy link
Collaborator

Backport failed for stable/8.1, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin stable/8.1
git worktree add -d .worktree/backport-11865-to-stable/8.1 origin/stable/8.1
cd .worktree/backport-11865-to-stable/8.1
git checkout -b backport-11865-to-stable/8.1
ancref=$(git merge-base c757d7b865a3ecd0e38f11869f82a07040d768a5 11cf8b466e25b8d65f9878a28a46b5572f9d5da4)
git cherry-pick -x $ancref..11cf8b466e25b8d65f9878a28a46b5572f9d5da4

@ChrisKujawa ChrisKujawa mentioned this pull request Mar 1, 2023
Merged
14 tasks
ghost pushed a commit that referenced this pull request Mar 1, 2023
@lenaschoenburg @ChrisKujawa
merge: #11867
b9ac819 
11867: [Backport 8.1] fix: never drop responses during batch processing r=oleschoenburg a=Zelldon

## Description

Backported #11865, had to fix several merge conflict related to structural changes which have been done on main, but not on 8.1

Had to fix another test, because this was removed on main, but still exists on 8.1 [test: adjust test case](aae1a80)

<!-- Please explain the changes you made here. -->

## Related issues

<!-- Which issues are closed by this PR or are related -->

closes #11848



Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
@lenaschoenburg lenaschoenburg mentioned this pull request Mar 2, 2023
Closed
github-merge-queue bot pushed a commit that referenced this pull request Aug 9, 2024
@OmranAbazid
improve(opensearch): display preview warning (#12173)
dac3007 
related to #11865
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisKujawa ChrisKujawa ChrisKujawa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Batch processing might drop responses
3 participants
@lenaschoenburg @backport-action @ChrisKujawa