-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
etcdctl: support etcd0.4 #4020
etcdctl: support etcd0.4 #4020
Conversation
141c01d
to
2f84eb8
Compare
@@ -162,6 +162,11 @@ type Client interface { | |||
// this may differ from the initial Endpoints provided in the Config. | |||
Endpoints() []string | |||
|
|||
// RestEndpoints resets the set of API endpoints used by Client to resolve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest->Reset
but how about just SetEndpoints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel Reset is slightly better as this is a client Interface and we want the func name to be more specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno, how is it more specific? To me Reset
has more of an implication that it's reverting to some default/initial state, whereas x.Set(y)
is very explicitly "set the state of x to y"
I don't feel extremely strongly about this but curious about your reasoning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. OK. I feel Reset means set something again (not set it initially). I trust your english. So I will change it to set then.
LGTM, I agree that this is somewhat preferable to the approach in #3993 |
ResetEndpoints is useful when the there is a scheduled cluster changes or when manually manage the cluster without auto-sync enabled.
For CoreOS users, they will get a updated version of etcdctl without updating the etcd server version. And the users cannot really control this behavior. We do not want to suddenly break them without enough communication. So we still want the most basic opeartions like get, set, watch of etcdctl2 work with etcd 0.4. This patches solve the incompability issue.
For CoreOS users, they will get a updated version of etcdctl without updating
the etcd server version. And the users cannot really control this behavior.
We do not want to suddenly break them without enough communication.
So we still want the most basic operations like get, set, watch of etcdctl2 work
with etcd 0.4. This patches solve the incompatibility issue.
/cc @mitak this involves a client API additional change.
/cc @philips @crawford Please take a look and give it a try.