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

feat: implement atomic operations #300

Merged
merged 7 commits into from
Sep 21, 2021
Merged

Conversation

yuryu
Copy link
Member

@yuryu yuryu commented Sep 14, 2021

What type of PR is this?
/kind feat

What this PR does / Why we need it:
Add the following gRPC methods

  • CompareAndSwap
  • CompareAndSwapGreaterInt
  • CompareAndSwapLessInt
  • AtomicAddInt
  • AtomicSubInt
  • AtomicInc
  • AtomicDec

Which issue(s) this PR fixes:

Closes #134

Special notes for your reviewer:

Add the following gRPC methods
- CompareAndSwap
- CompareAndSwapGreater
- CompareAndSwapLess
- AtomicAdd
- AtomicSub
- AtomicInc
- AtomicDec
@google-cla google-cla bot added the cla: yes label Sep 14, 2021
@yuryu yuryu requested review from hongalex and a user September 14, 2021 23:11
@yuryu
Copy link
Member Author

yuryu commented Sep 14, 2021

I'm not sure if these method names are good or do we want to clarify that they are for properties?
For example we can prepend them with Property

@yuryu
Copy link
Member Author

yuryu commented Sep 15, 2021

I think we should name these methods as IntProperty*. I'll send a commit later today.

@yuryu
Copy link
Member Author

yuryu commented Sep 15, 2021

So I did the rename, also changed some of the messages so that they would be easier to use.

  • CompareAndSwap is now CompareAndSwapProperty
  • all other methods were renamed to ...Int because it is pretty clear only Properties can hold structured data.

PTAL, thanks.

@yuryu yuryu self-assigned this Sep 15, 2021
@yuryu yuryu added this to the v0.2.0-beta3 milestone Sep 15, 2021
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I have added a few suggestions to keep the function names less cryptic and more streamlined. I do not feel strongly about them, except for the AtomicIncInt and AtomicDecInt, which is really confusing when reading the implementation with function names such as doAtomicIncDec (maybe rename to doAtomicOperation).

@yuryu
Copy link
Member Author

yuryu commented Sep 21, 2021

Did the renames according to our discussion.
Now

  • rpc CompareAndSwap(CompareAndSwapRequest) returns (CompareAndSwapResponse) {}
  • rpc CompareAndSwapGreaterInt(AtomicIntRequest) returns (AtomicIntResponse) {}
  • rpc CompareAndSwapLessInt(AtomicIntRequest) returns (AtomicIntResponse) {}
  • rpc AtomicAddInt(AtomicIntRequest) returns (AtomicIntResponse) {}
  • rpc AtomicSubInt(AtomicIntRequest) returns (AtomicIntResponse) {}
  • rpc AtomicInc(AtomicIncRequest) returns (AtomicIntResponse) {}
  • rpc AtomicDec(AtomicIncRequest) returns (AtomicIntResponse) {}

The rationale is that inc/dec is always performed on integers.

@yuryu yuryu merged commit 29a5ab6 into googleforgames:main Sep 21, 2021
@yuryu yuryu deleted the feat/atomic branch September 21, 2021 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add atomic increment and decrement operations
1 participant