Restore memory queue's internal event cleanup after a batch is vended #41356
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
faec
added
bug
Team:Elastic-Agent-Data-Plane
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
botelastic
bot
added
needs_team
jlind23
approved these changes
Oct 22, 2024
jlind23
reviewed
Oct 22, 2024
pierrehilbert
requested review from
rdner
and removed request for
khushijain21
4 tasks
swiatekm
approved these changes
Oct 22, 2024
mauri870
reviewed
Oct 22, 2024
| @@ -127,6 +133,10 @@ func (b *mockQueueBatch) Entry(i int) queue.Entry { | |||
| return fmt.Sprintf("event %v", i) | |||
| } | |||
|
|
|||
| func (b *mockQueueBatch) FreeEntries() { | |||
| b.freeEntriesCalled++ | |||
There was a problem hiding this comment.
This can cause problems if the test code runs in multiple goroutines somehow, might not be a problem rn.
There was a problem hiding this comment.
True, though right now this struct is only used in a single synchronous call to newBatch.
Since the disk queue's main buffer isn't in memory, it isn't hit by this bug. Originally, using FreeEntries this way was a memory / GC optimization and the benchmarking focus was on the memory queue. We could implement FreeEntries in the disk queue also and probably get a moderate memory reduction, but it would require some refactoring in how the disk queue tracks acknowledgments. (I'd be happy to spend some time cleaning up the disk queue's ack handling but it's never been top priority :-) ) |
mergify bot
pushed a commit
that referenced
this pull request
Oct 22, 2024
…#41356) Fix #41355, where event data in the memory queue was not being freed when event batches were acknowledged, but only gradually as the queue buffer was overwritten by later events. This gave the same effect as if all beat instances, even low-volume ones, were running with a full / saturated event queue. The root cause, found by @swiatekm, is [this PR](#39584), an unrelated cleanup of old code that accidentally included one live call along with the deprecated ones. (There was an old `FreeEntries` hook in pipeline batches that was only used for deprecated shipper configs, but the cleanup also removed the `FreeEntries` call _inside_ the queue which was essential for releasing event memory.) (cherry picked from commit fdb912a)
mergify bot
pushed a commit
that referenced
this pull request
Oct 22, 2024
…#41356) Fix #41355, where event data in the memory queue was not being freed when event batches were acknowledged, but only gradually as the queue buffer was overwritten by later events. This gave the same effect as if all beat instances, even low-volume ones, were running with a full / saturated event queue. The root cause, found by @swiatekm, is [this PR](#39584), an unrelated cleanup of old code that accidentally included one live call along with the deprecated ones. (There was an old `FreeEntries` hook in pipeline batches that was only used for deprecated shipper configs, but the cleanup also removed the `FreeEntries` call _inside_ the queue which was essential for releasing event memory.) (cherry picked from commit fdb912a)
mergify bot
pushed a commit
that referenced
this pull request
Oct 22, 2024
…#41356) Fix #41355, where event data in the memory queue was not being freed when event batches were acknowledged, but only gradually as the queue buffer was overwritten by later events. This gave the same effect as if all beat instances, even low-volume ones, were running with a full / saturated event queue. The root cause, found by @swiatekm, is [this PR](#39584), an unrelated cleanup of old code that accidentally included one live call along with the deprecated ones. (There was an old `FreeEntries` hook in pipeline batches that was only used for deprecated shipper configs, but the cleanup also removed the `FreeEntries` call _inside_ the queue which was essential for releasing event memory.) (cherry picked from commit fdb912a)
Merged
6 tasks
faec
added a commit
that referenced
this pull request
Oct 22, 2024
…#41356) (#41364) Fix #41355, where event data in the memory queue was not being freed when event batches were acknowledged, but only gradually as the queue buffer was overwritten by later events. This gave the same effect as if all beat instances, even low-volume ones, were running with a full / saturated event queue. The root cause, found by @swiatekm, is [this PR](#39584), an unrelated cleanup of old code that accidentally included one live call along with the deprecated ones. (There was an old `FreeEntries` hook in pipeline batches that was only used for deprecated shipper configs, but the cleanup also removed the `FreeEntries` call _inside_ the queue which was essential for releasing event memory.) (cherry picked from commit fdb912a) Co-authored-by: Fae Charlton <[email protected]>
faec
added a commit
that referenced
this pull request
Oct 22, 2024
…#41356) (#41363) Fix #41355, where event data in the memory queue was not being freed when event batches were acknowledged, but only gradually as the queue buffer was overwritten by later events. This gave the same effect as if all beat instances, even low-volume ones, were running with a full / saturated event queue. The root cause, found by @swiatekm, is [this PR](#39584), an unrelated cleanup of old code that accidentally included one live call along with the deprecated ones. (There was an old `FreeEntries` hook in pipeline batches that was only used for deprecated shipper configs, but the cleanup also removed the `FreeEntries` call _inside_ the queue which was essential for releasing event memory.) (cherry picked from commit fdb912a) Co-authored-by: Fae Charlton <[email protected]>
faec
added a commit
that referenced
this pull request
Oct 22, 2024
…#41356) (#41362) Fix #41355, where event data in the memory queue was not being freed when event batches were acknowledged, but only gradually as the queue buffer was overwritten by later events. This gave the same effect as if all beat instances, even low-volume ones, were running with a full / saturated event queue. The root cause, found by @swiatekm, is [this PR](#39584), an unrelated cleanup of old code that accidentally included one live call along with the deprecated ones. (There was an old `FreeEntries` hook in pipeline batches that was only used for deprecated shipper configs, but the cleanup also removed the `FreeEntries` call _inside_ the queue which was essential for releasing event memory.) (cherry picked from commit fdb912a) Co-authored-by: Fae Charlton <[email protected]>
|
@faec can you create an issue describing the benchmark we would have needed to have caught this before release? |
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.
Fix #41355, where event data in the memory queue was not being freed when event batches were acknowledged, but only gradually as the queue buffer was overwritten by later events. This gave the same effect as if all beat instances, even low-volume ones, were running with a full / saturated event queue.
The root cause, found by @swiatekm, is this PR, an unrelated cleanup of old code that accidentally included one live call along with the deprecated ones. (There was an old
FreeEntrieshook in pipeline batches that was only used for deprecated shipper configs, but the cleanup also removed theFreeEntriescall inside the queue which was essential for releasing event memory.)Checklist
I have made corresponding changes to the documentationI have made corresponding change to the default configuration filesCHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.Related issues