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

new etcdv3 implementation #678

Merged
merged 5 commits into from
Apr 30, 2018
Merged

Conversation

hubo1016
Copy link
Contributor

@hubo1016 hubo1016 commented Feb 26, 2018

This should be a fix for #671

I have tested it with etcdv3 and it appears to be working, but still want some review.

Fixes #671
Fixes #685
Fixes #693

@hubo1016 hubo1016 changed the title [In Progress]new etcdv3 implementation new etcdv3 implementation Feb 26, 2018
@hubo1016
Copy link
Contributor Author

There is still a corner case when a connection failure happens between the watch response and the calling of GetValues. When this happens, the GetValues call fails and is not retried. If there are no further updates, the destination file is not updated even after the connection restore. It should be a rare case. We can add some retry logic in GetValues to prevent this from happening.

@okushchenko @trystanj What's your opinions?

use same revision on all txn requests

add a test case for many input keys
@hubo1016
Copy link
Contributor Author

@okushchenko I think this is ready to merge... In fact I've already using this implementation in the production environment of my company for a month

@hubo1016
Copy link
Contributor Author

should also fix #693

@EyciaZhou
Copy link

doesn't call defer close(cancelRoutine) is ok? because seems the goroutine below will leak also.

go func() {
	select {
	case <-stopChan:
		cancel()
	case <-cancelRoutine:
		return
	}
}()

@hubo1016
Copy link
Contributor Author

hubo1016 commented Apr 1, 2018

Quite right I think. Should I remove the channel and wait for ctx.Done() instead?

- use `chan struct{}` when possible
@hubo1016
Copy link
Contributor Author

hubo1016 commented Apr 2, 2018

@EyciaZhou should be fixed

@hubo1016
Copy link
Contributor Author

hubo1016 commented Apr 2, 2018

Also fix #685

@okushchenko
Copy link
Collaborator

okushchenko commented Apr 7, 2018

@hubo1016 thanks for submitting a PR! Sorry for not getting back to you for so long. The code looks solid. The only thing that I'm getting a hard time to grasp is why have you decided to group the get requests using a transaction? Does it reduce the number of requests to Etcd? What was your reasoning behind that?

The other thing that bugs me is that your changes are actually bigger than the current implementation of etcdv3 backend all together, but I'm ok with that.

@okushchenko okushchenko added this to the 0.16.0 milestone Apr 7, 2018
@hubo1016
Copy link
Contributor Author

hubo1016 commented Apr 7, 2018

@okushchenko this helps on the consistency of the configuration, ensuring all values are retrieved in the same revision. With the old implementation, if two changes are made in revision 1 and 2, both affect two keys (e.g. delete with prefix, and then insert two keys with transaction), confd may get the first key with value in revision 1, but the second in revision 2. Though finally the configuration will be synced to the latest revision, reloading an inconsistent configuration may cause errors.

It also reduces the latency of updating when there are a lot of keys.

@okushchenko okushchenko merged commit b51998d into kelseyhightower:master Apr 30, 2018
zyf0330 pushed a commit to zyf0330/confd that referenced this pull request Nov 21, 2018
@tinder-andyphan
Copy link

tinder-andyphan commented Apr 16, 2020

@hubo1016 @okushchenko curious about this line

https://github.com/kelseyhightower/confd/pull/678/files#diff-8386a15dc01968620740d0c9083008c7R88

and why it's necessary? couldn't this revision possibly not exist due to the disconnect/error being, say, a new leader being reeelcted/repartitioned, not due to compaction?

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