-
Notifications
You must be signed in to change notification settings - Fork 687
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
base: unstable
Are you sure you want to change the base?
RESP3 PUSH support for MONITOR command #1426
Conversation
Signed-off-by: KowalczykBartek <[email protected]>
30c1c7e
to
6babded
Compare
There was a problem hiding this 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.
$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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Add support for MONITOR command for the RESP3 push type, so, instead of
the connection after sending hello 3 and monitor will receive push messages on the form
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