Decouple domain cache entry from cluster metadata #4847
Conversation
vytautas-karpavicius
force-pushed
the
decouple-cluster-metadata
branch
from
1daf8a7
to
d84dc06
Compare
common/cache/domainCache.go
Outdated
| @@ -157,7 +150,6 @@ func NewDomainCache( | |||
| cacheNameToID: &atomic.Value{}, | |||
| cacheByID: &atomic.Value{}, | |||
| domainManager: domainManager, | |||
| clusterMetadata: clusterMetadata, | |||
| timeSource: clock.NewRealTimeSource(), | |||
| metricsClient: metricsClient, | |||
There was a problem hiding this comment.
as you are here... can you replace metricsClient with scope?
Something like this: scope := c.metricsClient.Scope(metrics.DomainCacheScope)
common/cache/domainCache.go
Outdated
| func newDomainCacheEntry( | ||
| clusterMetadata cluster.Metadata, | ||
| ) *DomainCacheEntry { | ||
| func newDomainCacheEntry() *DomainCacheEntry { |
There was a problem hiding this comment.
Seems like this function is noop? Consider to remove
There was a problem hiding this comment.
Yes, no longer needed
common/cache/domainCache.go
Outdated
|
|
||
| return &DomainCacheEntry{ | ||
| clusterMetadata: clusterMetadata, | ||
| initialized: false, | ||
| initialized: false, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
If that's for tests only, maybe it is worth to add *testing.T as a first arg and/or move to test file?
common/cache/domainCache.go
Outdated
| @@ -127,7 +122,6 @@ type ( | |||
|
|
|||
| // DomainCacheEntry contains the info and config for a domain | |||
| DomainCacheEntry struct { | |||
| clusterMetadata cluster.Metadata | |||
| sync.RWMutex | |||
There was a problem hiding this comment.
do we have to keep mutex public?
There was a problem hiding this comment.
We do not. Made private.
common/domain/isActive.go
Outdated
|
|
||
| // IsActive return whether the domain is active, i.e. non global domain or global domain which active cluster is the current cluster | ||
| // If domain is not active, it also returns an error | ||
| func IsActive(domain *cache.DomainCacheEntry, cluster cluster.Metadata) (bool, error) { |
There was a problem hiding this comment.
couple of thoughts to consider:
- Cluster metadata is not strictly needed here, only cluster name
- I would go with struct method:
DomainCacheEntry.IsActiveIn(cluster string)
There was a problem hiding this comment.
Thanks for suggestion, I like it much better this way! 👍
common/domain/isActive.go
Outdated
| return true, nil | ||
| } | ||
|
|
||
| func GetActiveDomainByID(cache cache.DomainCache, cluster cluster.Metadata, id string) (*cache.DomainCacheEntry, error) { |
There was a problem hiding this comment.
nit: id -> domainID
| @@ -791,7 +792,7 @@ func (r *mutableStateTaskGeneratorImpl) GenerateFromCrossClusterTask( | |||
| var targetCluster string | |||
|
|
|||
| sourceDomainEntry := r.mutableState.GetDomainEntry() | |||
| if !sourceDomainEntry.IsDomainActive() && !sourceDomainEntry.IsDomainPendingActive() { | |||
| if isActive, _ := domain.IsActive(sourceDomainEntry, r.clusterMetadata); !isActive && !sourceDomainEntry.IsDomainPendingActive() { | |||
There was a problem hiding this comment.
I guess you are skipping err check here because you need implementation details?
There was a problem hiding this comment.
We do not need error here. Both GetNotActiveErr and IsActive are now combined into one function. The error part is not relevant here.
| newEntry.notificationVersion = record.NotificationVersion | ||
| newEntry.initialized = true | ||
| return newEntry | ||
| return &DomainCacheEntry{ |
There was a problem hiding this comment.
this is so much better
What changed?
cache.DomainCacheEntryfromcluster.MetadataWhy?
cluster.Metadatawas used solely to check whether domain is active. This can be done by simply passing current cluster as a parameter. Decoupling domain entry simplifies code & testing.How did you test it?
Existing tests and new units tests covering moved out functions.
Potential risks
Release notes
Documentation Changes