-
Notifications
You must be signed in to change notification settings - Fork 1.8k
enhancement(sinks): Add support for max_bytes
for memory buffers
#23330
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.
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
enhancement(sinks): Add support for max_bytes
for memory buffers
#23330
Conversation
e3ef011
to
bdc9f1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds support for byte-based limits for in-memory buffers by introducing a unified MemoryBufferSize
enum and dynamically selecting between element-count and byte-size queues.
- Replace standalone
max_events
with asize
object backed byMemoryBufferSize
across configs and APIs - Implement
QueueImpl
to choose betweenArrayQueue
(by events) andSegQueue
(by bytes) using a semaphore guard - Update all tests, examples, benchmarks, and documentation to use the new byte/event buffer sizing model
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
website/cue/reference/components/base/sinks.cue | Update CUE schema to use new size object with max_bytes & max_size |
src/topology/test/backpressure.rs | Adapt backpressure tests to MemoryBufferSize |
src/test_util/mock/sources/basic.rs | Use MemoryBufferSize in mock sources |
src/source_sender/mod.rs | Initialize limited channel with MemoryBufferSize |
lib/vector-buffers/src/variants/in_memory.rs | Refactor MemoryBuffer to accept MemoryBufferSize |
lib/vector-buffers/src/topology/test_util.rs | Enhance Sample to account for heap-allocated bytes |
lib/vector-buffers/src/topology/channel/limited_queue.rs | Add QueueImpl , SizeTerms , & dynamic queue selection |
lib/vector-buffers/src/topology/builder.rs | Update topology builder to use MemoryBufferSize |
lib/vector-buffers/src/test/variant.rs | Update variant tests to use MemoryBufferSize |
lib/vector-buffers/src/lib.rs | Export MemoryBufferSize |
lib/vector-buffers/src/config.rs | Implement serde de/serialization for MemoryBufferSize |
lib/vector-buffers/examples/buffer_perf.rs | Adjust example to use new buffer size API |
lib/vector-buffers/benches/sized_records.rs | Add benchmarks for byte-based buffers via a BoundBy helper |
lib/vector-buffers/benches/common.rs | Extend Message to simulate heap allocation for size-based tests |
changelog.d/8679_add_support_max_bytes_memory_buffers.feature.md | Add changelog entry for the new max_bytes feature |
Comments suppressed due to low confidence (3)
lib/vector-buffers/src/config.rs:102
- [nitpick] The error message for invalid
max_bytes
is unclear and grammatically awkward; consider rephrasing to something like "max_bytes
must fit within the platform'susize
range" and include the actual bounds.
&"For memory buffers max_bytes expects an integer within the range of 268435488 and your architecture dependent usize",
lib/vector-buffers/src/config.rs:430
- Add a unit test in the config module to verify that a YAML or JSON config using
max_bytes
correctly deserializes intoMemoryBufferSize::MaxSize
underBufferType::Memory
.
for stage in self.stages() {
lib/vector-buffers/src/config.rs:207
- [nitpick] The variant name
MaxSize
is ambiguous; consider renaming it toMaxBytes
to clearly indicate it represents a byte-based limit.
MaxSize {
- Also removing its configurable_component tag as it is no longer officially part of the configuration
- Previous commit hash: 4354874
28f8661
to
2a7ddc9
Compare
- This makes the options more consistent with the existing ones like max_size
2a7ddc9
to
cde6698
Compare
cde6698
to
ee8e854
Compare
- When the flatten attribute was applied to a variant it would not be respected in the generated output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments around the memory buffer size enum variants, otherwise LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One issue with the sinks doc file, otherwise LGTM. I am approving it in advance of your resolution of that one, but please see if it can be fixed.
- This correctly identifies the attributes as having the bytes unit type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments, I still need to review the latter parts.
- Since the size is fixed and known at compile time this type is a better fit here then Vec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat!
…ectordotdev#23330) * New interface around buffers to select impl at runtime * Expose a non-fixed sized queue in vector-buffers * Modifications to make max_bytes configurable * Modify Sample to test limiting behavior on bytes allocated * Unit test for semaphore guarding SeqQueue * Include new SegQueue in buffering benchmarks * Generated documentation updates * Add changelog fragment * Implement QueueImpl trait directly on crossbeam queue types * Add helper method to reduce terseness * Convert config to flat layout * Modify MemoryBufferSize to be a tuple variant - Also removing its configurable_component tag as it is no longer officially part of the configuration * Update error message * Prefer size_of over magic numbers * Remove stray comment * Revert test behavior to use arbitrary u16s * Update documentation * Addressing some comments - Replace if let chain with match expression - Replace map/sum with just calls to + - Replace function pointer in limited_queue.rs with enum + variant check * Revert "Modify MemoryBufferSize to be a tuple variant" - Previous commit hash: f87aeab0 * Revert "Config config to flat layout" - Previous commit hash: 4354874 * Fix config bug where flatten option isn't respected for variants * Rename MemoryBufferSize variant options - This makes the options more consistent with the existing ones like max_size * Add unit test for parsing memory buffer config w/ byte_size * Update doc comments * Update generated documentation * Fix bug in config generator with flattening enum values - When the flatten attribute was applied to a variant it would not be respected in the generated output * Modify MemoryBufferSize to be a tuple variant * Updating generated documentation * Remove unnecessary serde attribute * Move MaxSizes configurable attribute within the tuple - This correctly identifies the attributes as having the bytes unit type * Modify _heap_allocated to be a heap allocated array - Since the size is fixed and known at compile time this type is a better fit here then Vec --------- Co-authored-by: Pavlos Rontidis <[email protected]>
Summary
This PR adds support for memory buffers to be bound in terms of bytes allocated. This is an opt-in feature which is defaulted to
false
, meaning that the current implementation and its defaults will still be selected when not explicitly supplying a value formax_bytes
.At the core of this change is a new interface that allowes the selection of a different lock-free queue. This queue is at the center of the implementation of memory buffers. Today that queue is
crossbeam_queue::ArrayQueue
which is a fixed-sized lock-free data structure. This queue being fixed size is the reason that #8679 could not easily be implemented. The new interface allows to drop in a non-fixed sized queue. Thecrossbeam_queue::SegQueue
was chosen, since it showed to be performant in initial testing and didn't require the inclusion of any new dependencies.The main resource (queue) is already guarded by a semaphore. This semaphore currently bounds the queue by number of elements but there's no reason for why it couldn't guard against bytes allocated, therefore much of that existing code remains the same - which is positive as it is already battle tested and seems relatively stable as is.
Finally a new unit test was added and new benchmarks included in
vector-buffers/benches
.Vector configuration
To any sink configuration try:
How did you test this PR?
Via the existing unit test and the developed benchmarks
Change Type
Is this a breaking change?
Does this PR include user facing changes?
no-changelog
label to this PR.References
max_bytes
for memory buffers #8679Notes
@vectordotdev/vector
to reach out to us regarding this PR.pre-push
hook, please see this template.cargo fmt --all
cargo clippy --workspace --all-targets -- -D warnings
cargo nextest run --workspace
(alternatively, you can runcargo test --all
)git merge origin master
andgit push
.Cargo.lock
), pleaserun
cargo vdev build licenses
to regenerate the license inventory and commit the changes (if any). More details here.