Add grpc keep alive server and client config for server and client initialization #7483
Conversation
service/history/configs/config.go
Outdated
| // gRPC keep alive options | ||
| // If a client pings too frequently, terminate the connection. | ||
| KeepAliveMinTime dynamicconfig.DurationPropertyFn | ||
| // Allow pings even when there are no active streams (RPCs) | ||
| KeepAlivePermitWithoutStream dynamicconfig.BoolPropertyFn | ||
| // Close the connection if a client is idle. | ||
| KeepAliveMaxConnectionIdle dynamicconfig.DurationPropertyFn | ||
| // Close the connection if it is too old. | ||
| KeepAliveMaxConnectionAge dynamicconfig.DurationPropertyFn | ||
| // Additive period after MaxConnectionAge after which the connection will be forcibly closed. | ||
| KeepAliveMaxConnectionAgeGrace dynamicconfig.DurationPropertyFn | ||
| // Ping the client if it is idle to ensure the connection is still active. | ||
| KeepAliveTime dynamicconfig.DurationPropertyFn | ||
| // Wait for the ping ack before assuming the connection is dead. | ||
| KeepAliveTimeout dynamicconfig.DurationPropertyFn |
There was a problem hiding this comment.
Wouldn't it make more sense to put these in static config instead of dynamic config? They can't be changed after server startup, right? And existing RPC-related settings (ports, bind addresses) are already in static config.
There was a problem hiding this comment.
Yeah, I think it make sense. Shall I create a Config struct in history/service.go similar as this one:
temporal/service/frontend/service.go
Line 60 in 9ea4876
There was a problem hiding this comment.
That's dynamic config too. I was thinking of putting it in here: https://github.com/temporalio/temporal/blob/main/common/config/config.go#L90
There was a problem hiding this comment.
If we put it here, are we able to change the value without rebuild and deploy the image?
There was a problem hiding this comment.
In general, sure. It's just a yaml file.
If you mean cloud: yes, it's part of the server component though. I can point it out on slack.
common/config/config.go
Outdated
| func (rpc *RPC) GetKeepAliveServerParameters() keepalive.ServerParameters { | ||
| // 0 are treated as default values in grpc: https://github.com/grpc/grpc-go/blob/43a4a84abcd406230e7471062dd2ee5c7ba2485d/internal/transport/http2_server.go#L215-L225 | ||
| // the default config is same as grpc default config, same for the below client config and enforcement policy | ||
| defaultConfig := &keepalive.ServerParameters{ |
There was a problem hiding this comment.
why not
| defaultConfig := &keepalive.ServerParameters{ | |
| defaultConfig := keepalive.ServerParameters{ |
? no need for a pointer here
| @@ -107,6 +108,10 @@ type ( | |||
| // forwarded from HTTP to gRPC. Any value with a trailing * will match the prefix before | |||
| // the asterisk (eg. `x-internal-*`) | |||
| HTTPAdditionalForwardedHeaders []string `yaml:"httpAdditionalForwardedHeaders"` | |||
| // KeepAliveServerConfig parameters for the server | |||
There was a problem hiding this comment.
parameters for the server
Should this say for the server and clients? KeepAliveClientConfig is for clients, right?
Actually, I would say this needs more comments. My initial read was that the server settings for history would affect the history server, and then client settings for history would affect clients connecting to history. But actually I think the client settings for history affect the history service connecting to any other service as a client, right?
There was a problem hiding this comment.
Yeah, after reading code more carefully, you are right. And I think it does no make sense to configure this way. So I updated to code for the client config to be used by other service when initialize the client.
common/resource/fx.go
Outdated
| @@ -405,6 +405,7 @@ func RPCFactoryProvider( | |||
| if tracingStatsHandler != nil { | |||
| options = append(options, grpc.WithStatsHandler(tracingStatsHandler)) | |||
| } | |||
| options = append(options, grpc.WithKeepaliveParams(svcCfg.RPC.GetKeepAliveClientParameters())) | |||
There was a problem hiding this comment.
As far as I can tell, this affects remote connections also, right? Is that intended? Do we need to differentiate local vs remote?
There was a problem hiding this comment.
I updated the code so we get some flexibility to configure it separately.
xwduan
changed the title
What changed?
Why?
To support configuring grpc keep alive for service and client.
How did you test it?
Potential risks
Documentation
Is hotfix candidate?