Fixed blocking command to be timed out based on the specified command argument by barshaul · Pull Request #1283 · valkey-io/valkey-glide

barshaul · 2024-04-15T15:06:01Z

Currently, blocking commands are being timed out based on the client's request timeout. Since blocking commands contain a timeout argument, we should use this command-specific timeout to determine when to terminate

glide-core/tests/test_client.rs

glide-core/src/client/mod.rs

CHANGELOG.md

glide-core/src/client/mod.rs

glide-core/tests/test_client.rs

glide-core/src/client/mod.rs

node/tests/SharedTests.ts

python/python/tests/test_async_client.py

glide-core/tests/test_client.rs

glide-core/src/client/mod.rs

ikolomi · 2024-04-17T07:43:15Z

glide-core/src/client/mod.rs

+fn get_request_timeout(cmd: &Cmd, default_timeout: Duration) -> Option<Duration> {
+    let command = cmd.command().unwrap_or_default();
+    let timeout = match command.as_slice() {
+        b"BLPOP" | b"BRPOP" | b"BLMOVE" | b"BZPOPMAX" | b"BZPOPMIN" | b"BRPOPLPUSH" => {


This has potential to miss the future commands having timeouts - we should add reference to this function to the "integrating new-commands" SOP.
In addition we should have an automatic test like this (for redis):

pull the target server commands.json

detect commands with the timeout param

check we handle each command correctly

@ikolomi Can you create an issue for that?

barshaul · 2024-04-18T15:58:55Z

@jonathanl-bq @Yury-Fridlyand @jonathanl-bq
round

glide-core/src/client/mod.rs

Yury-Fridlyand

LGTM, but couple of questions left

glide-core/src/client/mod.rs

Yury-Fridlyand · 2024-04-18T18:30:08Z

python/python/tests/test_async_client.py

            await redis_client.info(route=ByAddressRoute("foo"))


-@pytest.mark.asyncio


good to test: blpop with timeout 1 sec blocks for ~1 sec even if the request timeout set to 100 millis or so

I test it on the core

avifenesh · 2024-04-24T08:25:10Z

LGTM
But i think there's a point pooping out in result of this PR and other alike.
We are starting to have a lot of points which specific commands need to be added to specific points. Both in redis-rs and both in glide-core, and its create a lot of points-of-failure.
We need to start think how we can create more mature testing facilities which automatically know for a command wether it was added to each point needed or some points were missing.
@ikolomi offer is a good one, but we need to have it more widely to all check, beside time-out (e.g. key-based, routing, legal for pipeline etc.).

… argument (valkey-io#1283)

barshaul requested a review from a team as a code owner April 15, 2024 15:06

barshaul marked this pull request as draft April 15, 2024 15:12

Yury-Fridlyand linked an issue Apr 15, 2024 that may be closed by this pull request

Configure the timeout of blocking command to be user configured #1254

Closed

acarbonetto mentioned this pull request Apr 15, 2024

Java: Add BZPOPMAX command. #1255

Merged

barshaul force-pushed the blocking_cmds branch 2 times, most recently from 400ece5 to 911d098 Compare April 16, 2024 08:09

barshaul marked this pull request as ready for review April 16, 2024 08:11

barshaul requested a review from ikolomi April 16, 2024 08:11

barshaul marked this pull request as draft April 16, 2024 08:15

barshaul force-pushed the blocking_cmds branch from 911d098 to a5dc2d2 Compare April 16, 2024 08:48

barshaul marked this pull request as ready for review April 16, 2024 08:51

barshaul force-pushed the blocking_cmds branch 5 times, most recently from 26cbe61 to c3a67f1 Compare April 16, 2024 13:51

ikolomi reviewed Apr 16, 2024

View reviewed changes

glide-core/tests/test_client.rs Show resolved Hide resolved

glide-core/src/client/mod.rs Outdated Show resolved Hide resolved

Yury-Fridlyand requested a review from jonathanl-bq April 16, 2024 17:17

Yury-Fridlyand added the Rust core label Apr 16, 2024

jonathanl-bq reviewed Apr 16, 2024

View reviewed changes

glide-core/src/client/mod.rs Outdated Show resolved Hide resolved

glide-core/src/client/mod.rs Outdated Show resolved Hide resolved

glide-core/src/client/mod.rs Show resolved Hide resolved

Yury-Fridlyand reviewed Apr 17, 2024

View reviewed changes

barshaul force-pushed the blocking_cmds branch from ca2bbe0 to 918d82f Compare April 17, 2024 06:57

ikolomi reviewed Apr 17, 2024

View reviewed changes

barshaul force-pushed the blocking_cmds branch 6 times, most recently from 2558dd0 to 9e6acd4 Compare April 18, 2024 15:21

barshaul force-pushed the blocking_cmds branch from e44caf2 to 671e498 Compare April 18, 2024 16:11

Fixed blocking command to be timed out based on cmd arg

12b9095

barshaul force-pushed the blocking_cmds branch from 671e498 to 12b9095 Compare April 18, 2024 16:15

jonathanl-bq reviewed Apr 18, 2024

View reviewed changes

glide-core/src/client/mod.rs Outdated Show resolved Hide resolved

glide-core/src/client/mod.rs Outdated Show resolved Hide resolved

Yury-Fridlyand approved these changes Apr 18, 2024

View reviewed changes

Addressing comments

47a8d78

barshaul force-pushed the blocking_cmds branch 2 times, most recently from c6496a7 to 7eda646 Compare April 21, 2024 12:23

Try to revert test changes

59e70f4

barshaul force-pushed the blocking_cmds branch from 7eda646 to 59e70f4 Compare April 21, 2024 12:35

Yury-Fridlyand approved these changes Apr 22, 2024

View reviewed changes

jonathanl-bq approved these changes Apr 22, 2024

View reviewed changes

avifenesh approved these changes Apr 24, 2024

View reviewed changes

ikolomi mentioned this pull request Apr 24, 2024

Make timeout commands dynamic: Hardcoded timeout commands need to be processed dynamically #1333

Closed

2 tasks

ikolomi approved these changes Apr 24, 2024

View reviewed changes

barshaul merged commit c07100c into valkey-io:main Apr 24, 2024

barshaul mentioned this pull request Apr 24, 2024

Node - Added BLPOP command #1223

Merged

cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 24, 2024

Fixed blocking command to be timed out based on the specified command…

a8b78c2

… argument (valkey-io#1283)

Yury-Fridlyand mentioned this pull request Jun 29, 2024

Java: Adding command WAIT #1707

Merged

shohamazon mentioned this pull request Feb 1, 2026

fix(java): enforce immediate timeouts #5264

Merged

		await redis_client.info(route=ByAddressRoute("foo"))


		@pytest.mark.asyncio

Conversation

barshaul commented Apr 15, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ikolomi Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

barshaul Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

ikolomi Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

barshaul commented Apr 18, 2024

Uh oh!

Uh oh!

Uh oh!

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Yury-Fridlyand Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

barshaul Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

avifenesh commented Apr 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants