feat(streams): Add support for XTRIM #1337
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dranikpg
previously approved these changes
Jun 2, 2023
src/server/stream_family.cc
Outdated
Comment on lines
1059
to
1081
| int64_t deleted = 0; | ||
| if (!opts.limit) { | ||
| if (opts.trim_strategy == TrimStrategy::kMaxLen) { | ||
| /* Notify xtrim event if needed. */ | ||
| deleted = streamTrimByLength(s, opts.max_len, opts.trim_approx); | ||
| // TODO: when replicating, we should propagate it as exact limit in case of trimming. | ||
| } else if (opts.trim_strategy == TrimStrategy::kMinId) { | ||
| deleted = streamTrimByID(s, opts.minid.val, opts.trim_approx); | ||
| } | ||
| } else { | ||
| streamAddTrimArgs trim_args = {}; | ||
| trim_args.trim_strategy = static_cast<int>(opts.trim_strategy); | ||
| trim_args.approx_trim = opts.trim_approx; | ||
| trim_args.limit = opts.limit; | ||
|
|
||
| if (opts.trim_strategy == TrimStrategy::kMaxLen) { | ||
| trim_args.maxlen = opts.max_len; | ||
| } else if (opts.trim_strategy == TrimStrategy::kMinId) { | ||
| trim_args.minid = opts.minid.val; | ||
| } | ||
| deleted = streamTrim(s, &trim_args); | ||
| } | ||
|
|
There was a problem hiding this comment.
Isn't it the same as in OpAdd? I think we can unite it in a common trim function
There was a problem hiding this comment.
yep sure - updated
src/server/stream_family.cc
Outdated
Comment on lines
1126
to
1142
| if (opts.trim_strategy != TrimStrategy::kNone) { | ||
| (*cntx)->SendError("MAXLEN and MINID options at the same time are not compatible", | ||
| kSyntaxErr); | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| id_indx++; | ||
| arg = ArgS(args, id_indx); | ||
| if (add_opts.trim_strategy == TrimStrategy::kAddOptsTrimMaxLen && | ||
| !absl::SimpleAtoi(arg, &add_opts.max_len)) { | ||
| return (*cntx)->SendError(kSyntaxErr); | ||
| if (remaining_args >= 2 && arg == "~") { | ||
| opts.trim_approx = true; | ||
| id_indx++; | ||
| arg = ArgS(args, id_indx); | ||
| } else if (remaining_args >= 2 && arg == "=") { | ||
| opts.trim_approx = false; | ||
| id_indx++; | ||
| arg = ArgS(args, id_indx); | ||
| } |
There was a problem hiding this comment.
Its the same as above, except for the three lines below. I think we can handle MAXLEN and MINID within one branch by differentiating those two cases only at the very end
There was a problem hiding this comment.
sure yep updated
Signed-off-by: Andy Dunstall <[email protected]>
dranikpg
approved these changes
Jun 2, 2023
Closed
|
Currently testing this 🙏 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds support for XTRIM (#1332)
TrimStrategyenum given theAddOptsTrimprefix seems redundant given its always prefixed byTrimStrategy?OpTrimwhich is the same as trimming inOpAdd(theres a TODO about handling replication copied over fromOpAddwhich seems out of scope for this)XADDandXTRIMargument parsing intoParseAddOrTrimArgsOrReply(which matches how Redis parses the commands given their arguments are very similar)