-
Notifications
You must be signed in to change notification settings - Fork 160
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
BR: reconnect to dispatcher support #3040
Conversation
3da14e1
to
1fb8ac7
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.
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sgmonroy)
a discussion (no related file):
Code looks good. Did you manually test this somehow?
a discussion (no related file):
Note that this enables reconnects only on the one snetConn
. I think it's sufficient, but just pointing it out to be certain it's intended.
a discussion (no related file): You can avoid reconnecting forever at the start by passing in a timeout to the |
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @scrye)
a discussion (no related file):
Previously, scrye (Sergiu Costea) wrote…
Code looks good. Did you manually test this somehow?
Yes, manual check . I ended up running acceptance test setup, kill the dispatcher, check BR log error, start dispatcher, check BR sends BR IfStateReq packets.
a discussion (no related file):
Previously, scrye (Sergiu Costea) wrote…
Note that this enables reconnects only on the one
snetConn
. I think it's sufficient, but just pointing it out to be certain it's intended.
Yes, there is only a single snet connection from the control process.
a discussion (no related file):
Previously, scrye (Sergiu Costea) wrote…
Also, note that this will try to reconnect forever. There are some use cases where this is awkward (e.g., if the dispatcher socket is incorrect, a BR that starts up will never error out completely). Is this intended?
You can avoid reconnecting forever at the start by passing in a timeout to the
Listen
.
BR control is just another infra service and I think at the moment consistent behavior with other infra services.
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.
Reviewable status: complete! all files reviewed, all discussions resolved
1fb8ac7
to
963c763
Compare
Add logic to support automatic reconnect to dispatcher in case of socket error. The feature is enable in the config with the ReconnectToDispatcher option in the General config section.
963c763
to
ab46da4
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.
Reviewed 1 of 1 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved
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.
Reviewable status: complete! all files reviewed, all discussions resolved
This change is