Add API to probe the logstream batch writer if more bytes can be written without writing them #8798
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
Adds a new public API method to the Dispatcher, `#canClaimFragmentBatch(int, int)`. This method determines whether the batch with the given fragment count and of the given length can actually be claimed for this dispatcher instance. This method is mostly useful to determine if something will be claimable before you wish to claim it, mostly to avoid having to abort your claim unnecessarily if the batch isn't finished yet when calling this method.
Introduces a new API to the `LogStreamBatchWriter` API, `#canWriteAdditionalEvent(int)`. This API lets one probe whether a batch including an additional event of the given size could be written. While this could be done via tryWrite, there are cases, such as with the job activation, where you want to know if adding data to a record (not to the writer, but the record) will cause it to be unwritable, at which point you want to handle it differently. Rolling back changes to the record itself is more complicated than to the writer, so as a compromise you can probe the writer to know if it will be able to write more bytes before writing them, even, as in our example, to your DTO.
10 tasks
|
Looking forward to talk about tests 😄 I'm not very satisfied with the tests I added, especially in the dispatcher, but nothing came to mind without essentially reusing the implementation in the tests 😵 |
9 tasks
saig0
approved these changes
Feb 17, 2022
dispatcher/src/test/java/io/camunda/zeebe/dispatcher/DispatcherTest.java
Outdated
Show resolved
Hide resolved
Adds a common case test for `canClaimFragmentBatch`. The idea is that the previous test was testing a corner case - i.e. when the given batch length fits the max fragment length, but will not when it's framed and aligned.
|
I also took the opportunity to update the |
|
bors merge |
ghost
pushed a commit
that referenced
this pull request
Feb 17, 2022
8798: Add API to probe the logstream batch writer if more bytes can be written without writing them r=npepinpe a=npepinpe ## Description This PR adds a new API method, `LogStreamBatchWriter#canWriteAdditionalEvent(int)`. This allows users of the writer to probe if adding the given amount of bytes to the batch would cause it to become un-writable, without actually having to write anything to the batch, or even modify their DTO (e.g. the `TypedRecord<?>` in the engine). To avoid having dispatcher details leak into the implementation, an analogous method is added to the dispatcher, `Dispatcher#canClaimFragmentBatch(int, int)`, which will compare the given size, framed and aligned, with the max fragment length. This is the main building block to eventually solve #5525, and enable other use cases (e.g. multi-instance creation) which deal with large batches until we have a more permanent solution (e.g. chunking follow up batches). NOTE: the tests added in the dispatcher are not very good, but I couldn't come up with something else that wouldn't be too coupled to the implementation (i.e. essentially reusing `LogBufferAppender`). I would like some ideas/suggestions. NOTE: this PR comes out of the larger one, #8491. You can check that one out to see how the new API would be used, e.g. in the `JobBatchCollector`. As such, this is marked for backporting, since we'll backport the complete fix for #5525. ## Related issues related to #5525 Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
|
Build failed: |
|
bors r+ ES tests were flaky, which incidentally is my next issue to work on 😄 |
|
Build succeeded:
|
ghost
merged commit 
|
Successfully created backport PR #8809 for |
|
Successfully created backport PR #8810 for |
ghost
pushed a commit
that referenced
this pull request
Feb 23, 2022
8799: Correctly truncate a job activation batch if it will not fit in the dispatcher r=npepinpe a=npepinpe ## Description This PR fixes a bug where at times, we would try to activate a job batch which was too big and could not be written into the dispatcher, as when it was framed and aligned, its size would exceed the `maxFragmentLength`. The fix itself is simple, and it was to delegate, all the way down to the dispatcher (with layers adding their own framing in between), the responsibility to decide whether more jobs can be added to the batch or not. To simplify the code and testing, the responsibility of collecting jobs into a batch was extracted into a specific class. The new API, `TypedEventWriter#canWriteEventOfLength(int)` could potentially be reused in other places where we want to prevent writing batches that are too large. However, this is a bit brittle, since it requires the code in the engine to properly compute beforehand how the record will grow when you modify it. For example, in the `JobBatchCollector`, to calculate the length of the `TypedRecord<JobBatchRecordValue> record` when adding a given `JobRecordValue` to it, we need the following: ```java // the expected length is based on the current record's length plus the length of the job // record we would add to the batch, the number of bytes taken by the additional job key, // as well as one byte required per job key for its type header. if we ever add more, // should be updated accordingly. final var jobRecordLength = jobRecord.getLength(); final var expectedEventLength = record.getLength() + jobRecordLength + Long.BYTES + 1; ``` Not extremely complicated, but could be error prone. A better solution, chunking the follow up events, is not something we can sanely do in between KRs, unfortunately, and would most likely carry a high risk, so wouldn't be anything we can backport. For now, I think this is an OK compromise. NOTE: this PR depends on #8797 and #8798. Wait for these to be merged before reviewing! ## Related issues closes #5525 Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
Zelldon
added
Release: 1.2.11
version:1.3.5
This pull request was closed.
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
This PR adds a new API method,
LogStreamBatchWriter#canWriteAdditionalEvent(int). This allows users of the writer to probe if adding the given amount of bytes to the batch would cause it to become un-writable, without actually having to write anything to the batch, or even modify their DTO (e.g. theTypedRecord<?>in the engine).To avoid having dispatcher details leak into the implementation, an analogous method is added to the dispatcher,
Dispatcher#canClaimFragmentBatch(int, int), which will compare the given size, framed and aligned, with the max fragment length. This is the main building block to eventually solve #5525, and enable other use cases (e.g. multi-instance creation) which deal with large batches until we have a more permanent solution (e.g. chunking follow up batches).NOTE: the tests added in the dispatcher are not very good, but I couldn't come up with something else that wouldn't be too coupled to the implementation (i.e. essentially reusing
LogBufferAppender). I would like some ideas/suggestions.NOTE: this PR comes out of the larger one, #8491. You can check that one out to see how the new API would be used, e.g. in the
JobBatchCollector. As such, this is marked for backporting, since we'll backport the complete fix for #5525.Related issues
related to #5525
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: