valkey-cli auto-exit from subscribed mode by Nikhil-Manglore · Pull Request #1432 · valkey-io/valkey

Nikhil-Manglore · 2024-12-12T22:05:52Z

Resolves issue with valkey-cli not auto exiting from subscribed mode on reaching zero pub/sub subscription (previously filed on Redis) redis/redis#12592

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

codecov · 2024-12-12T23:43:58Z

Codecov Report

Attention: Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.89%. Comparing base (b3b4bdc) to head (fe091cf).
Report is 7 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/valkey-cli.c	96.29%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1432      +/-   ##
============================================
+ Coverage     70.83%   70.89%   +0.05%     
============================================
  Files           120      120              
  Lines         64911    64980      +69     
============================================
+ Hits          45982    46069      +87     
+ Misses        18929    18911      -18

Files with missing lines	Coverage Δ
src/valkey-cli.c	`55.85% <96.29%> (+0.36%)`	⬆️

... and 50 files with indirect coverage changes

zuiderkwast

Hello! Welcome to Valkey!

I remember this issue. :)

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

zuiderkwast

Logic looks correct, but I have two comments.

I think it would be good to add a test for valkey-cli that uses SUBSCRIBE, PSUBSCRIBE, SSUBSCRIBE in the same session, an exits pubsub mode when all three are unsubscribed.

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

zuiderkwast

Good, thanks. I have some new comments. :)

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

zuiderkwast

It's getting better! I'm not sure if check for "(subscribed mode)" in the prompt works as intended though.

Maybe we need to test this in some other way. If you can't solve it, then I'll try to think of something.

…mplementing test cases Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

…y into subscribe-mode-bug

…s more robust Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

hpatro

LGTM overall. minor nitpicks.

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

zuiderkwast · 2025-01-08T18:22:48Z

The clang-format CI job is complaining about trailing whitespace. See https://github.com/valkey-io/valkey/actions/runs/12676568798/job/35330205216?pr=1432

To fix it, you can run clang-format -i src/valkey-cli.c or just manually read the diff suggested by clang-format CI job.

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

Nikhil-Manglore · 2025-01-08T19:55:23Z

I just realized one of the external tests seems to fail here in the checks but always passes locally. Do you know why this could be occurring?

zuiderkwast

Thanks for your patience!

zuiderkwast · 2025-01-08T19:59:05Z

I just realized one of the external tests seems to fail here in the checks but always passes locally. Do you know why this could be occurring?

I don't know. I think some github runners are slow or there are some other problems. The failures are not related to this PR.

hpatro · 2025-01-08T20:09:27Z

Thanks @Nikhil-Manglore for working on this!

The commands used in valkey-cli tests are not important the reply schema validation. Skip them to avoid the problem if tests hanging. This has failed lately in the daily job: ``` [TIMEOUT]: clients state report follows. sock55fedcc19be0 => (IN PROGRESS) valkey-cli pubsub mode with single standard channel subscription Killing still running Valkey server 33357 ``` These test cases use a special valkey-cli command `:get pubsub` command, which is an internal command to valkey-cli rather than a Valkey server command. This command hangs when compiled with with logreqres enabled. Easy solution is to skip the tests in this setup. The test cases were introduced in #1432. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Resolves issue with valkey-cli not auto exiting from subscribed mode on reaching zero pub/sub subscription (previously filed on Redis) redis/redis#12592 --------- Signed-off-by: Nikhil Manglore <nmanglor@amazon.com> Signed-off-by: proost <jwalag87@gmail.com>

The commands used in valkey-cli tests are not important the reply schema validation. Skip them to avoid the problem if tests hanging. This has failed lately in the daily job: ``` [TIMEOUT]: clients state report follows. sock55fedcc19be0 => (IN PROGRESS) valkey-cli pubsub mode with single standard channel subscription Killing still running Valkey server 33357 ``` These test cases use a special valkey-cli command `:get pubsub` command, which is an internal command to valkey-cli rather than a Valkey server command. This command hangs when compiled with with logreqres enabled. Easy solution is to skip the tests in this setup. The test cases were introduced in valkey-io#1432. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Signed-off-by: proost <jwalag87@gmail.com>

Resolves issue with valkey-cli not auto exiting from subscribed mode on reaching zero pub/sub subscription (previously filed on Redis) redis/redis#12592 --------- Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

The commands used in valkey-cli tests are not important the reply schema validation. Skip them to avoid the problem if tests hanging. This has failed lately in the daily job: ``` [TIMEOUT]: clients state report follows. sock55fedcc19be0 => (IN PROGRESS) valkey-cli pubsub mode with single standard channel subscription Killing still running Valkey server 33357 ``` These test cases use a special valkey-cli command `:get pubsub` command, which is an internal command to valkey-cli rather than a Valkey server command. This command hangs when compiled with with logreqres enabled. Easy solution is to skip the tests in this setup. The test cases were introduced in valkey-io#1432. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

The commands used in valkey-cli tests are not important the reply schema validation. Skip them to avoid the problem if tests hanging. This has failed lately in the daily job: ``` [TIMEOUT]: clients state report follows. sock55fedcc19be0 => (IN PROGRESS) valkey-cli pubsub mode with single standard channel subscription Killing still running Valkey server 33357 ``` These test cases use a special valkey-cli command `:get pubsub` command, which is an internal command to valkey-cli rather than a Valkey server command. This command hangs when compiled with with logreqres enabled. Easy solution is to skip the tests in this setup. The test cases were introduced in #1432. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

The commands used in valkey-cli tests are not important the reply schema validation. Skip them to avoid the problem if tests hanging. This has failed lately in the daily job: ``` [TIMEOUT]: clients state report follows. sock55fedcc19be0 => (IN PROGRESS) valkey-cli pubsub mode with single standard channel subscription Killing still running Valkey server 33357 ``` These test cases use a special valkey-cli command `:get pubsub` command, which is an internal command to valkey-cli rather than a Valkey server command. This command hangs when compiled with with logreqres enabled. Easy solution is to skip the tests in this setup. The test cases were introduced in valkey-io#1432. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Fixed issue with exiting subscribe mode

c21bf9d

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

Nikhil-Manglore force-pushed the subscribe-mode-bug branch from a7fb910 to c21bf9d Compare December 12, 2024 22:41

zuiderkwast reviewed Dec 12, 2024

View reviewed changes

Comment thread src/valkey-cli.c Outdated

Comment thread src/valkey-cli.c Outdated

fixed issue with the counters when exit subscribe mode

cb71718

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

zuiderkwast reviewed Dec 18, 2024

View reviewed changes

Comment thread src/valkey-cli.c Outdated

Comment thread tests/unit/pubsub.tcl Outdated

Nikhil-Manglore added 2 commits December 19, 2024 00:17

refactored code in valkey-cli.cand fixed the test cases

926a713

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

fixed typo in the comments

2278fce

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

zuiderkwast reviewed Dec 19, 2024

View reviewed changes

Comment thread src/valkey-cli.c Outdated

Comment thread tests/integration/valkey-cli.tcl Outdated

Comment thread tests/integration/valkey-cli.tcl Outdated

Refactored code again and added new tests to the test case

0c4a177

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

zuiderkwast reviewed Dec 20, 2024

View reviewed changes

Comment thread src/valkey-cli.c Outdated

Comment thread tests/integration/valkey-cli.tcl Outdated

Nikhil-Manglore and others added 6 commits December 20, 2024 12:05

Merge branch 'valkey-io:unstable' into subscribe-mode-bug

398c482

Merge branch 'valkey-io:unstable' into subscribe-mode-bug

a047778

Merge branch 'valkey-io:unstable' into subscribe-mode-bug

791a80c

Merge branch 'valkey-io:unstable' into subscribe-mode-bug

d6c863e

Fixed all issues with subscribe mode, refactored code, and finished i…

79aff57

…mplementing test cases Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

Merge branch 'subscribe-mode-bug' of github.com:Nikhil-Manglore/valke…

6743126

…y into subscribe-mode-bug

hpatro reviewed Jan 3, 2025

View reviewed changes

Comment thread src/valkey-cli.c Outdated

Comment thread src/valkey-cli.c Outdated

Comment thread tests/integration/valkey-cli.tcl

Comment thread src/valkey-cli.c Outdated

Nikhil-Manglore added 2 commits January 6, 2025 08:43

Refactored code, added punsubscribe functionality, and made test case…

b09ef32

…s more robust Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

cleaned the code up to make it more readable

930b4db

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

zuiderkwast reviewed Jan 6, 2025

View reviewed changes

Comment thread src/valkey-cli.c Outdated

Fixed corner cases, added test case and split up existing test cases

1849195

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

zuiderkwast reviewed Jan 7, 2025

View reviewed changes

Comment thread src/valkey-cli.c Outdated

Nikhil-Manglore added 3 commits January 7, 2025 19:24

Refactored code

9f99a51

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

Removed redundant code

022b2cc

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

Removed redundant code

fe091cf

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

zuiderkwast reviewed Jan 8, 2025

View reviewed changes

Comment thread src/valkey-cli.c Outdated

hpatro approved these changes Jan 8, 2025

View reviewed changes

Comment thread src/valkey-cli.c Outdated

Comment thread src/valkey-cli.c Outdated

Comment thread src/valkey-cli.c Outdated

Comment thread src/valkey-cli.c Outdated

removed if statement and updated comments

9aff4d8

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

Nikhil-Manglore added 2 commits January 8, 2025 19:13

Fixed formatting errors

6aebdb9

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

Fixed final formatting error

fcc2742

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>

zuiderkwast approved these changes Jan 8, 2025

View reviewed changes

zuiderkwast changed the title ~~valkey-cli doesn't auto exit from subscribed mode on reaching zero subscription~~ valkey-cli auto exit from subscribed mode on reaching zero subscriptions Jan 8, 2025

zuiderkwast changed the title ~~valkey-cli auto exit from subscribed mode on reaching zero subscriptions~~ valkey-cli auto-exit from subscribed mode Jan 8, 2025

zuiderkwast merged commit 9e02049 into valkey-io:unstable Jan 8, 2025

Nikhil-Manglore deleted the subscribe-mode-bug branch January 8, 2025 22:17

zuiderkwast mentioned this pull request Jan 8, 2025

[test-failure] valkey-cli shard pubsub test #1531

Closed

Nikhil-Manglore restored the subscribe-mode-bug branch January 8, 2025 23:08

Nikhil-Manglore deleted the subscribe-mode-bug branch January 8, 2025 23:13

zuiderkwast mentioned this pull request Jan 11, 2025

Skip CLI tests with reply schema validation #1545

Merged

zuiderkwast mentioned this pull request Feb 11, 2025

Valkey release 8.1.0-rc1 #1713

Merged

Uh oh!

Conversation

Nikhil-Manglore commented Dec 12, 2024 • edited by zuiderkwast Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hpatro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zuiderkwast commented Jan 8, 2025

Uh oh!

Nikhil-Manglore commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

zuiderkwast commented Jan 8, 2025

Uh oh!

hpatro commented Jan 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Nikhil-Manglore commented Dec 12, 2024 •

edited by zuiderkwast

Loading

codecov Bot commented Dec 12, 2024 •

edited

Loading

Nikhil-Manglore commented Jan 8, 2025 •

edited

Loading