Skip to content

Comments

embeddable etcdmain#5925

Merged
heyitsanthony merged 3 commits intoetcd-io:masterfrom
heyitsanthony:embed-etcdmain
Jul 13, 2016
Merged

embeddable etcdmain#5925
heyitsanthony merged 3 commits intoetcd-io:masterfrom
heyitsanthony:embed-etcdmain

Conversation

@heyitsanthony
Copy link

Mostly a config / startEtcd refactor

etcdmain/etcd.go Outdated
}

// 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
Author

/cc @purpleidea

@heyitsanthony
Copy link
Author


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
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!

embed/config.go Outdated
}

// 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
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
Author

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

@heyitsanthony heyitsanthony merged commit 41a98db into etcd-io:master Jul 13, 2016
@heyitsanthony heyitsanthony deleted the embed-etcdmain branch July 13, 2016 23:36
@siddontang siddontang mentioned this pull request Jul 15, 2016
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