cleanup: move list pop logic to single function#7997
Conversation
oranagra
left a comment
oranagra
left a comment
There was a problem hiding this comment.
i'm not sure how i feel about moving the addReply into that helper function. i think we may want to keep it part of the actual command processing function, and not the helper function.
the other parts are all about modifying the source list, so i guess that's ok. so maybe remove the value argument and rename the helper function to listElementsRemoved?
other notes:
- redis uses 4 spaces indentation.
- if you change the order of popElementFromList and popGenericCommand (and add a forward declaration at the top), maybe the diff will be easier to read (and less changes to the blame log).
this is in some way a very small part of #7551. i.e. once we do the change it suggests, the one in this PR isn't needed.
@felipou will this conflict with any work you already started on #766
|
@oranagra I moved the logic into a single function because at that point both
Sure will take note of it. Actually having a Having major refactoring in one PR will take a lot of iteration, having smaller changes into smaller makes landing easier as well as review cycles. Again these are my own opinions 😅 |
|
@yashLadha the intention behind that other refactoring project is that when redis unblocks the client (when BLPOP gets unblocked), it'll just re-run BLPOP, so it'll eliminate code duplication by simply re-running the same code again. regarding the addReply, IMHO the fact that it's used in the duplicate code is not necessarily a good reason to share it IMHO. i think the handling of the reply and value are more intimately related to the calling function and the command and how it wants to handle that. |
I haven't done anything relevant there yet, but I definitely must sync with this for that work. Also, I think there is a very similar code in the LMOVE/BLMOVE commands, that could also be replaced with Anyway, when thinking about #766 I thought that it would be interesting to also add the COUNT argument to LMOVE, since it could probably use the same code. If we unify all the pop logic, it probably would make that quite easier. |
|
I haven't looked into it, but maybe the push functions could be refactored in the same way? |
Yes, it can be done but I thought instead of pushing large changes, one can iteratively improve on it. Let me know your thoughts.
Cool, I will read more about the code and see how it can be improved, and work on your suggestions. @felipou Should I proceed with the review changes or you are also working on the same thing? This is to avoid duplication of work. |
Yes, please proceed, I'll monitor this issue closely as I work on the other issue, so if you just keep this pull requests up to date, I believe it will be easy to sync. I'll comment here if needed. |
yashLadha
force-pushed
the
fix_blpop_dup_code
branch
2 times, most recently
from
1c37138 to
24eb977
Compare
src/t_list.c
Outdated
There was a problem hiding this comment.
in my eyes, the decrRefcount is not part of the responsibility of this function.
it shouldn't deal with the value.
it didn't removed it from the list, and it didn't put it in the reply buffer, so it doesn't suppose to know if the refcount needs decrement or not, that's part of the responsibilities of the caller.
this function is just about what needs to be done the key, keyspace and general server tasks after the list was modified.
src/t_list.c
Outdated
There was a problem hiding this comment.
this function must not assume that argv[1] is the key name, it should get it as an argument from the caller.
BLPOP when there are elements in the list workes in the same way as LPOP does. Due to this they also does the same repeatitive action and logic for the same is written at two different places. This is a bad code practice as the one needs the context to change the BLPOP list pop code as well when the LPOP code gets changed. Separated the generic logic from LPOP to a function that is being used by the BLPOP code as well.
yashLadha
force-pushed
the
fix_blpop_dup_code
branch
from
24eb977 to
161ecd8
Compare
oranagra
changed the title
|
merged. thank you. |
BLPOP when there are elements in the list works in the same way as LPOP does. Due to this they also does the same repetitive action and logic for the same is written at two different places. This is a bad code practice as the one needs the context to change the BLPOP list pop code as well when the LPOP code gets changed. Separated the generic logic from LPOP to a function that is being used by the BLPOP code as well.
3 participants
BLPOP when there are elements in the list works in the same way as LPOP
does. Due to this they also does the same repeatitive action and logic
for the same is written at two different places. This is a bad code
practice as the one needs the context to change the BLPOP list pop code
as well when the LPOP code gets changed.
Separated the generic logic from LPOP to a function that is being used
by the BLPOP code as well.