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.

Sign up for GitHub

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

Jump to bottom

A more robust way to gather facts from nodes #406

Open
doodlesbykumbi opened this issue May 17, 2021 · 7 comments · May be fixed by #408
Open

A more robust way to gather facts from nodes #406

doodlesbykumbi opened this issue May 17, 2021 · 7 comments · May be fixed by #408

Comments

@doodlesbykumbi
Copy link

I have a local setup with Redis and some application nodes. I have another redis client that is snooping on the channels that the adapter is using.

Whenever I'm snooping I get this error:

Error: timeout reached while waiting for sockets response

I looked into it and this error is because while snooping request.numSub > request.msgCount since my other client is just a spy. It would be nice if observing the channel didn't break the functionality. Perhaps there could be a way to count participant subs only.

socket.io-redis-adapter/lib/index.ts

Lines 380 to 386 in 3242d8e

if (request.msgCount === request.numSub) {
clearTimeout(request.timeout);
if (request.resolve) {
request.resolve(request.sockets);
}
this.requests.delete(requestId);
}

@darrachequesne
Copy link
Member

Perhaps there could be a way to count participant subs only.

I'm open to suggestions on this!

@doodlesbykumbi
Copy link
Author

I'll think about it a bit and see if I can suggest anything useful.

@doodlesbykumbi
Copy link
Author

@darrachequesne, so here's an idea. We can have the "active" participants subscribe to a "proxy" channel when they come online. Nothing actually goes through that particular channel, it's just used for a participant to explicitly mark themselves as a participant.

@darrachequesne
Copy link
Member

@doodlesbykumbi that's an interesting idea. That would be a backward incompatible change though, which means users will need to upgrade the whole cluster at the same time, isn't it?

@doodlesbykumbi
Copy link
Author

@darrachequesne You can fall back to the request channel for backwards compatibility if desired.

@doodlesbykumbi doodlesbykumbi linked a pull request May 18, 2021 that will close this issue
Open
@doodlesbykumbi
Copy link
Author

I've created a PR to demonstrate the approach^

@doodlesbykumbi
Copy link
Author

Ping @darrachequesne

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@doodlesbykumbi @darrachequesne