-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
There is still a corner case when a connection failure happens between the watch response and the calling of @okushchenko @trystanj What's your opinions? |
use same revision on all txn requests add a test case for many input keys
@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 |
should also fix #693 |
doesn't call
|
Quite right I think. Should I remove the channel and wait for ctx.Done() instead? |
- use `chan struct{}` when possible
@EyciaZhou should be fixed |
Also fix #685 |
@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 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. |
new etcdv3 implementation
@hubo1016 @okushchenko curious about this line
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? |
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