derive Spanner default gRPC connection count from GOMAXPROCS #1622
Conversation
vroldanbet
changed the title
| @@ -37,14 +37,17 @@ const ( | |||
| type Option func(*spannerOptions) | |||
|
|
|||
| func generateConfig(options []Option) (spannerOptions, error) { | |||
| // originally SpiceDB didn't use connection pools for Spanner SDK, so it opened 1 single connection | |||
| // This determines if there are more CPU cores to increase the default number of connections | |||
| defaultNumberConnections := max(1, math.Round(float64(runtime.GOMAXPROCS(0)/2.0))) | |||
There was a problem hiding this comment.
Did you set to maxprocs/2 to leave more room for non-spanner work?
The docs say:
Increasing the size of the session pool beyond the maximum number of threads that a single application instance can handle is not recommended. Instead, increase the number of application instances.
So I would think just setting to GOMAXPROCs is a fine default. (This quote says session pool but from context they mean connection count)
There was a problem hiding this comment.
Did you set to maxprocs/2 to leave more room for non-spanner work?
Because I don't know if folks are running lots of replicas and this could open a whole lot of connections to Spanner, so tried to be more conservative. I feel like GFE should be able to handle it but 🤷🏻
So I would think just setting to GOMAXPROCs is a fine default. (This quote says session pool but from context they mean connection count)
Yep! I agree that's the recommendation, it's just not sure how it's going to affect existing deployments
Updated it to use GOMAXPROCS
at some point when it was moved to connection pools, we set the default to 4, but before it used pools, it was using just one connection, so this reverts that regression on the defaults when the pool was introduced.
vroldanbet
force-pushed
the
spanner-conn-pool-count
branch
from
f09418b
to
95d4e35
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
at some point when it was moved to connection pools, we set the default to 4, but before it used pools, it was using just one connection, so this reverts that regression on the defaults when the pool was introduced.