Stream consumers: Re-purpose seen-time, add active-time#11099
Stream consumers: Re-purpose seen-time, add active-time#11099oranagra merged 8 commits intoredis:unstablefrom
Conversation
Seen-time now represents the last attempted read/claim (somehow breaking change) Added active-time which represents the last successful read/claim Seen-time corresponds to "idle" in XINFO CONSUMERS Active-time corresponds to "inactive" XINFO CONSUMERS Note that, in case the consumer has never read any entries, the values for both "active-time" (XINFO FULL) and "inactive" (XINFO CONSUMERS) will be -1, meaning here that the consumer was never active. Note that seen/active time are only affected by XREADGROUP/X[AUTO]CLAIM, not by XPENDING, XINFO and other "read-only" stream CG commands (always has been, even befoe this PR) Other changes: Use the cached time, saving a system call, in many places in t_stream.c
|
@itamarhaber @oranagra currently we are not saving |
guybe7
added
the
breaking-change
oranagra
left a comment
oranagra
left a comment
There was a problem hiding this comment.
assuming we wanna avoid RDB format changes, maybe we wanna re-purpose that field in the RDB to store the active-time (which is what it actually was, right), rather than save the seen time?
also, when loading the rdb, maybe we can load it into both fields?
i.e. we'll be saving the active time (same as we did), but loading it into both.
|
@redis/core-team please comment / approve. |
|
approved in a core-team meeting. shall we do that now? or only after the above mentioned PR matures? |
|
IIRC the comments i gave there are quite substantial, so maybe take the safe route and merge change the rdb part in a separate PR after it becomes a reality. |
|
@oranagra we can but:
|
|
i don't think the damage in the blame log is high, just a couple of comments to delete. on the other hand, maybe there's also no harm in keeping this PR open for much longer.. there's nothing that depends on it, and i don't think it'll accumulate any conflicts. |
|
@guybe7 the RDB version was incremented, so you can now update this PR |
oranagra
added
release-notes
|
@oranagra fixed RDB stuff, please have a look |
oranagra
left a comment
oranagra
left a comment
There was a problem hiding this comment.
are there any new tests we wanna have around rdb loading (to specifically test the feature of this PR)?
regardless, i was about to say that this change is trivial and there's no need to add a compatibility test for the previous type, but i guess you proved me wrong, so let's do that.
have a look stream_cgroups.tcl, there's a test using RESTORE, we can do the same.
The test failed with `ERR DUMP payload version or checksum are wrong.` And it only fails on CentOS, i guess this payload is not suitable on CentOS. Payload regenerated using string2printable, introduced in redis#11099
1. "Fixed" the current code so that seen-time/idle actually refers to interaction attempts (as documented; breaking change) 2. Added active-time/inactive to refer to successful interaction (what seen-time/idle used to be) At first, I tried to avoid changing the behavior of seen-time/idle but then realized that, in this case, the odds are the people read the docs and implemented their code based on the docs (which didn't match the behavior). For the most part, that would work fine, except that issue redis#9996 was found. I was working under the assumption that people relied on the docs, and for the most part, it could have worked well enough. so instead of fixing the docs, as I would usually do, I fixed the code to match the docs in this particular case. Note that, in case the consumer has never read any entries, the values for both "active-time" (XINFO FULL) and "inactive" (XINFO CONSUMERS) will be -1, meaning here that the consumer was never active. Note that seen/active time is only affected by XREADGROUP / X[AUTO]CLAIM, not by XPENDING, XINFO, and other "read-only" stream CG commands (always has been, even before this PR) Other changes: * Another behavioral change (arguably a bugfix) is that XREADGROUP and X[AUTO]CLAIM create the consumer regardless of whether it was able to perform some reading/claiming * RDB format change to save the `active_time`, and set it to the same value of `seen_time` in old rdb files.
1. "Fixed" the current code so that seen-time/idle actually refers to interaction attempts (as documented; breaking change) 2. Added active-time/inactive to refer to successful interaction (what seen-time/idle used to be) At first, I tried to avoid changing the behavior of seen-time/idle but then realized that, in this case, the odds are the people read the docs and implemented their code based on the docs (which didn't match the behavior). For the most part, that would work fine, except that issue redis#9996 was found. I was working under the assumption that people relied on the docs, and for the most part, it could have worked well enough. so instead of fixing the docs, as I would usually do, I fixed the code to match the docs in this particular case. Note that, in case the consumer has never read any entries, the values for both "active-time" (XINFO FULL) and "inactive" (XINFO CONSUMERS) will be -1, meaning here that the consumer was never active. Note that seen/active time is only affected by XREADGROUP / X[AUTO]CLAIM, not by XPENDING, XINFO, and other "read-only" stream CG commands (always has been, even before this PR) Other changes: * Another behavioral change (arguably a bugfix) is that XREADGROUP and X[AUTO]CLAIM create the consumer regardless of whether it was able to perform some reading/claiming * RDB format change to save the `active_time`, and set it to the same value of `seen_time` in old rdb files.
5 participants
Fix #9996
At first, I tried to avoid changing the behavior of seen-time/idle but then realized that, in this case, the odds are the people read the docs and implemented their code based on the docs (which didn't match the behavior).
For the most part, that would work fine, except that issue #9996 was found.
I was working under the assumption that people relied on the docs, and for the most part, it could have worked well enough. so instead of fixing the docs, as I would usually do, I fixed the code to match the docs in this particular case.
Note that, in case the consumer has never read any entries, the values
for both "active-time" (XINFO FULL) and "inactive" (XINFO CONSUMERS) will
be -1, meaning here that the consumer was never active.
Note that seen/active time is only affected by XREADGROUP / X[AUTO]CLAIM, not
by XPENDING, XINFO, and other "read-only" stream CG commands (always has been,
even before this PR)
Other changes:
active_time, and set it to the same value ofseen_timein old rdb files.Todo: