Set dynamic config values and default search attribute cache as disabled #136
Conversation
cretz
force-pushed
the
dynamic-config
branch
from
f8006a1 to
97e8486
Compare
cretz
force-pushed
the
dynamic-config
branch
from
97e8486 to
195a1c2
Compare
cmd/temporalite/main.go
Outdated
| // If there is no config value for search attribute cache disabling, | ||
| // default it to true | ||
| if len(configVals[dynamicconfig.ForceSearchAttributesCacheRefreshOnRead]) == 0 { | ||
| configVals[dynamicconfig.ForceSearchAttributesCacheRefreshOnRead] = []dynamicconfig.ConstrainedValue{ | ||
| {Value: true}, | ||
| } | ||
| } |
There was a problem hiding this comment.
This further cements that Temporalite is not for production use.
I'd love to ship "production ready" defaults as much as possible. Do we think that disabling this cache will be a limiting factor in terms of performance? Trading a small amount of runtime performance for no race conditions or startup wait time sounds fine to me, though we could also consider only enabling this in the temporaltest package if the runtime cost is high.
There was a problem hiding this comment.
Also instead of setting a default here, I think we should probably do it inside temporalite.NewServer so that library users get the same behavior.
There was a problem hiding this comment.
I originally had the same concerns but thought Temporalite was going for "development defaults" even in the CLI. I would be happy only setting this in temporaltest. Let me confirm with @bergundy who wanted this defauted in the CLI.
There was a problem hiding this comment.
This makes me think that having the temporalite "yolo" command for common test setup (includes --ephemeral too) is starting to make sense.
There was a problem hiding this comment.
IMO Temporalite values go something like this
- User experience (for development use cases)
- Security
- Performance
- Simplicity
- Configurability
So I'm very happy to trade off some performance for a better development experience. We can always add a --production flag to swap around a few defaults and keep things simple.
Another thought is that we could add flags to register search attributes at startup like we do for namespaces. This would essentially be the best of both worlds since we'd be able to ensure that each of those SAs is registered synchronously, without disabling caching completely.
There was a problem hiding this comment.
Another thought is that we could add flags to register search attributes at startup like we do for namespaces. This would essentially be the best of both worlds since we'd be able to ensure that each of those SAs is registered synchronously, without disabling caching completely.
I'd rather users do this with the SDK so their tests can run against any server implementation without customization.
| return nil, fmt.Errorf("dynamic config value not in KEY=JSON_VAL format") | ||
| } | ||
| key := dynamicconfig.Key(keyVal[0]) | ||
| // We don't support constraints currently |
There was a problem hiding this comment.
Constraints sound nice... what are they and is there future work we should track to support them?
There was a problem hiding this comment.
I don't really know, I'd have to speak to the server team. My use case and most use cases I've seen don't have these constraints. We can block this PR for that if we really need to.
| // WithSearchAttributeCacheDisabled disables search attribute caching. This | ||
| // delegates to WithDynamicConfigValue. | ||
| func WithSearchAttributeCacheDisabled() ServerOption { | ||
| return WithDynamicConfigValue( | ||
| dynamicconfig.ForceSearchAttributesCacheRefreshOnRead, | ||
| []dynamicconfig.ConstrainedValue{{Value: true}}, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Commented above, but should we make this the default and instead add an option for enabling the cache? We could comment to the effect of changing this default if/when we are able to remove the waiting period after registering search attributes, and if you need stable behavior use WithDynamicConfigValue.
There was a problem hiding this comment.
I think it should be opt-in programmatically, like persistence-disabled, especially now that we are going to turn it off by default on the CLI. (will still use in temporaltest)
|
This lgtm if we turn off the SA cache in @cretz would you mind adding a note on dynamic config to the relevant section of the README as well? |
|
I think some schedule-to-close is too aggressive for Windows in some tests causing flakes. |
What changed?
Why?
For use with search attributes, the dev-only setting to disable cache is needed or there is a wait period before the search attributes are available for use.
This further cements that Temporalite is not for production use. This is the same config as dev docker compose at https://github.com/temporalio/docker-compose/blob/7c4f708c579cd735145e49816750ca4368ff5bf3/dynamicconfig/development-sql.yaml#L5. Any concerns here?