-
Notifications
You must be signed in to change notification settings - Fork 249
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
Small mailserver async fetching refactor #5420
Conversation
Jenkins BuildsClick to see older builds (20)
|
38a6bd0
to
ed34eec
Compare
protocol/messenger.go
Outdated
@@ -1503,18 +1503,14 @@ func (m *Messenger) watchConnectionChange() { | |||
// Instead we will poll the connection status. | |||
m.logger.Warn("using WakuV1, can't watch connection changes, this might be have side-effects") | |||
go pollConnectionStatus() | |||
return | |||
} | |||
} else { |
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 actually think return
is better comparing to else
.
It's a check of err != nil
, so returning makes sense there. If we will have to add more code with conditions after go subscribedConnectionStatus(subscription)
, the tabulation will grow to the right.
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.
Agree, fixed.
m.logger.Error("could not perform mailserver request", zap.Error(err)) | ||
} | ||
}() | ||
m.asyncRequestAllHistoricMessages() |
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.
asyncRequestAllHistoricMessages
doesn't use performMailserverRequest
.
Not sure if we want it, but noting the difference.
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.
Sorry, this one i don't quite understand - asyncRequestAllHistoricMessages
does invoke performMailserverRequest
here
status-go/protocol/messenger_mailserver.go
Line 362 in e1e04e6
response, err := m.performMailserverRequest(ms, func(ms mailservers.Mailserver) (*MessengerResponse, error) { |
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.
oh, I got confused
So we were calling performMailserverRequest
-> RequestAllHistoricMessages
-> performMailserverRequest
😵💫
I guess then it's ok now
ed34eec
to
e1e04e6
Compare
m.logger.Error("could not perform mailserver request", zap.Error(err)) | ||
} | ||
}() | ||
m.asyncRequestAllHistoricMessages() |
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.
oh, I got confused
So we were calling performMailserverRequest
-> RequestAllHistoricMessages
-> performMailserverRequest
😵💫
I guess then it's ok now
4b86906
to
d5bd177
Compare
d5bd177
to
3dc67e2
Compare
Two small refactors extracted from #5292