Restricting sender queue size #1132
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
dougqh
commented
Dec 9, 2019
| this(api, new NoopMonitor(), disruptorSize, senderQueueSize, flushFrequencySeconds); | ||
| } | ||
|
|
||
| // DQH - TODO - Update the tests & remove this |
There was a problem hiding this comment.
I'll probably end-up taking out the TODO for now.
However, I do think we need to find a better way to construct test objects.
I'd suggest creating finer grained Config objects with a Builder that works for both the Config & the object.
dougqh
force-pushed
the
dougqh/bounded-sender-queue
branch
2 times, most recently
from
8f1ec67
to
c71cec5
Compare
tylerbenson
approved these changes
Dec 10, 2019
dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java
Show resolved
Hide resolved
To prevent unbounded memory consumption, restricting the size of the sender queue. Also, lowering the size of the Disruptor queue. Unfortunately, our choice of a ScheduledExecutorService makes this a bit difficult, since ScheduledExecutorService doesn't allow us to supply the queue. A bigger change is in-order but for now, this change restricts the queue size by introducing a Semaphore around the ScheduledExecutorService. In effort to making testing easier, I introduced Monitor.onFlush. This is used in the new slow response test which attempts to simulate a situation where the sending queue would back up.
Improving the reliability of the slow sender test. Still needs work, but to make this truly reliable, I'd need to add to the DDAgentWriter API. I'll probably do that, but I'm trying to start with sticking to the existing API.
dougqh
force-pushed
the
dougqh/bounded-sender-queue
branch
from
f4ed2e0
to
c614df7
Compare
Adding call to monitor.onFailedSend in case senderSemaphore acquisition is interrupted during shutdown
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.
To prevent unbounded memory consumption, restricting the size of the sender queue. Also, lowering the size of the Disruptor queue.
Unfortunately, our choice of a ScheduledExecutorService makes this a bit difficult, since ScheduledExecutorService doesn't allow us to supply the queue.
A bigger change is in-order but for now, this change restricts the queue size by introducing a Semaphore around the ScheduledExecutorService.
In effort to making testing easier, I introduced Monitor.onFlush. This is used in the new slow response test which attempts to simulate a situation where the sending queue would back up.
To my chagrin, the current test doesn't fail reliably when the queue is unbounded; however, it does fail.