[Compatibility] Added BZMPOP, BZPOPMAX and BZPOPMIN commands #884
Conversation
| @@ -20,6 +20,12 @@ public CollectionItemResult(byte[] key, byte[][] items) | |||
| Items = items; | |||
| } | |||
|
|
|||
| public CollectionItemResult(byte[] key, (double Score, byte[] Element)[] scoredItems) | |||
There was a problem hiding this comment.
I would like to create a separate PR in future to remove CollectionItemResult class and replace it with SpanByteAndMemory, Let me know if there is any issue with that
There was a problem hiding this comment.
Why? It'd make the code less readable. I'd leave it as-is.
Vijay-Nirmal
changed the title
| @@ -6,6 +6,7 @@ | |||
| using System.Collections.Generic; | |||
| using System.Diagnostics; | |||
| using System.Linq; | |||
| using System.Runtime.Intrinsics.X86; | |||
| @@ -886,6 +887,19 @@ private void SortedSetRank(ref ObjectInput input, ref SpanByteAndMemory output, | |||
| } | |||
| } | |||
|
|
|||
| public (double Score, byte[] Element) Pop(bool popMaxScoreElement = false) | |||
There was a problem hiding this comment.
Please add an XML comment
| @@ -886,6 +887,19 @@ private void SortedSetRank(ref ObjectInput input, ref SpanByteAndMemory output, | |||
| } | |||
| } | |||
|
|
|||
| public (double Score, byte[] Element) Pop(bool popMaxScoreElement = false) | |||
There was a problem hiding this comment.
I think we should rename this to PopMinOrMax for clarity
| @@ -2,6 +2,7 @@ | |||
| // Licensed under the MIT license. | |||
|
|
|||
| using System; | |||
| using System.Linq; | |||
| keysBytes[i] = parseState.GetArgSliceByRef(i).SpanByte.ToByteArray(); | ||
| } | ||
|
|
||
| if (!parseState.TryGetDouble(parseState.Count - 1, out var timeout)) |
There was a problem hiding this comment.
Could do this verification (as well as the itemBroker one) before the keysBytes allocation
| } | ||
| else | ||
| { | ||
| _ = EXPIRE(destinationKey, TimeSpan.Zero, out _, StoreType.Object, ExpireOption.None, | ||
| ref lockableContext, ref objectContext); | ||
| } | ||
|
|
||
| itemBroker.HandleCollectionUpdate(destinationKeyBty); |
There was a problem hiding this comment.
What about SortedSetUnionStore & SortedSetIntersectStore?
| @@ -10,7 +10,7 @@ | |||
|
|
|||
| namespace Garnet.test | |||
| { | |||
| public class RespBlockingListTests | |||
| public class RespBlockingTests | |||
There was a problem hiding this comment.
naming: RespBlockingCollectionTests might be a bit more informative
| @@ -20,6 +20,12 @@ public CollectionItemResult(byte[] key, byte[][] items) | |||
| Items = items; | |||
| } | |||
|
|
|||
| public CollectionItemResult(byte[] key, (double Score, byte[] Element)[] scoredItems) | |||
There was a problem hiding this comment.
Why? It'd make the code less readable. I'd leave it as-is.
| /// <summary> | ||
| /// Scored items retrieved from collection, where each item has an associated score. | ||
| /// </summary> | ||
| internal (double Score, byte[] Element)[] ScoredItems { get; } |
There was a problem hiding this comment.
I think instead of doing this we should re-use the Item & Items properties to store the elements & have matching double Score and double[] Scores properties.
| @@ -389,14 +389,154 @@ public void BlmpopBlockingWithCountTest() | |||
| var pushingTask = taskFactory.StartNew(() => | |||
| { | |||
| using var lcr = TestUtils.CreateRequest(); | |||
| Task.Delay(TimeSpan.FromSeconds(2)).Wait(); | |||
| Task.Delay(TimeSpan.FromSeconds(5)).Wait(); | |||
There was a problem hiding this comment.
Might want to choose smaller time constants here, as this affects the CI runtime
Adding the BZMPOP, BZPOPMAX and BZPOPMIN commands to garnet