-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
feat: implement monitor command #483
base: master
Are you sure you want to change the base?
Conversation
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.
@ArturSharapov Thanks for opening the PR! The implementation looks good overall. Just one comment.
monitor() { | ||
throw new Error("not supported yet"); | ||
async monitor() { | ||
const connection = this.executor.connection.duplicate(); |
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.
I think it would be better not to duplicate the connection here for the following reasons:
- to keep consistency with
subscribe()
- node-redis also works that way
What do you think about this?
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.
I've been using ioredis historically (which also seems to be 1.5+ times more popular than node-redis on npm).
Unlike node-redis, it does duplicate the connection by default on monitor()
which I think was rational because all other commands become unavailable then.
The only positive side of using a single connection I can see is to cover the case when one would want to create redis connection only to perform monitoring and nothing else.
But this comes with a cost, making it dangerous, as it easily leads to unexpected behavior, blocking execution of all other redis commands. For those who are not experienced with redis much or for those coming from ioredis
, it would become unclear, why some redis commands just get stuck and take forever to execute; in a large codebase it would be especially challenging to debug. It's also unclear whether the commands you call are accumulated and executed once you terminate the monitor (which can only be done manually), or they are just ignored entirely.
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.
I would advocate for the duplication of connection as it make it simpler and doesn't "break all things", as well as eliminates potential issues and unforeseen behavior.
That said, what about brining an argument (option) to the monitor()
that could make it use the same connection?
To cover the case when one needs to have redis only for monitoring and doesn't need a regular working redis instance to use the rest of the API.
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.
@ArturSharapov I'm in favor of adding an option to the monitor()
👍
We are planning to support RESP3 in the future, and it would be ideal to have an option of not duplicating a connection to monitor()
.
I have two suggestions:
- Add
reuseConnection
option tomonitor()
. - Make
Connection#duplicate
a private API for now- I'll implement a connection pool in this library and would like to keep
Connection#duplicate
private until then - e.g.)
Line 51 in ad96f2c
[kUnstableReadReply](returnsUint8Arrays?: boolean): Promise<RedisReply>;
- I'll implement a connection pool in this library and would like to keep
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.
Good call 👍
@ArturSharapov To run tests locally, you'll need to install Alternatively, CI will also run tests, so you can take advantage of that. |
Reference
Proposed changes
First implemented with a few lines of code, but then extended taking PubSub as an example to meet the project structure/style, reconnection policy, etc.
@uki00a I would appreciate your assistance with covering it with tests, as I couldn't get them running locally.
Example:
Expected output: