-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
ZMPOP and LMPOP #2094
ZMPOP and LMPOP #2094
Conversation
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.
Going to ask @mgravell for some time here too, but looking a this current API thoughts:
- I think the return types make sense, but would prefer not-nullable, this is a regret with existing I found inconsistent in doing NRTs. For example a
ListSpan.Null
(which would need theRedisKey.Null
from your other PR) instead of nullable.- Same for
SortedSetSpan
- ...but we shouldn't call them that, I don't think. Span is a very core term in .NET now and these aren't related really. Maybe
ListValues
?SortedSetValues
? The latter is weird because it's score plus value, but anyway I think we should find an alternative name for these types.SortedSetEntries
? Spitballing here. - A ton of line formatting changes landed in
RedisDatabase.cs
, can you revert those please? - Docs need a tidy but I'm happy to do that normalization pass/cleanup
- Same for
Thoughts on the above? There's definitely a pending message question w.r.t. cluster handling and key hashing we need to revisit on several recent PRs that I don't think I caught. Let's do a full pass there but address in this one as a baseline.
args[i++] = count; | ||
} | ||
|
||
return Message.Create(Database, flags, RedisCommand.LMPOP, args); |
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.
After looking at the recent issues, I'm not sure how this behaves in cluster. Can you pop across multiple servers? If not, how does it behave? This message overload will not calculate the hash correctly for any keys, so I think we're lacking a proper message type here. /cc @mgravell
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.
Almost certainly not, this will probably break horribly if the keys don't happen to lie on the same shard as the multiplexer hits, curiously there's another multi-key command that I used this pattern from that likely has the same problem, as you said maybe let's categorize these, I'll write the prototype here, and then after this is merged I'll import that pattern elsewhere?
@NickCraver - apologies for the formatting changes in RedisDatabase, Rider apparently had some opinions and I didn't catch it when I was pushing it, I'll clean it up
|
@NickCraver - this looks promising? |
@NickCraver - just pushed a change to take advantage of the slot-specified Message type, I'll look into adding some proper cluster tests for it next week. I think there's a philosophical question, do you want to pre-flight validate that all the keys are in the same slot? Or just make our best effort to point it at the right slot and let the rejection be Redis's problem? |
I dunno - @mgravell is working on connection shenanigans and probably has better input here - note I'm out next week but I'll try and keep a few things moving as around on a laptop. I had to get a lot in queue wrapped up at work today so didn't get to peruse PRs much here. |
Okay I played with this...still not digging the naming, we're inconsistent e.g. ListEntries.Values and SortedSetEntries.Entries but...I don't think any combination of that is awesome. Thoughts on ListPopResult.Values, SortedSetPopResult.Entries? This would match our general terminology and the single results now (e.g. RedisResult/SortedSetEntry). I'm happy to hack at the change, just looking for feedback before more code shenanigans :) |
@NickCraver, I'm good with those names, discrete enough to what they're being used for I suppose, I can make the change. |
In the RedisValue case this isn't needed because of implicit operators, but we may have a null array here with a default, so explicitly use the defaults.
/// Removes and returns at most specified <paramref name="count"/> of elements from the first non-empty list | ||
/// within the set of <paramref name="keys"/> passed into it. | ||
/// Starts on the left side of the list. | ||
/// If the length of the first non-empty list is less than <paramref name="count"/>, only the elements within that list are returned. |
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.
@slorello89 Does this matter? e.g. is it equally correct and maybe clearer to simplify to:
Only the elements from the first non-empty list are ever returned, even if
<paramref name="count"/>
is higher than its length.
...or is that more confusing and/or plain wrong?
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.
The important point is that the first list it encounters is what it pops elements out of. Both ways of phrasing it get at that point so whichever is simpler.
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.
kk - to me it's confusing currently - will adjust, can always change again!
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.
I think current looks good, but I also touched this a lot. @mgravell, @Avital-Fine could one of you lend eyes on latest here please?
@NickCraver - do you feel like the issues surrounding key-slot mappings have been sufficiently addressed? |
I think we're good, but would like to holistically visit adding some tests targeting cluster for several of these APIs before releasing 2.6. I'm mobile at the moment but was going to create an issue there to track so we remember. |
@NickCraver, working on `BITFIELD`/`BITFIELD_RO` and I realized that several of the new commands we've been adding are write-commands and therefore won't work on replicas, opening this to fix the command-map in Message.cs (pretty sure this is the right place but LMK if I'm offbase here) will need to push an update to #2095 #2094 to reflect this as well. Co-authored-by: Nick Craver <[email protected]>
Implementing
ZMPOP
andLMPOP
as part of #2055