Handle append error differently based on ResultBuilder #10191
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test Results 852 files ± 0 852 suites ±0 1h 39m 55s ⏱️ + 1m 11s For more details on these failures, see this check. Results for commit 0b9d865. ± Comparison against base commit ab0a90b. ♻️ This comment has been updated with latest results. |
10 tasks
deepthidevaki
approved these changes
Aug 26, 2022
| * | ||
| * @return returns either a failure or itself for chaining | ||
| */ | ||
| Either<RuntimeException, ProcessingResultBuilder> appendRecordReturnEither( |
There was a problem hiding this comment.
🔧 Suggestion - rename to tryAppendRecord
Return a boolean indicates whether the appending to the TaskResult was successful or not, this allows us to indicate whether the batch is full or not.
Fixes multiple bugs, where we appended records to a batch of records without checking our limits.
Zelldon
force-pushed
the
zell-task-success-append
branch
from
3f79291
to
0b9d865
Compare
The test didn't work if the appendRecord returns false (which is per default with mocked instances). Fix the test and add another test to verify that it stops to append.
|
bors r+ |
|
Build succeeded: |
zeebe-bors-camunda bot
added a commit
that referenced
this pull request
Aug 26, 2022
10196: Introduce ProcessingResponse r=Zelldon a=Zelldon ## Description blocked by #10191 ✅ blocked by #10188 ✅ Introduce the ProcessingResponse, which encapsulates the information of the Record which should be send as response on a user command and the request- and streamId which identifies the request. The ProcessingResponse usage replaces the usage of the CommandResponseWriter, and deletes several now unused code. <!-- Please explain the changes you made here. --> ## Related issues <!-- Which issues are closed by this PR or are related --> closes #10001 Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
❌ Blocked by #10188
This PR adds Either to the RecordBatch as return value. This allows to handle the error case differently. For example in the Processing right now we want to throw an exception, but later this can also handled differently which is why we added here a separate Method on the ProcessingResultBuilder. This is discussable whether we already want that.
What we need and want is to being able to handle failed appends in the consumers of the ProcessingScheduleService. Means at the TaskResultBuilder we want to now whether it was successful or not and just stop to append more records. This is slightly similar to how we did it before.
This fixes a critical bug #10147
Related issues
closes #10147
related #10001
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:
Please refer to our review guidelines.