Skip to content
This repository has been archived by the owner on Feb 18, 2021. It is now read-only.

Add UpdateHostsForDeployment API to replicatorClientFactory so that replicator host is updatable #237

Merged
merged 10 commits into from
Jul 19, 2017

Conversation

datoug
Copy link
Contributor

@datoug datoug commented Jun 28, 2017

No description provided.

@jprojs
Copy link

jprojs commented Jun 30, 2017

You should implement zookeeper server for host updater

@datoug datoug force-pushed the replicator_host_updater branch 2 times, most recently from 783d638 to dd6d35a Compare July 10, 2017 18:39
@datoug datoug changed the title Add replicator host updater Add UpdateHostsForDeployment API to replicatorClientFactory so that replicator host is updatable Jul 18, 2017
@datoug datoug requested a review from thuningxu July 18, 2017 21:31
@@ -42,3 +47,14 @@ func (r *ReplicatorConfig) GetReplicatorHosts() map[string]string {
func (r *ReplicatorConfig) GetDefaultAuthoritativeZone() string {
return r.DefaultAuthoritativeZone
}

// GetUseStandalone checks whether a specific deployment is using standalone deployment
func (r *ReplicatorConfig) GetUseStandalone(deployment string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this is irrelevant to open source users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, open source is not using this config. Right now we don't have a clean way to have 'internal only' config(interface is defined in open source) so keep it here for now.

h, tc := replicator.NewReplicator(serviceName, sCommon, meta, replicator.NewReplicatorClientFactory(cfg, common.GetDefaultLogger()), cfg)
allHosts := cfg.GetReplicatorConfig().GetReplicatorHosts()
allSplitHosts := make(map[string][]string)
for deployment, hosts := range allHosts {
Copy link
Contributor

Choose a reason for hiding this comment

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

I almost feel GetReplicatorHosts() should include this logic and return map[string][]string. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These logic is more like a util function rather than some logic that is specific to replicator, so I slightly feel better to keep it outside.

Copy link
Contributor

@thuningxu thuningxu left a comment

Choose a reason for hiding this comment

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

Generally looks good. Minor comments that it is up to you whether you want to address them.

@datoug datoug force-pushed the replicator_host_updater branch from 73e9736 to 958fd58 Compare July 19, 2017 04:25
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 67.889% when pulling b138a38 on replicator_host_updater into 98771ed on master.

@datoug datoug merged commit 901e092 into master Jul 19, 2017
@datoug datoug deleted the replicator_host_updater branch July 19, 2017 05:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants