Make tracking invalidation messages always after command's reply#9422
Make tracking invalidation messages always after command's reply#9422oranagra merged 12 commits intoredis:unstablefrom
Conversation
|
@huangzhw what's the content of that last force-push? is it just a rebase from unstable or anything else? p.s. we usually merge PRs with squash-merge, so it's more convenient to review if there are no force-pushes, and i'd rather merge unstable into the PR instead of a rebase. |
|
I just rebase unstable as there are conflicts. Only the last commit is new. |
oranagra
left a comment
There was a problem hiding this comment.
LGTM.
@madolson @yossigo @soloestoy does one of you want to run a second pair of eyes on this?
in theory there's a potential for a temp memory spike, since we flush that list quite frequently, i don't think that's a realistic concern.
There's also an additional lookupClientByID, but that's kinda trivial too i think.
|
triggered daily CI: https://github.com/redis/redis/actions/runs/1250884079 (failures are unrelated to this PR) |
madolson
left a comment
There was a problem hiding this comment.
I suppose I'm also not clear about the direction where we are deferring the sending of the invalidation, and not deferring the invalidation itself until outside of the command execution, which seems a little more straight forward of an implementation.
@madolson assuming i understand what you mean, and i remember the details: |
response are in the same connection.
Co-authored-by: Oran Agra <[email protected]>
oranagra
left a comment
There was a problem hiding this comment.
I guess this is ready to be merged.
any last concerns?
madolson
left a comment
There was a problem hiding this comment.
I have no concerns, took another pass through and it LGTM.
|
@huangzhw one of the new tests in this PR seems to have a rare timing issue: can you please take a look |
|
interestingly, it looks like the failure is semi consistent now, happening a lot in one of the external tests (either clustered or not clustered) |
|
I have a question. In external test between every |
|
yes, look for |
|
looking at the test code i think i may realized the problem. so maybe we need to copy this code from lazyfree.tcl: # make the previous test is really done before sampling used_memory
wait_for_condition 50 100 {
[s lazyfree_pending_objects] == 0
} else {
fail "lazyfree isn't done"
}or maybe even promote that to a function in util.tcl to be shared between them and maybe others in the future... |
|
I check the code before this test in |
|
yes, the fact it only happens in external mode, is also a good indication it depends on other tests. |
|
Looks like my hypothesis was wrong https://github.com/redis/redis/actions/runs/1412769681 |
|
found the problem: #9726 |
Tracking invalidation messages were sometimes sent in inconsistent order, before the command's reply rather than after. In addition to that, they were sometimes embedded inside other commands responses, like MULTI-EXEC and MGET. (cherry picked from commit fd135f3)
Tracking invalidation messages were sometimes sent in inconsistent order, before the command's reply rather than after.
In addition to that, they were sometimes embedded inside other commands responses, like MULTI-EXEC and MGET.
Fix #8206 and #8935