-
Notifications
You must be signed in to change notification settings - Fork 47
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
sidecar: Init mailbox after server restarts #340
Conversation
This fixes #338 |
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.
Fix looks good to me. We could even try to init the mailbox every time we try to read from it. But then that would perhaps cause too many "already exists" errors to be logged server side. So this is a nice middle ground.
@@ -314,6 +314,23 @@ func (a *SidecarNegotiator) autoSidecarReceiver(ctx context.Context, | |||
|
|||
select { | |||
case <-retryTimer.backOff(backoffLabel): | |||
// It is possible that we were not able to receive 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.
nit: line length.
auto_sidecar.go
Outdated
// packet because the server went down. In that case, | ||
// we will try to init the sidecar mailbox again. | ||
// There is no need to log/check this error. There are | ||
// three possibilties: |
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.
nit: typo in possibilties (same below).
// iteration. | ||
// | ||
// 3) If the server is down, we won't be able to reconnect. | ||
_ = a.cfg.MailBox.InitAcctMailbox(streamID, acct.TraderKey) |
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.
👍
I think this is a good fix.
Whenever we are are unable to receive a sidecar packet in an auto sidecar negotiator try to init its mailbox. This will allow us to continue reading/writting in the mailbox in cases where the server went down and came back.
e422051
to
ac3c44a
Compare
Whenever we are are unable to receive a sidecar packet in an auto
sidecar negotiator try to reinitialize its mailbox. This will allow us to
continue reading/writing in the mailbox in cases where the error was
caused by the server going down (and already came back).