Make optimistic version locking optional#856
Conversation
| s.ver = &field{reflect.TypeOf(int64(0)), "", -1, false, true, false} | ||
| s.verless = true |
There was a problem hiding this comment.
I wonder if it is possible to just keep s.ver == nil?
There was a problem hiding this comment.
Definitely can do that, just would result in more LOC changed.
There was a problem hiding this comment.
@sgtsquiggs, maybe we should keep the current approach. Is this PR ready for merge?
There was a problem hiding this comment.
Hi @sgtsquiggs, could you help resolve the conflicts?
|
@sgtsquiggs Are you continuing on this? |
5d17aa9 to
0b51819
Compare
| if r.schema.verless { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
When will this happen? If it never happens, can we just delete it?
| if r.schema.verless { | |
| return nil | |
| } |
There was a problem hiding this comment.
On the other hand, if the case does happen, we should not return nil either.
There was a problem hiding this comment.
I modified the script (here) to return nil if the ver arg is not present. Alternatively I could have used another script that does not perform optimistic locking, and forked based on r.schema.verless.
There was a problem hiding this comment.
Oh, can we align with the original behavior to return ARGV[2] as well?
There was a problem hiding this comment.
Absolutely. Wrote this PR quickly so I could fork and use om - looking at this code today I don't remember a lot of it and it looks a little dodgy to me. I'll try to reimplement a bit cleaner this week.
| if r.schema.verless { | ||
| continue |
There was a problem hiding this comment.
Same here. Can we delete this?
| // ver is no longer required | ||
| if s.ver == nil { | ||
| panic(fmt.Sprintf("schema %q should have one field with `redis:\",ver\"` tag", t)) | ||
| s.ver = &field{reflect.TypeOf(int64(0)), "", -1, false, true, false} |
There was a problem hiding this comment.
Field names should be explicitly specified.
|
Withdrawing this PR to rework it 👍 |
|
Hi @sgtsquiggs Are you still working on this? |
|
Hi @SoulPancake, I think it is fine if you can take it over. |
Continuation from redis/rueidis#856 solves #854 --------- Co-authored-by: Matthew Crenshaw <mcrenshaw@clearstreet.io> Signed-off-by: Rueian <rueiancsie@gmail.com>
This PR will make the
reids:",ver"tag optional. This PR will resolve #854.Usecase:
You are using
rueidis/omto keep a cache up-to-date, reading directly off a kafka queue. You do not want to deal with versioning your data in redis. You are always upserting.TODO: