Refactor: not require db visibility when ES visibility is provided #4241
Conversation
longquanzheng
changed the title
common/config/persistence.go
Outdated
| @@ -39,8 +39,20 @@ func (c *Persistence) DefaultStoreType() string { | |||
|
|
|||
| // Validate validates the persistence config | |||
| func (c *Persistence) Validate() error { | |||
| stores := []string{c.DefaultStore, c.VisibilityStore} | |||
| for _, st := range stores { | |||
| if _, ok := c.DataStores[c.DefaultStore]; !ok { | |||
There was a problem hiding this comment.
nit: line 42 - 44 is already covered in the for loop
There was a problem hiding this comment.
Removed. Good catch, thanks!
github-actions
bot
force-pushed
the
qlong-visibility-require
branch
from
eddfebd
to
44f3210
Compare
common/dynamicconfig/resource.go
Outdated
|
|
||
| type ( | ||
| // ResourceConfig is a subset of the service dynamic config | ||
| ResourceConfig struct { |
There was a problem hiding this comment.
shall we move this to common/resource package?
There was a problem hiding this comment.
I did that initially, but that would cause a circular dependency issue. We can also have a separate package for this: common/resource/config package.
There was a problem hiding this comment.
Moved to resource/config/ pkg
| ctx context.Context, | ||
| request *VisibilityDeleteWorkflowExecutionRequest, | ||
| ) error { | ||
| return v.chooseVisibilityManagerForWrite( |
There was a problem hiding this comment.
Original behavior deletes workflows from all available storages, which is better for data cleaning. What do you think of it?
There was a problem hiding this comment.
Yes I changed that. I don't see it necessary. People uses dual write mode only for migration. After the migration, one of them will be removed anyway. Changing it here will make the behavior consistent with the configuration, or under the expectation of the configuration. It would be very hard to explain/documenting to have such a exceptional behavior in deleting. Another downside of keeping it is that would send more unnecessary DB requests. After switching from dual write mode to ES only mode, new workflows will only be written into ES, and deleting from DB will not delete anything.
| if visConfig != nil && visConfig.EnableSampling() { | ||
| result = p.NewVisibilitySamplingClient(result, visConfig, f.metricsClient, f.logger) | ||
| if visibilityConfig.EnableDBVisibilitySampling() { | ||
| result = p.NewVisibilitySamplingClient(result, &p.SamplingConfig{ |
There was a problem hiding this comment.
Fields in ResourceConfig can be nil, but we are not doing any checks on the caller side or callee side.
There was a problem hiding this comment.
What kind of checking do you think we should have? I am open to add more. There is one checking in the factoryImpl which would return nil when both read and write mode are nil.
There was a problem hiding this comment.
Something similar to line 324 before invoking the functions
There was a problem hiding this comment.
Ah I see. Added the nil check here.
service/worker/service.go
Outdated
| logger log.Logger, | ||
| ) (persistence.VisibilityManager, error) { | ||
| return persistenceBean.GetVisibilityManager(), nil | ||
| common.MatchingServiceName, |
longquanzheng
force-pushed
the
qlong-visibility-require
branch
from
44f3210
to
60a2f97
Compare
What changed?
Why?
How did you test it?
existing tests
Potential risks
Low
Release notes
Announce this change in release to the community
Documentation Changes
cadence-workflow/Cadence-Docs#51