Skip to content

fix ssubscribe command#178

Merged
bjosv merged 21 commits intovalkey-io:mainfrom
VyacheslavVanin:fix-ssubscribe
Apr 25, 2025
Merged

fix ssubscribe command#178
bjosv merged 21 commits intovalkey-io:mainfrom
VyacheslavVanin:fix-ssubscribe

Conversation

@VyacheslavVanin
Copy link
Copy Markdown
Contributor

@VyacheslavVanin VyacheslavVanin commented Mar 10, 2025

Support ssubscribe command

Fixes: #177

@bjosv
Copy link
Copy Markdown
Collaborator

bjosv commented Mar 11, 2025

Thanks!
It would be great if we could add a test, but I'm not sure if we have any slots assigned when running client_test.c.. test_pubsub_handling_resp3 could be a good starting point in that case.

In comparison to subscribe and psubscribe the ssubscribe command can return -MOVED. Do we just return this response to the user or how is it handled?

@VyacheslavVanin
Copy link
Copy Markdown
Contributor Author

Yes, you are right. I wanted that MOVED just be returned to users, but seems like a callback is not called in case of error.
I will try to fix this

@VyacheslavVanin VyacheslavVanin force-pushed the fix-ssubscribe branch 2 times, most recently from 68d0cc5 to 5fab92c Compare March 20, 2025 11:32
@VyacheslavVanin
Copy link
Copy Markdown
Contributor Author

I turned off sharded_pubsub_test in ci-workflows that don’t run clusters

@VyacheslavVanin
Copy link
Copy Markdown
Contributor Author

@bjosv Hi! Can we run tests again?

@VyacheslavVanin
Copy link
Copy Markdown
Contributor Author

@bjosv Hi! Added error handling and tests. Errors are now returned to users.

@VyacheslavVanin
Copy link
Copy Markdown
Contributor Author

Hi @bjosv, could you please rerun the tests again? Last time test passed but after that needed to resolve conflicts

@bjosv
Copy link
Copy Markdown
Collaborator

bjosv commented Apr 23, 2025

Hi @bjosv, could you please rerun the tests again? Last time test passed but after that needed to resolve conflicts

Sorry, missed it. I will look into your PR a bit more.

Copy link
Copy Markdown
Collaborator

@bjosv bjosv left a comment

Choose a reason for hiding this comment

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

Nice, looks like it should work. I just have some change requests about how we need to disable the new tests. I would like that we need to enable them instead. It might be that some users only uses the Makefile and then it might not work when testing with TEST_ASYNC, i.e. changing --skip-cluster-tests to --enable-cluster-tests.

Sorry for slow response; just tell me if your busy then I can make these changes afterwards or so.

Comment thread tests/test.sh Outdated
Comment thread tests/test.sh Outdated
Comment thread .github/workflows/build.yml Outdated
Comment thread src/async.c
Comment thread .github/workflows/build.yml Outdated
Comment thread tests/CMakeLists.txt Outdated
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
Signed-off-by: Vyacheslav Vanin <vaninvv@yandex-team.ru>
@bjosv bjosv merged commit 88b214d into valkey-io:main Apr 25, 2025
46 checks passed
@bjosv
Copy link
Copy Markdown
Collaborator

bjosv commented Apr 25, 2025

Had to fix some weird Github issue by rebasing and was able to merge now. Thanks.

@VyacheslavVanin VyacheslavVanin deleted the fix-ssubscribe branch April 25, 2025 09:11
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.

SSUBSCRIBE does not work

2 participants