-
Notifications
You must be signed in to change notification settings - Fork 59
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
Added reconnection logic when NATS is disconnected #49
Added reconnection logic when NATS is disconnected #49
Conversation
e110413
to
c747603
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.
Yes, this is what I had in mind for the mutex. Would still return result from nc.Publish() directly. Also, you did not address the logging when exhausting the number of reconnect. Granted that if the attempts are statically defined and you do log reconnect attempts, the absence of success logging implies the failure. But if you end-up making the attempts count user settable, then I would recommend logging the total failure.
8ce810d
to
8ea739e
Compare
@kozlovic I think that all of your comments are done now. Can you check again, please? |
@alexellis if we want to have two of the values (maxReconnect and delayBetweenReconnect) as a part of NatsConfig, we'll need to change it in gateway. I have created new function with defaults |
@bartsmykla I can comment only on the usage of the NATS Streaming API and that looks good to me! |
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.
Looking very positive so far. This work would need to be done in main.go too for the subscriber (nats-queue-worker itself)?
8ea739e
to
9bbc768
Compare
3c67929
to
01581b6
Compare
Hi Bart, good work on this 👍 Ivan thanks so much for your help here. Do you have a Docker image pushed that I can use to test the change locally? Alex |
01581b6
to
8dfa764
Compare
Quick question - (What output do you get when you scale the gateway from 1 to 0 replicas in the logs? Do you see it try to reconnect?) Alex |
@alexellis image is: |
@alexellis the change is for gateway, so when it's scaled down to 0, there is no logs related to reconnection |
Yes that's why I asked to scale the gateway down and watch the logs. Please could you try that and let us know if any logs about reconnecting are generated? This question is relating to Ivan's comment on shutdown. |
They are not (I checked that before, so my comment was written after me checking). There is no logs after. Container is just killed and that's it. |
How was it tested: I deployed this version of nats-queue-worker and then simulated NATS disconnection Signed-off-by: Bart Smykla <[email protected]>
8dfa764
to
cbed5ba
Compare
Thanks for your work on this Bart and @kozlovic for lending your expertise. This is now merged for the producer (gateway) and will be released. |
I've tested out the changes with a new gateway and it seems like the reconnect doesn't work with the in-memory store at all. Is this to be expected? Also the buffer seemed to fill up with a tiny request while the NATS connection was severed?
The result is after scaling NATS Streaming to 0 and back to 1 is:
|
If there is a request (calling Queue()) while the server is down and the connection is marked as closed, this is expected that the Publish() call would fail. How the returned error makes your software behave, that I don't know. It is also possible that even if the reconnect occurs, the Queue() still has a reference to the previous (now closed) connection and goes into a Publish() that would also result in error. |
I'll test it tomorrow, but before NATS server will reconnect |
Actually, you should get some error about |
It should resolve the issue: openfaas/faas#1031 and also resolves issues from: #33
Signed-off-by: Bart Smykla [email protected]
Description
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist:
git commit -s