refactor/testing domain update callback #6365
Conversation
| @@ -53,107 +53,121 @@ func (e *historyEngineImpl) registerDomainFailoverCallback() { | |||
| // failover min / max task levels calculated & updated to shard (using shard lock) -> failover start | |||
| // above 2 guarantees that failover start is after persistence of the task. | |||
|
|
|||
| failoverPredicate := func(shardNotificationVersion int64, nextDomain *cache.DomainCacheEntry, action func()) { | |||
There was a problem hiding this comment.
this large function has been decomposed into a few methods. It's not super easy to read as a diff, but hopefully it's more readable with a degree of decomposition
common/rpc/params.go
Outdated
| @@ -77,7 +77,7 @@ func NewParams(serviceName string, config *config.Config, dc *dynamicconfig.Coll | |||
| return Params{}, fmt.Errorf("inbound TLS config: %v", err) | |||
| } | |||
| outboundTLS := map[string]*tls.Config{} | |||
| for _, outboundServiceName := range service.List { | |||
| for _, outboundServiceName := range service.ShardedServicesList { | |||
There was a problem hiding this comment.
this was to fix a startup issue where the services were not being configured correctly on start. It might be no longer necessary, I might remove it
There was a problem hiding this comment.
can you move it to a separate PR?
There was a problem hiding this comment.
yeah, good call
davidporter-id-au
changed the title
davidporter-id-au
requested review from
Shaddoll,
neil-xie,
Groxx,
shijiesheng,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
What changed?
This predominantly adds some unit test coverage for the domain callback function triggered during domain updates and failovers. It also does some refactoring to enable such test coverage and fixes a few associated minor issues.
Refactoring
failover documentation
The sheer number of domain notification and version fields is quite confusing and requires a lot of careful understanding to even understand how they interact. The tests setup use some actual data examples and lay out a snapshot from the database of each of the scenarios before encoding that into assertions, along with some brief explanation of how notification versions and failover versions work.
Misc other changes:
Why?
This is intended to improve the reliability and coverage.
How did you test it?
Tested locally with three clusters talking to one another.
Potential risks
Assuming a problem here, this could break domain failover and possibly replication
Release notes
Documentation Changes