Skip to content

X[AUTO]CLAIM should skip deleted entries#10227

Merged
oranagra merged 4 commits intoredis:unstablefrom
guybe7:fix_xclaim_del
Feb 8, 2022
Merged

X[AUTO]CLAIM should skip deleted entries#10227
oranagra merged 4 commits intoredis:unstablefrom
guybe7:fix_xclaim_del

Conversation

@guybe7
Copy link
Collaborator

@guybe7 guybe7 commented Feb 2, 2022

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):

127.0.0.1:6379> XADD x 1 f1 v1
"1-0"
127.0.0.1:6379> XADD x 2 f1 v1
"2-0"
127.0.0.1:6379> XADD x 3 f1 v1
"3-0"
127.0.0.1:6379> XGROUP CREATE x grp 0
OK
127.0.0.1:6379> XREADGROUP GROUP grp Alice COUNT 2 STREAMS x >
1) 1) "x"
   2) 1) 1) "1-0"
         2) 1) "f1"
            2) "v1"
      2) 1) "2-0"
         2) 1) "f1"
            2) "v1"
127.0.0.1:6379> XDEL x 1 2
(integer) 2
127.0.0.1:6379> XCLAIM x grp Bob 0 0-99 1-0 1-99 2-0
1) (nil)
2) (nil)

Changes

Now, X[AUTO]CLAIM acts in the following way:

  1. If one tries to claim a deleted entry, we delete it from the PEL we found it in (and the group PEL too). So de facto, such entry is not claimed, just cleared from PEL (since anyway it doesn't exist in the stream)
  2. since we never claim deleted entries, X[AUTO]CLAIM will never return "nil" instead of an entry.
  3. add a new element to XAUTOCLAIM's response (see below)

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.

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.
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@oranagra oranagra added the breaking-change This change can potentially break existing application label Feb 2, 2022
Copy link
Member

@itamarhaber itamarhaber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@oranagra
Copy link
Member

oranagra commented Feb 2, 2022

@guybe7 do we need to document the new element of the response in the history section of the command's json file?
we recently did that for CLUSTER SLOTS, and the realization was that we usually only document changes in arguments in the history, and we avoid documenting bug fixes in the response, or new fields of a map (like INFO or MEMORY STATS), but we might wanna document new fields in arrays (like in SLOWLOG GET).
@itamarhaber WDYT?

@oranagra
Copy link
Member

oranagra commented Feb 2, 2022

@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)

@oranagra oranagra added approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus labels Feb 2, 2022
Copy link
Collaborator

@yossigo yossigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.

@oranagra
Copy link
Member

oranagra commented Feb 7, 2022

@guybe7 can you please make a redis-doc PR?

Copy link
Contributor

@madolson madolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change This change can potentially break existing application release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

[BUG] XAUTOCLAIM returns nil for old messages in PEL [BUG] xclaim claims the message but returns nil XCLAIMing XDELeted entries

5 participants