Delay connection attempts without addresses.#64
Merged
fabianfett merged 1 commit intomainfrom Jun 19, 2023
Merged
Conversation
Member
Author
|
Oh we need to change this a tiny bit to not break API. |
Joannis
approved these changes
Jun 18, 2023
Member
Joannis
left a comment
There was a problem hiding this comment.
Great change! My main question, not necessarily for this PR, is if we can/should add an eventLoop. preconditionInEventLoop() inside the newly added mutable stored property.
In some circumstances users may have connection pools configured without any SocketAddresses ready to go. This is particularly likely in service discovery configurations. Right now, the effect of attempting to use such a pool is two fold. First, we'll emit a bunch of error level logs telling users we have no addresses. Second, we'll fall into the exponential backoff phase of connection establishment. The first property is annoying, but the second one is actively harmful. If your construction is timed incorrectly, we'll have the awkward problem of burning a bunch of CPU trying to create connections we know we cannot, and then a lengthy delay after the addresses are actually configured before we start trying to use them. That's the worst of all worlds. This patch adds logic to detect the attempt to create connections when we don't have any configured addresses and delays them. This path should improve performance and ergonomics when in this mode.
e404c74 to
921b983
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Cherry pick:
https://gitlab.com/swift-server-community/RediStack/-/merge_requests/147
Motivation
In some circumstances users may have connection pools configured without any SocketAddresses ready to go. This is particularly likely in service discovery configurations. Right now, the effect of attempting to use such a pool is two fold. First, we'll emit a bunch of error level logs telling users we have no addresses. Second, we'll fall into the exponential backoff phase of connection establishment.
The first property is annoying, but the second one is actively harmful. If your construction is timed incorrectly, we'll have the awkward problem of burning a bunch of CPU trying to create connections we know we cannot, and then a lengthy delay after the addresses are actually configured before we start trying to use them. That's the worst of all worlds.
This patch adds logic to detect the attempt to create connections when we don't have any configured addresses and delays them. This path should improve performance and ergonomics when in this mode.