Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RESP3 PUSH support for MONITOR command #1426

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from

Conversation

KowalczykBartek
Copy link

@KowalczykBartek KowalczykBartek commented Dec 11, 2024

Add support for MONITOR command for the RESP3 push type, so, instead of

+1733939274.692107 [0 127.0.0.1:51014] "get" "a"

the connection after sending hello 3 and monitor will receive push messages on the form

>2
$7
monitor
+1733939225.708882 [0 127.0.0.1:51014] "get" "a"

This makes it possible to send commands while the client is in monitoring mode.

@zuiderkwast adviced to reopen original pr here redis/redis#11216 (comment) -
So, I am :)

and issue: #1428

Signed-off-by: KowalczykBartek <[email protected]>
@zuiderkwast zuiderkwast changed the title RESP3 support for MONITOR command RESP3 PUSH support for MONITOR command Dec 11, 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.

Hello! Nice!

Clang-format complains about missing space after if and after comma.

Comment on lines +304 to +312
$rd HELLO 3
$rd read ; # Consume the HELLO reply

$rd monitor
$rd read ; # Consume the MONITOR reply

r set foo bar

assert_match {monitor*"set"*"foo"*"bar"*} [$rd read]
Copy link
Contributor

Choose a reason for hiding this comment

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

To check that we actually get RESP3 pushes, we could use $rd readraw 1 and verify that we get the correct RESP3 encoded replies.

We should probably also check that we can send regular commands (for example PING) while monitoring.

Copy link
Author

@KowalczykBartek KowalczykBartek Dec 11, 2024

Choose a reason for hiding this comment

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

We should probably also check that we can send regular commands (for example PING) while monitoring.

that's was good idea, I think I found a bug (I am playing with my toy client respy ), let me fix your comment, write some test and describe behaviour with monitor+regular command.

btw, thanks for super fast reply, in redis repo it took 2years ;)

EDIT:
there is no bug in valkey but in my poor implementation (client), on PING, I received correct PONG and the monitor push

+--------+-------------------------------------------------+----------------+
|00000000| 2b 50 4f 4e 47 0d 0a 3e 32 0d 0a 24 37 0d 0a 6d |+PONG..>2..$7..m|
|00000010| 6f 6e 69 74 6f 72 0d 0a 2b 31 37 33 33 39 35 38 |onitor..+1733958|
|00000020| 38 31 39 2e 38 39 37 37 36 34 20 5b 30 20 31 32 |819.897764 [0 12|
|00000030| 37 2e 30 2e 30 2e 31 3a 35 39 32 33 33 5d 20 22 |7.0.0.1:59233] "|
|00000040| 50 49 4e 47 22 0d 0a                            |PING"..         |
+--------+-------------------------------------------------+----------------+

Philosophical question:
should this be allowed ?

HELLO 3 //resp3 on
CLIENT TRACKING on
MONITOR
GET somekey

so, two push operations on the single connection. I am asking because right now, I receive

         +-------------------------------------------------+
         |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f |
+--------+-------------------------------------------------+----------------+
|00000000| 2d 45 52 52 20 52 65 70 6c 69 63 61 20 63 61 6e |-ERR Replica can|
|00000010| 27 74 20 69 6e 74 65 72 61 63 74 20 77 69 74 68 |'t interact with|
|00000020| 20 74 68 65 20 6b 65 79 73 70 61 63 65 0d 0a    | the keyspace.. |
+--------+-------------------------------------------------+----------------+

and tracking is silently closed

Copy link
Contributor

Choose a reason for hiding this comment

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

The answer to the philosophical question is just boring, not very philosophical. Monitor clients are flagged as replicas internally. I don't know why it's like that, but that's why you can't use GET. My guess is that in some old implementation it was the same code. We can change it if nothing breaks.

Can you try to find out by reading source code? If we don't flag monitor clients as replicas, do any test cases break?

Copy link
Author

Choose a reason for hiding this comment

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

Can you try to find out by reading source code? 

I will play around this and get back :)

Copy link
Author

@KowalczykBartek KowalczykBartek Dec 12, 2024

Choose a reason for hiding this comment

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

@zuiderkwast do you know maybe, if I can run some subset of tests, single unit tests file against my standalone redis ? Lets say I started my redis and want to run against it a tests/unit/introspection.tcl , is that possible ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it's possible.

./runtest --help is useful.

--single unit/introspection to run one test suite.

Copy link
Contributor

Choose a reason for hiding this comment

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

The test suite normally starts the server. If you want to run against an already started server, I think it's possible, but I don't know exactly how.

@zuiderkwast zuiderkwast added the major-decision-pending Major decision pending by TSC team label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-decision-pending Major decision pending by TSC team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants