Skip to content

Conversation

@PingXie
Copy link
Member

@PingXie PingXie commented May 28, 2024

There are currently three block types: BLOCKED_WAIT, BLOCKED_WAITAOF, and BLOCKED_WAIT_PREREPL, used to block clients executing WAIT, WAITAOF, and CLUSTER SETSLOT, respectively. They share the same workflow: the client is blocked until replication to the expected number of replicas completes. However, they provide different responses depending on the commands involved. Using distinct block types leads to code duplication and reduced readability. This PR consolidates the three types into a single WAIT type, differentiating them using the pending command to ensure the appropriate response is returned.

Fix #427

@codecov
Copy link

codecov bot commented May 29, 2024

Codecov Report

Attention: Patch coverage is 95.23810% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 70.04%. Comparing base (168da8b) to head (a0cefda).
Report is 3 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #562      +/-   ##
============================================
- Coverage     70.22%   70.04%   -0.19%     
============================================
  Files           109      109              
  Lines         59956    59955       -1     
============================================
- Hits          42104    41993     -111     
- Misses        17852    17962     +110     
Files Coverage Δ
src/cluster_legacy.c 86.36% <100.00%> (-0.10%) ⬇️
src/networking.c 85.35% <100.00%> (ø)
src/replication.c 87.20% <100.00%> (+0.04%) ⬆️
src/blocked.c 91.45% <92.85%> (-0.42%) ⬇️

... and 6 files with indirect coverage changes

@PingXie PingXie requested review from madolson and zuiderkwast May 29, 2024 07:31
@zuiderkwast
Copy link
Contributor

Nice. Please add some useful text in the description, rather than just a link to the issue. We always ask that from contributors. It's helpful when reviewing, it's used as commit message when merging and it's good for reference later, even if the PR is not material for release notes.

@PingXie
Copy link
Member Author

PingXie commented May 29, 2024

Will do

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good cleanup. The PR description helped me a lot! Thanks.

If you decide to take my suggestions, don't forget to add signoff.

@PingXie
Copy link
Member Author

PingXie commented May 29, 2024

If you decide to take my suggestions, don't forget to add signoff.

I feel that comparing to the cmd structure point is cleaner/more readable but yes it is very subjective.

@zuiderkwast
Copy link
Contributor

If you decide to take my suggestions, don't forget to add signoff.

I feel that comparing to the cmd structure point is cleaner/more readable but yes it is very subjective.

Pros and cons. It's perhaps cleaner, but it's more important IMO to be consistent with other code doing the same thing. (The alternative is to change all the existing command comparisons to your style.)

Avoiding the need to initialize and store these extra pointers is another point.

@PingXie
Copy link
Member Author

PingXie commented May 30, 2024

but it's more important IMO to be consistent with other code doing the same thing.

You got me on "consistency" :-). I will switch to proc.

@PingXie PingXie changed the title Consolidate various BLOCKED_WAIT states Consolidate various BLOCKED states May 30, 2024
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but there's some crash in test "XREAD: XADD + DEL + LPUSH should not awake client".

This reverts commit 949d017.

Signed-off-by: Ping Xie <[email protected]>
@PingXie
Copy link
Member Author

PingXie commented May 31, 2024

This is not a crash but a behavior change caught by the test. The unification of the three blocked types,, incorrectly, allows an LPUSH operation to wake up a stream reader.

On a closer look, I don't think there is an easy way to unify these 3 states, for two reasons:

  1. each wait (list/zset/stream) can be initiated by multiple commands. it is quite a hassle to track down all these commands;
  2. stream can be waited on even when it is non-existent, which makes it impossible to infer the data type from blockForKeys implicitly

I reverted the last commit.

@PingXie PingXie changed the title Consolidate various BLOCKED states Consolidate various BLOCKED_WAIT* states May 31, 2024
@PingXie PingXie merged commit f927565 into valkey-io:unstable May 31, 2024
@PingXie PingXie deleted the wait-type branch May 31, 2024 06:45
naglera pushed a commit to naglera/placeholderkv that referenced this pull request Jun 10, 2024
There are currently three block types: BLOCKED_WAIT, BLOCKED_WAITAOF,
and BLOCKED_WAIT_PREREPL, used to block clients executing `WAIT`,
`WAITAOF`, and `CLUSTER SETSLOT`, respectively. They share the same
workflow: the client is blocked until replication to the expected number
of replicas completes. However, they provide different responses
depending on the commands involved. Using distinct block types leads to
code duplication and reduced readability. This PR consolidates the three
types into a single WAIT type, differentiating them using the pending
command to ensure the appropriate response is returned.


Fix valkey-io#427

---------

Signed-off-by: Ping Xie <[email protected]>
enjoy-binbin added a commit to enjoy-binbin/valkey that referenced this pull request Jul 22, 2025
BLOCKED_WAITAOF was consolidated into BLOCKED_WAIT in valkey-io#562.

Signed-off-by: Binbin <[email protected]>
enjoy-binbin added a commit that referenced this pull request Aug 4, 2025
BLOCKED_WAITAOF was consolidated into BLOCKED_WAIT in #562.

Signed-off-by: Binbin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[NEW] Consolidate wait states

2 participants