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

improve: use optimistic locking to avoid concurrency problem in admin… #2216

Merged
merged 3 commits into from
Sep 18, 2020

Conversation

tokers
Copy link
Contributor

@tokers tokers commented Sep 11, 2020

What this PR does / why we need it:

There is a potential concurrency problem in all admin PATCH APIs when
two patch requests come in simultaneously, in such case, the patched
result of the first applied request will be overridden, although the
probability is tiny, from the perspective of software's robustness,
that's not what we wanna see.

In this commit, we use the optimistic locking to avoid this problem, for
the example aforementioned, the second PATCH request will failure, and
it's up to the user to retry this PATCH request again.

The optimistic locking mechanism in ETCD v2 APIs is showed by arg
prevIndex.

Signed-off-by: tokers zchao1995@gmail.com

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible?

@tokers tokers force-pushed the improve/patch_with_cas branch 2 times, most recently from cb19ca6 to 0ad096e Compare September 11, 2020 09:13
Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice PR

@tokers
Copy link
Contributor Author

tokers commented Sep 14, 2020

@membphis :)

@tokers
Copy link
Contributor Author

tokers commented Sep 16, 2020

@membphis @moonming This PR should be adjusted to the ETCD v3 data structure, So i mark it as work in progress.

@tokers tokers changed the title improve: use optimistic locking to avoid concurrency problem in admin… WIP: improve: use optimistic locking to avoid concurrency problem in admin… Sep 16, 2020
@membphis
Copy link
Member

@tokers got it, we can merge this PR later

@tokers tokers changed the title WIP: improve: use optimistic locking to avoid concurrency problem in admin… improve: use optimistic locking to avoid concurrency problem in admin… Sep 16, 2020
@tokers
Copy link
Contributor Author

tokers commented Sep 16, 2020

@membphis Adjusted.

… PATCH APIs.

There is a potential concurrency problem in all admin PATCH APIs when
two patch requests come in simultaneously, in such case, the patched
result of the first applied request will be overridden, also the
probability is tidy, but from the perspective of software's robust,
that's not what we wanna to see.

In this commit, we use the optimistic locking to avoid this problem, for
the example aforementioned, the second PATCH request will failure, and
it's up to the user to retry this PATCH request again.

The optimistic locking mechanism in ETCD v3 APIs is showed by it's
transcation mechanism.

Signed-off-by: tokers <zchao1995@gmail.com>
@tokers
Copy link
Contributor Author

tokers commented Sep 17, 2020

@membphis @moonming Please help me to retry the checks 😂, it's weird that this failure only in linux_tengine.

@tokers
Copy link
Contributor Author

tokers commented Sep 18, 2020

@membphis Could you review this PR once again? The adjustment was done.

@moonming moonming merged commit dd6ee5e into apache:master Sep 18, 2020
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.

3 participants