rpc: properly delay config initialization until server instantiation#109901
rpc: properly delay config initialization until server instantiation#109901craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
aliher1911
left a comment
There was a problem hiding this comment.
Some nits, don't want to block you from merging.
| var enableRPCCompression = envutil.EnvOrDefaultBool("COCKROACH_ENABLE_RPC_COMPRESSION", true) | ||
|
|
||
| func getWindowSize(name string, c ConnectionClass, defaultSize int) int32 { | ||
| func getWindowSize(ctx context.Context, name string, c ConnectionClass, defaultSize int) int32 { |
There was a problem hiding this comment.
Since we don't yet allow reconfiguration and compute the same value on every call maybe call out why we do this in a comment? Otherwise someone changing a caller may decide to cache the value indefinitely by mistake.
| s := envutil.EnvOrDefaultInt(name, defaultSize) | ||
| if s > maxWindowSize { | ||
| log.Warningf(context.Background(), "%s value too large; trimmed to %d", name, maxWindowSize) | ||
| log.Warningf(ctx, "%s value too large; trimmed to %d", name, maxWindowSize) |
There was a problem hiding this comment.
Looking at the places where this is called (grpcDialRaw which is gossip and dial backs for example) it could become quite noisy in the logs. Should we limit this by log.Every? That's not on critical path and even when we make this configurable we would see it at least on first connection and then periodically for the case where log rolls.
A recent refactor surfaced a misdesign that had existed in the RPC package for a while: the RPC window size parameters was based off global variables only computed once. The misdesign was surfaced by the introduction of a bug in this recent refactor: the code was made to log some messages in case of misconfiguration. The logging was happening at init-time, and thus too early. There are a couple of problems with this misdesign: - it prevents computing new window parameters off new values of env vars set after the process has started up. (This is important as we evolve towards a server architecture with delayed initialization.) - the associated logging did not properly output the log events to the right place, because it was not subject to the log config. - the associated logging did not use the right logging tags. This patch fixes this by delaying the computation to the point the RPC server is instantiated. (No release note because the logging-related bug was not included in a user-facing release yet.) Release note: None
34fe909 to
47f40bc
Compare
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @abarganier, @aliher1911, @cucaroach, @erikgrinaker, and @rhu713)
pkg/rpc/settings.go line 53 at r1 (raw file):
Previously, aliher1911 (Oleg) wrote…
Since we don't yet allow reconfiguration and compute the same value on every call maybe call out why we do this in a comment? Otherwise someone changing a caller may decide to cache the value indefinitely by mistake.
I think I'm ok caching this value for the lifetime of the rpc.Context. I've modified the PR to add a memoizer.
pkg/rpc/settings.go line 57 at r1 (raw file):
Previously, aliher1911 (Oleg) wrote…
Looking at the places where this is called (grpcDialRaw which is gossip and dial backs for example) it could become quite noisy in the logs. Should we limit this by
log.Every? That's not on critical path and even when we make this configurable we would see it at least on first connection and then periodically for the case where log rolls.
Good call - but by memoizing this now this is only logged once per rpc.Context.
|
TFYR! bors r=aliher1911 |
|
Build succeeded: |
Fixes #109900.
Epic: CRDB-28893
A recent refactor surfaced a misdesign that had existed in the RPC package for a while: the RPC window size parameters was based off global variables only computed once. The misdesign was surfaced by the introduction of a bug in this recent refactor: the code was made to log some messages in case of misconfiguration. The logging was happening at init-time, and thus too early.
There are a couple of problems with this misdesign:
it prevents computing new window parameters off new values of env vars set after the process has started up. (This is important as we evolve towards a server architecture with delayed initialization.)
the associated logging did not properly output the log events to the right place, because it was not subject to the log config.
the associated logging did not use the right logging tags.
This patch fixes this by delaying the computation to the point the RPC server is instantiated.
(No release note because the logging-related bug was not included in a user-facing release yet.)
Release note: None