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

genericetcd.Etcd should test resourceVersion #5010

Merged
merged 1 commit into from
Mar 4, 2015

Conversation

smarterclayton
Copy link
Contributor

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

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
@googlebot
Copy link

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)
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

@lavalamp
Copy link
Member

lavalamp commented Mar 4, 2015

LGTM; I don't understand why the code is using AtomicUpdate but this seems like a strict improvement.

lavalamp added a commit that referenced this pull request Mar 4, 2015
genericetcd.Etcd should test resourceVersion
@lavalamp lavalamp merged commit 4cd59e6 into kubernetes:master Mar 4, 2015
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.

Pod update doesn't check for resource version
3 participants