Update the default values of dynamic config to not depend on static config #4858
Conversation
Shaddoll
force-pushed
the
dynamic-static
branch
5 times, most recently
from
3f9fbd6 to
50b8f7b
Compare
Shaddoll
changed the title
common/archiver/archivalMetadata.go
Outdated
| @@ -55,7 +55,8 @@ type ( | |||
| archivalConfig struct { | |||
| staticClusterStatus ArchivalStatus | |||
| dynamicClusterStatus dynamicconfig.StringPropertyFn | |||
| enableRead dynamicconfig.BoolPropertyFn | |||
| staticEnableRead bool | |||
There was a problem hiding this comment.
I don't quite understand what these two values are intended to imply from the user of the config.
what does dynamicEnableRead == true and staticEnalbleRead mean, for example? Is this something you're intending to hide through a getter (I might have missed it if you have).
There was a problem hiding this comment.
yes, it's hided through ReadEnabled() bool. if both are true, read is enabled
common/archiver/archivalMetadata.go
Outdated
| @@ -177,7 +183,7 @@ func (a *archivalConfig) ReadEnabled() bool { | |||
| if !a.ClusterConfiguredForArchival() { | |||
| return false | |||
| } | |||
| return a.enableRead() | |||
| return a.staticEnableRead && a.dynamicEnableRead() | |||
There was a problem hiding this comment.
This configuration might be surprising as some folks might expect either || or if dynamicConfig != nil return dynamicConfig else staticConfig
There was a problem hiding this comment.
I dont have a strong opinion on what it should be as I don't know enough about how this and similar config values are used in production, but I think maybe we should call out this both in the head of the diff and maybe in comments?
There was a problem hiding this comment.
This is to keep the behavior the same as GetClusterStatus function
Shaddoll
force-pushed
the
dynamic-static
branch
3 times, most recently
from
6dc1a8b to
b05ee62
Compare
| @@ -117,7 +117,7 @@ func (cf *rpcClientFactory) NewHistoryClientWithTimeout(timeout time.Duration) ( | |||
| peerResolver := history.NewPeerResolver(cf.numberOfHistoryShards, cf.resolver, namedPort) | |||
|
|
|||
| supportedMessageSize := cf.rpcFactory.GetMaxMessageSize() | |||
| maxSizeConfig := cf.dynConfig.GetIntProperty(dynamicconfig.GRPCMaxSizeInByte, supportedMessageSize) | |||
| maxSizeConfig := cf.dynConfig.GetIntProperty(dynamicconfig.GRPCMaxSizeInByte, 4*1024*1024) | |||
There was a problem hiding this comment.
maybe could we put 4*1024*1024 as a constant too? it's not super clear to the ignorant reader (me) what this refers to?
There was a problem hiding this comment.
will be addressed in #4863
What changed?
Update the default values of dynamic config to be static values, and to not depend on values read from static config file
Why?
How did you test it?
existing test
Potential risks
Release notes
If there is no
system.grpcMaxSizeInByteconfig, it needs to be created. And the value should be the same as themaxPayloadSizein the static config.Documentation Changes