Use setTimeout instead of socket.setTimeout #23
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
We noticed unexpected linear increases in memcached command latency that greatly exceeded the configured timeout value. Digging in, we realized that the issue is that scoket.setTimeout only triggers the timeoutHandler if the socket has been idle for longer than the timeout. In our case, commands were piling up and we were running a huge queue because an issue increased the latency of each command but not enough to trigger the timeout. All of the commands were long past their deadlines when they were processed, but the socket was never idle long enough to trigger the deadline checking. This switches the command timeout to regular setTimeout since what the client cares about and wants to guard against is waiting longer than timeout for the command to complete. The fact that bits are flowing is irrelevant. This will cause commands to timeout properly and cause the queue to be flushed, reducing load and allowing new work to be prioritized over already dead work.
leon-root
approved these changes
Sep 30, 2024
|
@leon-root any reasons you think this will not help? Is it an explanation issue or do you have other concerns? |
Oh, I could not understand how it works before. |
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.
We noticed unexpected linear increases in memcached command latency that greatly exceeded the configured timeout value.
Digging in, we realized that the issue is that scoket.setTimeout only triggers the timeoutHandler if the socket has been idle for longer than the timeout. In our case, commands were piling up and we were running a huge queue because an issue increased the latency of each command but not enough to trigger the timeout. All of the commands were long past their deadlines when they were processed, but the socket was never idle long enough to trigger the deadline checking.
This switches the command timeout to regular setTimeout since what the client cares about and wants to guard against is waiting longer than timeout for the command to complete. The fact that bits are flowing is irrelevant. This will cause commands to timeout properly and cause the queue to be flushed, reducing load and allowing new work to be prioritized over already dead work.