Fix propagation of entries_read by calling streamPropagateGroupID unconditionally#12898
Conversation
…onditionally In XREADGROUP ACK, because streamPropagateXCLAIM does not propagate entries-read, entries-read will be inconsistent between master and replicas. The fix was suggested by guybe7, call streamPropagateGroupID unconditionally, so that we will normalize entries_read on the replicas. Issue was introduced in redis#9127.
|
revisiting the decision not to add another arg to XCLAIM: why is that a problem? it's only a problem if the master is new and replica is old (major-version-wise), which anyway isn't supported |
@guybe7 you're right, but there are also other cases (like RL's replica-of, or loading an AOF), which can replicate a new version to and old one. so if the current approach is viable, maybe it's a better one... |
|
@oranagra ok, so what happens with the current code when the user uses NOACK? the server propagates the new form of XGROUP SETID (with ENTRIESREAD) and the receiving server (assuming it's an older version) will fail to execute it... I mean, we probably have the same problem (old server fails to execute commands from a new server) in many other places, so I see no reason to avoid adding a new arg to XCLAIM |
|
i already lost context 😞 the difference could be if the command / combination that breaks it is new, or rarely used, in which case the replication won't break unless the user uses a certain feature. |
|
talked about this with @oranagra and we decided to keep the current approach because we want minimal damage in case there's an old target and new source (in the worst case scenario, the new source doesn't recognize we also understand that if the user uses |
|
to elaborate on that, we assume the extra command in case of COUNT 1 (4x factor, changing from one XCLAIM to MULTI+XCLAIM+XSETID+EXEC), is probably ok since reading just one entry is in any case very inefficient (a client round trip per record), so we're hoping it's not a common case. |
|
@enjoy-binbin please ping me that it's ready for merge from your perspective |
|
@oranagra I browsed through it again, it is ready to merge. |
In XREADGROUP ACK, because streamPropagateXCLAIM does not propagate
entries-read, entries-read will be inconsistent between master and replicas.
I.e. if no entries were claimed, it would have propagated correctly, but if some
were claimed, then the entries-read field would be inconsistent on the replica.
The fix was suggested by guybe7, call streamPropagateGroupID unconditionally,
so that we will normalize entries_read on the replicas. In the past, we would
only set propagate_last_id when NOACK was specified. And in #9127, XCLAIM did
not propagate entries_read in ACK, which would cause entries_read to be
inconsistent between master and replicas.
Another approach is add another arg to XCLAIM and let it propagate entries_read,
but we decided not to use it. Because we want minimal damage in case there's an
old target and new source (in the worst case scenario, the new source doesn't
recognize XGROUP SETID ... ENTRIES READ and the lag is lost. If we change XCLAIM,
the damage is much more severe).
In this patch, now if the user uses XREADGROUP .. COUNT 1 there will be an additional
overhead of MULTI, EXEC and XGROUPSETID. We assume the extra command in case of
COUNT 1 (4x factor, changing from one XCLAIM to MULTI+XCLAIM+XSETID+EXEC), is probably
ok since reading just one entry is in any case very inefficient (a client round trip
per record), so we're hoping it's not a common case.
Issue was introduced in #9127.