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.

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

Replicate domain #586

Merged
merged 6 commits into from
Mar 1, 2018
Merged

Replicate domain #586

merged 6 commits into from
Mar 1, 2018

Conversation

wxing1292
Copy link
Contributor

solve #569

@coveralls
Copy link

coveralls commented Feb 28, 2018

Coverage Status

Coverage increased (+0.08%) to 67.353% when pulling ec1a93b on replicate-domain into f265fd8 on master.

@wxing1292 wxing1292 force-pushed the replicate-domain branch 4 times, most recently from e3ccb0e to ac69497 Compare March 1, 2018 00:45
… update domain transfer task

add is global domain for domain
add domain replication logic
@@ -46,8 +46,10 @@ services:
port: 7937

clustersInfo:
enableGlobalDomain: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have enableGlobalDomain and masterClusterName as dynamic config knobs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me do a separate PR just for dynamic config integration

@@ -0,0 +1,8 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have these schema changes as part of v0.5? We haven't released that yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have not release that yet, however, 0.5 is rolled out to staging.
we can definitely do some manual work on staging.
let me know your preference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, changed to 0.5


// whether active cluster is changed
activeClusterChanged := false
// whether anything other than active cluster is changed
configurationChanged := false

validateReplicationConfig := func(existingDomain *persistence.GetDomainResponse,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Why this validation is defined as a closure? Can we just have a validation function defined on the handler like we do for all different kind of validation?

Copy link
Contributor Author

@wxing1292 wxing1292 Mar 1, 2018

Choose a reason for hiding this comment

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

the reason i put this validation as a closure: this function is not reusable, by other logics.
Plus this closure captured a lot of variables.

if i change this to a dedicated function, than the number of return values will be > 4.

for _, cluster := range replicationConfig.Clusters {
if cluster.ClusterName == replicationConfig.ActiveClusterName {
activeClusterInClusters = true
break
Copy link
Contributor

Choose a reason for hiding this comment

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

one coding practice we follow is to always use label when breaking out of loops.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok sure

@wxing1292 wxing1292 merged commit 4a60ac3 into master Mar 1, 2018
@wxing1292 wxing1292 deleted the replicate-domain branch March 1, 2018 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants