-
Notifications
You must be signed in to change notification settings - Fork 40k
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
genericetcd.Etcd should test resourceVersion #5010
genericetcd.Etcd should test resourceVersion #5010
Conversation
Also fix that Update was returning AlreadyExists instead of NotFound (when create is disabled) and that Update when CreateOnUpdate was true was not populating the returned object. Added more tests
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/. If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits. Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name. |
creating = false | ||
newVersion, err := e.Helper.ResourceVersioner.ResourceVersion(obj) |
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.
Why is this Update function using the AtomicUpdate pattern? Shouldn't it just use CompareAndSwap?
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.
Setting the stage for being able to pass an arbitrary function into Update(), to allow Update() to be used from contexts that want to use the AtomicUpdate style remerge behavior on specific fields. The normal Update case is what it uses today.
----- Original Message -----
creating = false
newVersion, err := e.Helper.ResourceVersioner.ResourceVersion(obj)
Why is this Update function using the AtomicUpdate pattern? Shouldn't it just
use CompareAndSwap?
Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/5010/files#r25822475
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.
An example is the generic Resize controller. I'd like to expose Resize in a way that allows it to use the same update logic, but detect whether a size changed in the event of a conflict. If it didn't, it should allow the update to continue.
----- Original Message -----
Setting the stage for being able to pass an arbitrary function into Update(),
to allow Update() to be used from contexts that want to use the AtomicUpdate
style remerge behavior on specific fields. The normal Update case is what
it uses today.----- Original Message -----
creating = false
newVersion, err := e.Helper.ResourceVersioner.ResourceVersion(obj)
Why is this Update function using the AtomicUpdate pattern? Shouldn't it
just
use CompareAndSwap?
Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/5010/files#r25822475
LGTM; I don't understand why the code is using AtomicUpdate but this seems like a strict improvement. |
genericetcd.Etcd should test resourceVersion
Also fix that Update was returning AlreadyExists instead of NotFound (when create is disabled) and that Update when CreateOnUpdate was true was not populating the returned object.
Added more tests
Fixes #4982