refactor existing domain API for cross DC, refactor existing domain p… #527
Conversation
…ersistence layer, refactor existing domain cache
wxing1292
force-pushed
the
cross-dc-API
branch
from
08937a0
to
a1fce54
Compare
wxing1292
force-pushed
the
cross-dc-API
branch
from
a1fce54
to
ebeba2a
Compare
common/cache/domainCache.go
Outdated
| var config *persistence.DomainConfig | ||
| var result *domainCacheEntry | ||
|
|
||
| copyEntry := func(input *domainCacheEntry) *domainCacheEntry { |
There was a problem hiding this comment.
Would it be better if put copyEntry and isCacheExpired outside? (to save some cycles on definition)
| Message: fmt.Sprintf("GetDomain operation failed. Error %v", err), | ||
| } | ||
| } | ||
|
|
||
| domainName := request.Name | ||
| if len(request.ID) > 0 { | ||
| query = m.session.Query(templateGetDomainQuery, request.ID) |
There was a problem hiding this comment.
I think this change will lead to an additional call to get domain info. Why we cannot get all info with domainID?
There was a problem hiding this comment.
Cassandra do not support conditional update to multiple table.
Previously, we were using batch query to atomically update 2 tables, which (in my opinion) is an overkill, and cause a lot of unnecessary load on Cassandra.
This change will use the domains table just for domain uuid -> domain name lookup.
The actual content will be in domain_by_name table.
There will be additional call to Cassandra, but we have cache, and most of time, we do get, not update
|
|
||
| func (config *ClusterReplicationConfig) serialize() map[string]interface{} { | ||
| output := make(map[string]interface{}) | ||
| output["cluster_name"] = config.ClusterName |
There was a problem hiding this comment.
make cluster_name a const probably
wxing1292
force-pushed
the
cross-dc-API
branch
from
46759c2
to
405a4df
Compare
| @@ -749,17 +759,22 @@ struct DescribeDomainRequest { | |||
| struct DescribeDomainResponse { | |||
| 10: optional DomainInfo domainInfo | |||
| 20: optional DomainConfiguration configuration | |||
| 30: optional DomainReplicationConfiguration replicationConfiguration | |||
| 40: optional i64 (js.type = "Long") failoverVersion | |||
There was a problem hiding this comment.
Any reason why failoverVersion is not within DomainReplicationConfiguration?
There was a problem hiding this comment.
because the failover cannot be modified by customer and DomainReplicationConfiguration is used for update API
|
|
||
| if !applied || err != nil { | ||
| // Domain already exist. Delete orphan domain record before returning back to user | ||
| if errDelete := m.session.Query(templateDeleteDomainQuery, domainUUID).Exec(); errDelete != nil { |
There was a problem hiding this comment.
It is dangerous to delete the domain on any errors.
There was a problem hiding this comment.
this is the create API, and existing logic does the same thing, except existing logic have bug dealing with second create call to Cassandra having errors.
| } | ||
|
|
||
| replicationConfig.ActiveClusterName = m.clusterMetadata.GetOrUseDefaultActiveCluster(replicationConfig.ActiveClusterName) | ||
| for index := range replicationClusters { |
There was a problem hiding this comment.
Can you move this out to a helper method.
| }, nil | ||
| } | ||
|
|
||
| func (m *cassandraMetadataPersistence) UpdateDomain(request *UpdateDomainRequest) error { | ||
| batch := m.session.NewBatch(gocql.LoggedBatch) | ||
| clusterReplicationConfigs := []map[string]interface{}{} |
There was a problem hiding this comment.
It would be great to create some helpers around dealing with replicationConfigs to make the code easier to read.
|
|
||
| type ( | ||
| // ClusterMetadata provides information about clusters | ||
| ClusterMetadata interface { |
There was a problem hiding this comment.
Define the interface in dataInterfaces.go and use the same naming convention we use for other persistence API.
|
|
||
| type ( | ||
| // ClusterMetadata provides information about clusters | ||
| ClusterMetadata interface { |
There was a problem hiding this comment.
Actually I'm not sure if this even belongs in the persistence layer. Let's talk about this.
There was a problem hiding this comment.
the thing is, this originally belongs to common package, until go compiler complains about circular reference between its package and persistence package
wxing1292
force-pushed
the
cross-dc-API
branch
from
fe0d2cb
to
b6af1fb
Compare
wxing1292
force-pushed
the
cross-dc-API
branch
from
b6af1fb
to
7e9ae2e
Compare
…ersistence layer, refactor existing domain cache
solve #493 #495