feat: implement monitor command#483
feat: implement monitor command#483ArturSharapov wants to merge 2 commits intodenodrivers:masterfrom
Conversation
There was a problem hiding this comment.
@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.
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.
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.
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.
@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
reuseConnectionoption tomonitor(). - Make
Connection#duplicatea private API for now- I'll implement a connection pool in this library and would like to keep
Connection#duplicateprivate until then - e.g.)
Line 51 in ad96f2c
- I'll implement a connection pool in this library and would like to keep
|
@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: