Added support to expire specfic member of a sorted set #1000
Conversation
|
@TalZaccai @badrishc With this PR, // Code to reproduce the issue
var header = new RespInputHeader(GarnetObjectType.SortedSet) { SortedSetOp = SortedSetOperation.ZCOLLECT };
Assert.AreEqual(SortedSetOperation.ZCOLLECT, header.SortedSetOp ) // Fails Actual value: ZCARD |
Hey @Vijay-Nirmal, thanks for reaching out! |
|
@TalZaccai Thought of the same thing as a temp fix, I will do it as part of a separate PR
So formal ;-) |
|
I am marking it as ready for review but there are 2 pending items before merging this PR
|
|
Here is the benchmark result, I don't see any much difference between main and this PR with no expiring items. I see very minor change in the item but it could be run to run variant. If you guys have any concern then let me know Main as of 046fcd8
This PR as of c236828
|
|
Are we supporting Resp format v2? If not why some of the commands has respProtocolVersion flag? |
We do indeed support RESP3, not fully yet though. |
| /// <param name="outputFooter">The output footer object to store the result.</param> | ||
| /// <param name="objectContext">The object context for the operation.</param> | ||
| /// <returns>The status of the operation.</returns> | ||
| public GarnetStatus SortedSetExpire<TObjectContext>(ArgSlice key, long expireAt, bool isMilliseconds, ExpireOption expireOption, ref ObjectInput input, ref GarnetObjectStoreOutput outputFooter, ref TObjectContext objectContext) |
There was a problem hiding this comment.
There should be 2 implementations for each of these methods - 1 for programmatic calls and 1 for network calls. The network call should have the ref ObjectInput input, ref GarnetObjectStoreOutput outputFooter parameters and shouldn't have to make any changes to the ObjectInput / parseState. The programmatic call should not have any of these parameters and should build the ObjectInput & parseState from scratch as well as process the output.
Refer to other methods for examples (e.g. SortedSetPop)
There was a problem hiding this comment.
Done. I have fixed a bug in ProcessRespIntegerArrayOutput method as part of this
There was a problem hiding this comment.
I don't see that this was fully resolved, I still see that you're recreating the parseState and the ObjectInput here. Same goes for SortedSetTimeToLive...
There was a problem hiding this comment.
Why is this not fully resolved? We need to create parseState as it is being accessed inside SortedSetObjectImpl
Vijay-Nirmal
requested review from
TalZaccai and
badrishc
and removed request for
TalZaccai
|
Would new command also require changes in TxnKeyManager.cs for NetworkSKIP()? (i.e. grabbing locks in MULTI/EXEC). |
| using Tsavorite.core; | ||
|
|
||
| namespace Garnet.server | ||
| { | ||
| using static System.Runtime.InteropServices.JavaScript.JSType; |
There was a problem hiding this comment.
Done, not sure how this happens often
| /// <param name="outputFooter">The output footer object to store the result.</param> | ||
| /// <param name="objectContext">The object context for the operation.</param> | ||
| /// <returns>The status of the operation.</returns> | ||
| public GarnetStatus SortedSetExpire<TObjectContext>(ArgSlice key, long expireAt, bool isMilliseconds, ExpireOption expireOption, ref ObjectInput input, ref GarnetObjectStoreOutput outputFooter, ref TObjectContext objectContext) |
There was a problem hiding this comment.
I don't see that this was fully resolved, I still see that you're recreating the parseState and the ObjectInput here. Same goes for SortedSetTimeToLive...
| using Garnet.common; | ||
| using HdrHistogram; |
| using ObjectStoreAllocator = GenericAllocator<byte[], IGarnetObject, StoreFunctions<byte[], IGarnetObject, ByteArrayKeyComparer, DefaultRecordDisposer<byte[], IGarnetObject>>>; | ||
| using ObjectStoreFunctions = StoreFunctions<byte[], IGarnetObject, ByteArrayKeyComparer, DefaultRecordDisposer<byte[], IGarnetObject>>; | ||
|
|
||
| sealed partial class StorageSession : IDisposable | ||
| { | ||
| private SingleWriterMultiReaderLock _zcollectTaskLock; |
There was a problem hiding this comment.
This should be closed and disposed in Dispose()
There was a problem hiding this comment.
I am not sure. I have checked other SingleWriterMultiReaderLock usage in the project. In most cases they have called WriteLock in the dispose method in other places they haven't referred to these locks in the dispose method. Think specifically for this lock, I don't think we have to call WriteLock to hold the disposal of the session till ZCOLLECT is completed. Let me know if you have a different opinion.
…ay-Nirmal/garnet into new/sorted-set-member-expire
Adding support for member-specific expiration in a sorted set. This PR adds ZEXPIRE, ZPEXPIRE, ZEXPIREAT, ZPEXPIREAT, ZTTL, ZPTTL, ZEXPIRETIME, ZPEXPIRETIME, ZPERSIST and ZCOLLECT commands to garnet to support this feature.
Since this is a garnet specific command, I used hashset expire API format as the base line. Below is the syntax for each command