Skip to content

Conversation

NaughtySora
Copy link

@NaughtySora NaughtySora commented Sep 18, 2025

Description

Adding events for cluster client, some people were interested in it.
I have read the issue but didn't see any concrete approved suggestions so i come up with some.

for cluster:

  1. connect - when cluster ready.
  2. disconnect - when cluster is closed.
  3. error (already exists).

for nodes:

  1. node-connect - when tcp socket connected.
  2. node-ready - when tcp socket are ready.
  3. node-reconnecting - when tcp socket is reconnecting.
  4. node-disconnect - when tcp socket disconnected.
  5. node-error - when tcp socket errored.

Node events provide host/port to identify a particular node.

I would like to hear suggestions or opinion if this feature needed.

issue #1855

Checklist

  • Does npm test pass with this change (including linting)?
    I tried to run tests via npm run build and npm test but it fails with timeout "before hook" after 30000ms after 1 hour of going a couple of time. But git checks are fine i guess.

  • Is the new or changed code fully tested?
    I haven't seen any tests for emitting events, i have tested it only locally.

  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
    Can't find where i can update desctiption about cluster events.
    I saw small section of client events on redis website.
    Can't update that.

@nkaradzhov nkaradzhov self-assigned this Oct 2, 2025
@nkaradzhov
Copy link
Collaborator

@NaughtySora hi, sorry for big delay! This looks good! Could I ask you to:

  1. Add some tests here that verify the events are properly fired? I have made a PR to your branch with a template of the test
  2. Document the events in a new section called Events here. You can take inspiration from here

@nkaradzhov nkaradzhov self-requested a review October 2, 2025 11:22
@NaughtySora
Copy link
Author

Hi, thank you for the reply @nkaradzhov . I've added the test example from your PR, and I will proceed with the steps above .

@NaughtySora
Copy link
Author

I added documentation inspired by standalone client.
Test is also included but I have 2 concerns.

  1. I found only one way so far to get cluster's amount of nodes for testing. I'm looking for it in cluster._options, it can easily break encapsulation and cause troubles in the future.
  2. I have tried to add negative routes for tests but faced the fact I have no idea how to force cluster node to reconnect in test framework. So the events 'error', 'node-error' and 'node-reconnecting' haven't tested yet.

@nkaradzhov
Copy link
Collaborator

I added documentation inspired by standalone client. Test is also included but I have 2 concerns.

  1. I found only one way so far to get cluster's amount of nodes for testing. I'm looking for it in cluster._options, it can easily break encapsulation and cause troubles in the future.
  2. I have tried to add negative routes for tests but faced the fact I have no idea how to force cluster node to reconnect in test framework. So the events 'error', 'node-error' and 'node-reconnecting' haven't tested yet.
  1. Take a look here - you can specify how many masters/replicas you want your cluster. Essentially:
  testUtils.testWithCluster('foo', async cluster => {
    //tests
  }, {
    serverArguments: [],
    numberOfMasters: 3,
    numberOfReplicas: 3
  });
  1. I think its fine to leave those untested for now

@NaughtySora
Copy link
Author

I've found the way to make test more safe and changed docs a bit for more clear description.

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.

2 participants