feat: Added store option for sort command. #5095
Conversation
H4R5H1T-007
force-pushed
the
main
branch
2 times, most recently
from
9300a91 to
f54e3ac
Compare
src/server/generic_family.cc
Outdated
|
|
||
| // This would overwrite existing value if any with new list. | ||
| // Set the expiry at 300 seconds. | ||
| auto op_res = op_args.GetDbSlice().AddOrUpdate(op_args.db_cntx, key, std::move(pv), 300000); |
There was a problem hiding this comment.
why do you set the expiry to 300s?
There was a problem hiding this comment.
Oh Sorry for that. I read this in Valkey documents and thought we had to add some expiry earlier then forgot to remove it. Thanks for spotting it I will remove it.
An interesting pattern using SORT ... STORE consists in associating an EXPIRE timeout to the resulting key so that in applications where the result of a SORT operation can be cached for some time. Other clients will use the cached list instead of calling SORT for every request. When the key will timeout, an updated version of the cache can be created by calling SORT ... STORE again.
| cmd_cntx.tx->Execute(std::move(fetch_cb), true); | ||
| } | ||
|
|
||
| // OpResultTyped<SortEntryList> fetch_result = |
| OpResultTyped<SortEntryList> fetch_result; | ||
| auto fetch_cb = [&](Transaction* t, EngineShard* shard) { | ||
| ShardId shard_id = shard->shard_id(); | ||
| if (shard_id == source_sid) { |
There was a problem hiding this comment.
pls add a comment: in case of SORT option, we fetch only on the source shard
|
|
||
| auto* rb = static_cast<RedisReplyBuilder*>(builder); | ||
| if (!fetch_result.ok()) | ||
| if (!fetch_result.ok()) { |
There was a problem hiding this comment.
I would unify all the non-ok conditions here:
if (!fetch_result.ok()) {
cmd_cntx.tx->Conclude();
if (fetch_result.status() == OpStatus::INVALID_NUMERIC_RESULT)
return builder->SendError(...);
....
}
There was a problem hiding this comment.
yup that will be better. Will do that
| @@ -1438,10 +1439,33 @@ OpResultTyped<SortEntryList> OpFetchSortEntries(const OpArgs& op_args, std::stri | |||
| return success ? res : OpStatus::INVALID_NUMERIC_RESULT; | |||
| } | |||
|
|
|||
| template <typename IteratorBegin, typename IteratorEnd> | |||
There was a problem hiding this comment.
why do you need a template here? what are the types of the iterators?
There was a problem hiding this comment.
There are two types of iterator present as SortedEntryList is variant of two vector and hence iterator type will also change based on the arguments so I added this template. Let me know if I should handle this using some different method.
|
Hey @romange should I create Sort Generic like we have for other commands? We also have to make Sort_RO command it will be useful there. |
Added Store option in Sort command as mentioned in this comment which is in issue #3886. Please review this PR in the meanwhile I will start with SORT_RO command implementation.