fix: adjust rcmgr limits for accelerated DHT client rt refresh #8982
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
aschmahmann
reviewed
May 19, 2022
aschmahmann
reviewed
May 19, 2022
aschmahmann
reviewed
May 19, 2022
core/node/libp2p/rcmgr_defaults.go
Outdated
Comment on lines
44
to
45
| defaultLimits.SystemBaseLimit.ConnsOutbound = logScale(4 * maxconns) | ||
| defaultLimits.SystemBaseLimit.Conns = logScale(8 * maxconns) |
There was a problem hiding this comment.
I'm not sure that only doing this in the context of 2*HighWater > defaultLimits.SystemBaseLimit.ConnsInbound makes sense. Even if your HighWater is lower (e.g. 100 connections) you might still want to allow for spikiness when your application demands it (e.g. for the accelerated DHT client routing table refreshes).
There was a problem hiding this comment.
Do you have a suggestion here? I'm mostly focused on making sure this works for the default scenario, not for folks tweaking connmgr or rcmgr settings.
There was a problem hiding this comment.
What's the downside of having the default outbound limits just be hardcoded here rather than a function of highwater?
To some extent I guess the question is how much "magic" is worthwhile here? If we just hard code static numbers then it's easier for folks to understand at the cost of us not being able to give users a "reasonable" default config according to parameters they've already set (i.e. highwater)
3 tasks
|
2022-05-19 conversation:
Pros: |
guseggert
force-pushed
the
feat/rcmgr-dht-defaults
branch
from
cdb2a48
to
7520778
Compare
|
I saw this comment coment come through:
I'm concerned about the "I'm not sure which limit this is coming from though". Is this made clear in the logs? |
The Accelerated DHT client periodically refreshes its routing table, including at startup, and if Resource Manager throttling causes the client's routing table to be incomplete, then content routing may be degraded or broken for users. This adjusts the default limits to a level that empirically doesn't cause Resource Manager throttling during initial DHT client bootstrapping. Ideally the Accelerated DHT client would handle this scenario more gracefully, but this works for now to unblock the 0.13 release.
guseggert
force-pushed
the
feat/rcmgr-dht-defaults
branch
from
6381d0e
to
25372b0
Compare
aschmahmann
reviewed
Jun 1, 2022
This also sets the default overall conns as a function of the outbound and inbound conns, since they are adjusted dynamically, and it makes the intention of the value clear.
guseggert
force-pushed
the
feat/rcmgr-dht-defaults
branch
from
25372b0
to
5c7c88f
Compare
aschmahmann
approved these changes
Jun 1, 2022
aschmahmann
reviewed
Jun 1, 2022
guseggert
force-pushed
the
feat/rcmgr-dht-defaults
branch
from
ca4dd40
to
f26c96c
Compare
aschmahmann
approved these changes
Jun 2, 2022
guseggert
added a commit
that referenced
this pull request
Jun 8, 2022
* fix: adjust rcmgr limits for accelerated DHT client rt refresh The Accelerated DHT client periodically refreshes its routing table, including at startup, and if Resource Manager throttling causes the client's routing table to be incomplete, then content routing may be degraded or broken for users. This adjusts the default limits to a level that empirically doesn't cause Resource Manager throttling during initial DHT client bootstrapping. Ideally the Accelerated DHT client would handle this scenario more gracefully, but this works for now to unblock the 0.13 release. * Set default outbound conns unconditionally This also sets the default overall conns as a function of the outbound and inbound conns, since they are adjusted dynamically, and it makes the intention of the value clear. * increase min FD limit
guseggert
added a commit
that referenced
this pull request
Jun 8, 2022
* fix: adjust rcmgr limits for accelerated DHT client rt refresh The Accelerated DHT client periodically refreshes its routing table, including at startup, and if Resource Manager throttling causes the client's routing table to be incomplete, then content routing may be degraded or broken for users. This adjusts the default limits to a level that empirically doesn't cause Resource Manager throttling during initial DHT client bootstrapping. Ideally the Accelerated DHT client would handle this scenario more gracefully, but this works for now to unblock the 0.13 release. * Set default outbound conns unconditionally This also sets the default overall conns as a function of the outbound and inbound conns, since they are adjusted dynamically, and it makes the intention of the value clear. * increase min FD limit (cherry picked from commit b8617b9)
guseggert
added a commit
that referenced
this pull request
Jun 8, 2022
* fix: adjust rcmgr limits for accelerated DHT client rt refresh The Accelerated DHT client periodically refreshes its routing table, including at startup, and if Resource Manager throttling causes the client's routing table to be incomplete, then content routing may be degraded or broken for users. This adjusts the default limits to a level that empirically doesn't cause Resource Manager throttling during initial DHT client bootstrapping. Ideally the Accelerated DHT client would handle this scenario more gracefully, but this works for now to unblock the 0.13 release. * Set default outbound conns unconditionally This also sets the default overall conns as a function of the outbound and inbound conns, since they are adjusted dynamically, and it makes the intention of the value clear. * increase min FD limit (cherry picked from commit b8617b9)
guseggert
added a commit
that referenced
this pull request
Jun 8, 2022
* fix: adjust rcmgr limits for accelerated DHT client rt refresh The Accelerated DHT client periodically refreshes its routing table, including at startup, and if Resource Manager throttling causes the client's routing table to be incomplete, then content routing may be degraded or broken for users. This adjusts the default limits to a level that empirically doesn't cause Resource Manager throttling during initial DHT client bootstrapping. Ideally the Accelerated DHT client would handle this scenario more gracefully, but this works for now to unblock the 0.13 release. * Set default outbound conns unconditionally This also sets the default overall conns as a function of the outbound and inbound conns, since they are adjusted dynamically, and it makes the intention of the value clear. * increase min FD limit (cherry picked from commit b8617b9)
3 tasks
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.
The Accelerated DHT client periodically refreshes its routing table,
including at startup, and if Resource Manager throttling causes the
client's routing table to be incomplete, then content routing may be
degraded or broken for users.
This adjusts the default limits to a level that empirically doesn't
cause Resource Manager throttling during initial DHT client
bootstrapping. Ideally the Accelerated DHT client would handle this
scenario more gracefully, but this works for now to unblock the 0.13
release.