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

fix etcd lease #2573

Merged
merged 1 commit into from
Jun 23, 2017
Merged

fix etcd lease #2573

merged 1 commit into from
Jun 23, 2017

Conversation

helinwang
Copy link
Contributor

I made a comment in WuYi's PR that this is not necessary, so WuYi
removed it. Turns out it's necessary after confirming with coreOS
developer.

I made a comment in WuYi's PR that this is not necessary, so WuYi
removed it. Turns out it's necessary after confirming with coreOS
developer.
Copy link
Contributor

@typhoonzero typhoonzero left a comment

Choose a reason for hiding this comment

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

LGTM. I'll try to figure out why we need this today.

@typhoonzero
Copy link
Contributor

The steps are:

ch := make(chan *LeaseKeepAliveResponse, leaseResponseChSize)

and then start a go routine to loop and send lease grant request every TTL:

	l.firstKeepAliveOnce.Do(func() {
		go l.recvKeepAliveLoop()
		go l.deadlineLoop()
	})

and then write the response to the channel

// send update to all channels
	nextKeepAlive := time.Now().Add((time.Duration(karesp.TTL) * time.Second) / 3.0)
	ka.deadline = time.Now().Add(time.Duration(karesp.TTL) * time.Second)
	for _, ch := range ka.chs {
		select {
		case ch <- karesp:
			ka.nextKeepAlive = nextKeepAlive
		default:
		}

You can see the channel is writed using select so it's non-blocking write. So TTL will still refresh as you wanted, but if you don't consume the messages in the channel, etcd client will cosume more and more memory as it runs.

@helinwang
Copy link
Contributor Author

@typhoonzero The channel buffer will be capped at leaseResponseChSize, so it looks fine to not eat all the messages. I would rather not do it, if not necessary, since it adds more code (complexity).
I have created a issue hoping to get an authoritative answer: etcd-io/etcd#8168

@helinwang
Copy link
Contributor Author

@typhoonzero I dig a littler more into the code as well. Seems if we don't eat the message, TTL will be send to etcd every 500ms instead of TTL time: etcd-io/etcd#7446 It's an area that I think the etcd document could improve.

@helinwang helinwang merged commit f277726 into PaddlePaddle:develop Jun 23, 2017
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.

None yet

2 participants