Fix the missing server.dirty increment and redundant signalModifiedKey in serveClientBlockedOnList#11326
Conversation
…y in serveClientBlockedOnList
sundb
changed the title
|
why is it wrong (or bad) to signalModifiedKey twice? |
It can only be considered redundant.
This is equivalent to
When we enter |
|
@oranagra I missed something. |
|
ok, but still i wonder, considering it doesn't do any harm, maybe we can keep it as a statement (can be useful if the code is copied someday) |
|
@oranagra Not sure if you missed something. |
|
ohh, i see.. in all blocking POP / MOVE commands, we had skipped this pop-driven signal with a comment that the push had done that. and the 'm' variant (blmpop) was the odd one. |
oranagra
left a comment
oranagra
left a comment
There was a problem hiding this comment.
so we have 3 types of changes:
- cleanup for sharing code.
- remove a redundant signalModifiedKey with no implications
- fix a missing dirty counter increment.
i'll argue that there's no reason to mention the dirty counter fix in the release notes, since the command did increment it, and the bug is that it incremented it once instead of twice, probably not a big issue)
|
thanks, i haven't had time to look at this yet... but the 3 types of changes LGTM, i will catch up the code later in person |
…y in serveClientBlockedOnList (redis#11326) Mainly fix two minor bug 1. When handle BL*POP/BLMOVE commands with blocked clients, we should increment server.dirty. 2. `listPopRangeAndReplyWithKey()` in `serveClientBlockedOnList()` should not repeat calling `signalModifiedKey()` which has been called when an element was pushed on the list. (was skipped in all bpop commands, other than blmpop) Other optimization add `signal` param for `listElementsRemoved` to skip `signalModifiedKey()` to unify all pop operation. Unifying all pop operations also prepares us for redis#11303, so that we can avoid having to deal with the conversion from quicklist to listpack() in various places when the list shrinks.
…y in serveClientBlockedOnList (redis#11326) Mainly fix two minor bug 1. When handle BL*POP/BLMOVE commands with blocked clients, we should increment server.dirty. 2. `listPopRangeAndReplyWithKey()` in `serveClientBlockedOnList()` should not repeat calling `signalModifiedKey()` which has been called when an element was pushed on the list. (was skipped in all bpop commands, other than blmpop) Other optimization add `signal` param for `listElementsRemoved` to skip `signalModifiedKey()` to unify all pop operation. Unifying all pop operations also prepares us for redis#11303, so that we can avoid having to deal with the conversion from quicklist to listpack() in various places when the list shrinks.
3 participants
Mainly fix two minor bug
listPopRangeAndReplyWithKey()inserveClientBlockedOnList()should not repeat callingsignalModifiedKey()which has been called when an element was pushed on the list.Other optimization
add
signalparam forlistElementsRemovedto skipsignalModifiedKey()to unify all pop operation.Unifying all pop operations also prepares us for #11303, so that we can avoid having to deal with the conversion from quicklist to listpack() in various places when the list shrinks.