ZMPOP and LMPOP

[slorello89]

Implementing ZMPOP and LMPOP as part of #2055

[NickCraver]

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 the RedisKey.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

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.

[NickCraver]

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

[slorello89]

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?

[slorello89]

@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

• RE naming, yeah names are tough to come up for these, I was considering using RedisList and RedisSortedSet because the precedent was already there for RedisStream which does something similar, but I didn't want to reserve those class names, hence the use of ListSpan and SortedSetSpan as they are contiguous regions of lists/sorted sets (the name was an intentional homage to that). Maybe SubList and SortedSubSet?

• I can work up some derived Null value

• RE Cluster - I had similar questions, given the questions RE streams that have come up in the last couple days, I can spin up a cluster and check, but I'd guess that it will either throw up if all the keys aren't on the node, or if it comes across a key as it's scanning through this list of keys, either way, it shouldn't be operating across shards. There doesn't appear to be a message that takes an array of keys and an array of values, so maybe we need a new message CommandValueKeysValuesMessage? I'll work up some Cluster test-cases for this as well

[slorello89]

@NickCraver - this looks promising?

[slorello89]

@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?

[NickCraver]

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.

[NickCraver]

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 :)

[slorello89]

@NickCraver, I'm good with those names, discrete enough to what they're being used for I suppose, I can make the change.

[NickCraver]

@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?

[slorello89]

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.

[NickCraver]

kk - to me it's confusing currently - will adjust, can always change again!

[NickCraver]

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?

[slorello89]

@NickCraver - do you feel like the issues surrounding key-slot mappings have been sufficiently addressed?

[NickCraver]

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.