Add dynamic config for EnableReadVisibilityFromOS #6560
Conversation
neil-xie
requested review from
Shaddoll,
davidporter-id-au,
shijiesheng,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| // Value type: Bool | ||
| // Default value: true | ||
| // Allowed filters: DomainName | ||
| EnableReadVisibilityFromOS |
There was a problem hiding this comment.
There are 3 bool flags which can be set independently. What if more than one is true or all is false? Behavior is not clear from config perspective. Ideally there should be one flag ReadVisibilityStoreName
There was a problem hiding this comment.
Yeah, that can help preventing adding more flags here. When advanced visibility is enabled, it will first read from the advanced visibility store (ES or OS/Pinot), ES always has the most priority. If all of them are false, it will fall back to read from DB.
https://github.com/cadence-workflow/cadence/blob/master/common/persistence/visibility_dual_manager.go#L318
There was a problem hiding this comment.
https://github.com/cadence-workflow/cadence/blob/master/common/persistence/visibility_triple_manager.go#L666
the triple manager has more cases
There was a problem hiding this comment.
Adding a new dynamicconfig to all the environments is also a lot of manual work. If it's feasible to switch to a single flag let's do that
What changed?
Add dynamic config for EnableReadVisibilityFromOS to control reading from ES/OS during OS migration to better collect metrics
Fixed a minor bug that only checks ES when receive list request has large page size
Why?
OpenSearch migration
How did you test it?
Config change
Potential risks
Release notes
Documentation Changes