Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deprecate clustersInfo configuration in favor of new clusterMetadata… #1809

Merged
merged 5 commits into from
May 8, 2019
Merged

Deprecate clustersInfo configuration in favor of new clusterMetadata… #1809

merged 5 commits into from
May 8, 2019

Conversation

wxing1292
Copy link
Contributor

@wxing1292 wxing1292 commented May 8, 2019 •
edited
Loading

… configuration, which provides better flexibility

  • Add conversion function which turn clustersInfo into clusterMetadata
  • State builder in history service will always set the replication state whenever applying an event.
  • Fix & update workflow execution reset test code.

New config change allows a cluster to be disabled, which can be useful for cluster migration using cross DC

Before:

clustersInfo:
  enableGlobalDomain: false
  failoverVersionIncrement: 10
  masterClusterName: "active"
  currentClusterName: "active"
  clusterInitialFailoverVersion:
    active: 0
  clusterAddress:
    active:
      rpcName: "cadence-frontend"
      rpcAddress: "127.0.0.1:7933"

After:

clusterMetadata:
  enableGlobalDomain: false
  failoverVersionIncrement: 10
  masterClusterName: "active"
  currentClusterName: "active"
  clusterInformation:
    active:
      enabled: true
      initialFailoverVersion: 0
      rpcName: "cadence-frontend"
      rpcAddress: "127.0.0.1:7933"

@wxing1292 wxing1292 requested a review from longquanzheng May 8, 2019 00:21
@wxing1292 wxing1292 force-pushed the cross-dc-deprecated-cluster branch 5 times, most recently from 27d3eff to 24305cd Compare May 8, 2019 01:26
Deprecate clustersInfo configuration in favor of new clustersInformat…
eb6742a 
…ion configuration, which provides better flexbility
@wxing1292 wxing1292 force-pushed the cross-dc-deprecated-cluster branch from 24305cd to eb6742a Compare May 8, 2019 01:40
@wxing1292 wxing1292 force-pushed the cross-dc-deprecated-cluster branch from eebceb7 to c5b1d06 Compare May 8, 2019 17:39
longquanzheng
longquanzheng reviewed May 8, 2019
View reviewed changes
common/service/config/config.go Outdated
Address struct {
// RPCName indicate the remote service name
RPCName string `yaml:"rpcName"`
// Address indicate the remote service IP address
RPCAddress string `yaml:"rpcAddress"`
}

// ClustersInformation contains the all cluster which participated in cross DC
ClustersInformation struct {
Copy link
Contributor

@longquanzheng longquanzheng May 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use any other name other than ClustersInfomation. The reason is you also have ClusterInfomation and they look too similar, it is not eyeball-friendly. For example: GlobalReplicationConfig

longquanzheng
longquanzheng reviewed May 8, 2019
View reviewed changes
common/service/config/config.go
@@ -219,6 +222,8 @@ type (
}

// ClustersInfo contains the all cluster names and active cluster
//
// Deprecated: please use ClusterInformation instead
ClustersInfo struct {
Copy link
Contributor

@longquanzheng longquanzheng May 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not that hard for external customer to change their config, unlike RPC IDL.
I would recommend to remove all configs in ClustersInfo(make it an empty struct) and keep our code simple, and panic if ClustersInfo is specified.

longquanzheng
longquanzheng reviewed May 8, 2019
View reviewed changes
common/service/config/config.go Outdated
ClustersInformation struct {
EnableGlobalDomain bool `yaml:"enableGlobalDomain"`
// VersionIncrement is the increment of each cluster version when failover happens
VersionIncrement int64 `yaml:"versionIncrement"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go back to FailoverVersionIncrement as we said

longquanzheng
longquanzheng reviewed May 8, 2019
View reviewed changes
cmd/server/server.go Outdated
s.cfg.ClustersInfo.ClusterInitialFailoverVersions,
s.cfg.ClustersInfo.ClusterAddress,
dc.GetBoolProperty(dynamicconfig.EnableGlobalDomain, clustersInformation.EnableGlobalDomain),
clustersInformation.VersionIncrement,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename back for all: version->failoverVersion to be consistent

longquanzheng
longquanzheng reviewed May 8, 2019
View reviewed changes
Copy link
Contributor

@longquanzheng longquanzheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small comments

@wxing1292 wxing1292 changed the title Deprecate clustersInfo configuration in favor of new clustersInformat… Deprecate clustersInfo configuration in favor of new clusterMetadata… May 8, 2019
@wxing1292 wxing1292 merged commit 66a9a25 into master May 8, 2019
@wxing1292 wxing1292 deleted the cross-dc-deprecated-cluster branch May 8, 2019 22:39
@wxing1292 wxing1292 added this to the Sprint 4/29-5/10 milestone May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@longquanzheng longquanzheng longquanzheng approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Sprint 4/29-5/10
Development

Successfully merging this pull request may close these issues.
2 participants
@wxing1292 @longquanzheng