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

embeddable etcdmain #5925

Merged
merged 3 commits into from
Jul 13, 2016
Merged

Conversation

heyitsanthony
Copy link
Contributor

Mostly a config / startEtcd refactor

}

// Etcd contains a running etcd server and its listeners.
type Etcd struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to move this out of etcdmain? probably etcd/embed.Server?

@xiang90
Copy link
Contributor

xiang90 commented Jul 12, 2016

This is the direction I have in mind. Only comment is that we might move the etcd structure to somewhere else. etcdmain.Etcd seems strange to me. The original purpose of etcdmain is just to hold the main func. Now it grows...

@heyitsanthony
Copy link
Contributor Author

/cc @purpleidea

@heyitsanthony
Copy link
Contributor Author

@@ -450,9 +513,9 @@ func initialClusterFromName(name string) string {
return fmt.Sprintf("%s=http://localhost:2380", n)
}

func (cfg config) isNewCluster() bool { return cfg.clusterState.String() == clusterStateFlagNew }
func (cfg Config) isNewCluster() bool { return cfg.ClusterState == clusterStateFlagNew }
func (cfg config) isProxy() bool { return cfg.proxy.String() != proxyFlagOff }
Copy link
Contributor

Choose a reason for hiding this comment

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

cfg Config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

still tearing this up; proxy config stuff will be private to etcdmain, regular etcd config stuff will be in embed

@purpleidea
Copy link
Contributor

Thanks for looking into this. It's probably mentioning my earlier take on this in #5584

Let me know when this is passing and ready for review, I'll try refactoring my https://github.com/purpleidea/mgmt/ on top of it to get a feel for how it would work any any issues.

@heyitsanthony heyitsanthony force-pushed the embed-etcdmain branch 7 times, most recently from c60ddad to aa95d0a Compare July 13, 2016 08:00
@xiang90
Copy link
Contributor

xiang90 commented Jul 13, 2016

@purpleidea This pr tries to achieve what you want in: #5584, what openshift (another embedded etcd user) needs, what kuberentes wants, and what our friends from pingCAP needs. We figured it would be hard to ask you directly to coordinate with all the related people. So we decided to do it on our side. But we will try to make sure it achieves most of things you want. Thanks!

}

// config holds the config suitable for yaml parsing
type config struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we use this for any other purpose? if this is just for yaml, probably we should rename this to configYAML.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@xiang90
Copy link
Contributor

xiang90 commented Jul 13, 2016

@heyitsanthony Is the test failure related? seems like ctl_v3 failed. other than that, lgtm.

@heyitsanthony
Copy link
Contributor Author

@xiang90 yeah, there's something going on with the peer connections so quorum e2e tests are failing; still debugging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants