X[AUTO]CLAIM should skip deleted entries#10227
Merged
oranagra merged 4 commits intoredis:unstablefrom Feb 8, 2022
Merged
Conversation
Fix redis#7021 redis#8924 redis#10198 Instead of replying with "nil" (which is undocumented) and transferring the entry to the dst consumer, it just skips deleted entries (leaving them in the same consumer PEL they were found) This is somewhat of a breaking change, since X[AUTO]CLAIM used to be able to reply with "nil" and now it can't... But since it was undocumented the breakage is not that bad.
This was
linked to
issues
Feb 2, 2022
oranagra
approved these changes
Feb 2, 2022
Member
|
@guybe7 do we need to document the new element of the response in the history section of the command's json file? |
Member
|
@redis/core-team please approve. this is a small breaking change (omitting records from the reply), and also an interface change (new response element in XAUTOCLAIM) |
Member
|
@guybe7 can you please make a redis-doc PR? |
madolson
approved these changes
Feb 7, 2022
Contributor
madolson
left a comment
There was a problem hiding this comment.
Also makes sense to me.
Merged
This was referenced Mar 18, 2022
3 tasks
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
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.
Fix #7021 #8924 #10198
Intro
Before this commit X[AUTO]CLAIM used to transfer deleted entries from one PEL to another, but reply with "nil" for every such entry (instead of the entry id).
The idea (for XCLAIM) was that the caller could see this "nil", realize the entry no longer exists, and XACK it in order to remove it from PEL.
The main problem with that approach is that it assumes there's a correlation between the index of the "id" arguments and the array indices, which there isn't (in case some of the input IDs to XCLAIM never existed/read):
Changes
Now, X[AUTO]CLAIM acts in the following way:
Knowing which entries were cleared from the PEL
The caller may want to log any entries that were found in a PEL but deleted from the stream itself (it would suggest that there might be a bug in the application: trimming the stream while some entries were still no processed by the consumers)
XCLAIM
the set {XCLAIM input ids} - {XCLAIM returned ids} contains all the entry ids that were not claimed which means they were deleted (assuming the input contains only entries from some PEL). The user doesn't need to XACK them because XCLAIM had already deleted them from the source PEL.
XAUTOCLAIM
XAUTOCLAIM has a new element added to its reply: it's an array of all the deleted stream IDs it stumbled upon.
This is somewhat of a breaking change since X[AUTO]CLAIM used to be able to reply with "nil" and now it can't... But since it was undocumented (and generally a bad idea to rely on it, as explained above) the breakage is not that bad.