feat!: extract storenode cycle to go-waku api #5857
Conversation
Jenkins BuildsClick to see older builds (100)
|
richard-ramos
force-pushed
the
feat/mailserver-cycle
branch
2 times, most recently
from
1138045
to
f5dc5cb
Compare
richard-ramos
force-pushed
the
feat/mailserver-cycle
branch
from
f5dc5cb
to
7539682
Compare
richard-ramos
requested review from
plopezlpz,
Samyoul,
kaichaosun,
chaitanyaprem,
igor-sirotin and
osmaczko
richard-ramos
force-pushed
the
feat/mailserver-cycle
branch
from
7539682
to
f4aa80e
Compare
richard-ramos
changed the title
richard-ramos
changed the title
richard-ramos
force-pushed
the
feat/mailserver-cycle
branch
from
f4aa80e
to
d9d1247
Compare
richard-ramos
force-pushed
the
feat/mailserver-cycle
branch
2 times, most recently
from
9f4b082
to
ef3f160
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5857 +/- ##
============================================
+ Coverage 13.65% 60.86% +47.21%
============================================
Files 805 820 +15
Lines 107999 109189 +1190
============================================
+ Hits 14751 66462 +51711
+ Misses 91334 34902 -56432
- Partials 1914 7825 +5911
Flags with carried forward coverage won't be shown. Click here to find out more.
|
richard-ramos
force-pushed
the
feat/mailserver-cycle
branch
from
ef3f160
to
b69a3ea
Compare
richard-ramos
force-pushed
the
feat/mailserver-cycle
branch
2 times, most recently
from
65d7a85
to
a06eb72
Compare
eth-node/bridge/geth/wakuv2.go
Outdated
| TimeStart: proto.Int64(int64(batch.From) * int64(time.Second)), | ||
| TimeEnd: proto.Int64(int64(batch.To) * int64(time.Second)), |
There was a problem hiding this comment.
Would be nice to refactor MailserverBatch to have From/To fields as time.Time to avoid such weird conversions. Considering that we do it initially as well:
status-go/protocol/messenger_mailserver.go
Lines 175 to 177 in a06eb72
And then we have to convert them once again here, when passing to go-waku...
There was a problem hiding this comment.
I did this change, but while working on it, i realized the madness of how we handle time in status-go: we use uint64 for clock values (milliseconds), on waku we use time.Time, on mailserver code and message gaps calculation we use uint32 (in seconds, i imagine this is 'inherited' from back when we used to have this logic on the mobile app).
This lack of consistency is awful. In a subsequent PR i'll attempt to get rid of the uint32 code for mailserver code and gaps to use time.Time, but perhaps we should consider to later also attempt to get rid of the milliseconds time (uint64) and use time.Time consistently across all codebase?
There was a problem hiding this comment.
use time.Time consistently across all codebase
💯
protocol/messenger.go
Outdated
| go func() { | ||
| defer gocommon.LogOnPanic() | ||
| <-available | ||
| <-m.transport.OnStorenodeAvailableOneShot() |
There was a problem hiding this comment.
We probably need some <-ctx.Done() here as well?
Not related to your change, but would be nice to fix on the way.
There was a problem hiding this comment.
Actually, why not WaitForAvailableStoreNode here?
A function like OnStorenodeAvailableOneShot is quite specific. And only used here, but could easily be replaced with WaitForAvailableStoreNode and infinite timeout 🤔
eth-node/bridge/geth/wakuv2.go
Outdated
| func (w *gethWakuV2Wrapper) WaitForAvailableStoreNode(timeout time.Duration) bool { | ||
| return w.waku.StorenodeCycle.WaitForAvailableStoreNode(context.TODO(), timeout) | ||
| } |
There was a problem hiding this comment.
context should probably passed as argument to WaitForAvailableStoreNode
| func (w *gethWakuV2Wrapper) WaitForAvailableStoreNode(timeout time.Duration) bool { | |
| return w.waku.StorenodeCycle.WaitForAvailableStoreNode(context.TODO(), timeout) | |
| } | |
| func (w *gethWakuV2Wrapper) WaitForAvailableStoreNode(ctx context.Context, timeout time.Duration) bool { | |
| return w.waku.StorenodeCycle.WaitForAvailableStoreNode(ctx, timeout) | |
| } |
protocol/messenger_communities.go
Outdated
|
|
||
| // getCommunityMailserver returns the active mailserver if a communityID is present then it'll return the mailserver | ||
| // for that community if it has a mailserver setup otherwise it'll return the global mailserver | ||
| func (m *Messenger) getCommunityMailserver(communityID ...string) peer.ID { |
There was a problem hiding this comment.
At least for the new functions 😄
| func (m *Messenger) getCommunityMailserver(communityID ...string) peer.ID { | |
| func (m *Messenger) getCommunityStorenode(communityID ...string) peer.ID { |
protocol/messenger_mailserver.go
Outdated
|
|
||
| return m.findNewMailserver() | ||
| } | ||
| go func() { |
There was a problem hiding this comment.
| go func() { | |
| go func() { | |
| defer gocommon.LogOnPanic() |
There was a problem hiding this comment.
Also, pass some context to PeformStorenodeTask?
| if pinnedMailserver == nil { | ||
| m.disconnectActiveMailserver(graylistBackoff) | ||
| } | ||
| func (m *Messenger) performStorenodeTask(task func() (*MessengerResponse, error), opts ...history.StorenodeTaskOption) (*MessengerResponse, error) { |
There was a problem hiding this comment.
This wrapper looks quite difficult. We start a goroutine that has a callback that calls a task, whilst receiving result/error through channels 😵
Any chances to simplify this somehow?
There was a problem hiding this comment.
Should be simpler now!
protocol/messenger_mailserver.go
Outdated
| @@ -1172,13 +917,13 @@ func (m *Messenger) fetchMessages(chatID string, duration time.Duration) (uint32 | |||
| return nil, nil | |||
| } | |||
|
|
|||
| m.logger.Debug("fetching messages", zap.String("chatID", chatID), zap.String("mailserver", ms.Name)) | |||
| m.logger.Debug("fetching messages", zap.String("chatID", chatID), zap.Stringer("storenodeID", peerID)) | |||
There was a problem hiding this comment.
Never heard of storenodeID before 😄
| m.logger.Debug("fetching messages", zap.String("chatID", chatID), zap.Stringer("storenodeID", peerID)) | |
| m.logger.Debug("fetching messages", zap.String("chatID", chatID), zap.Stringer("peerID", peerID)) |
protocol/storenodes/storenodes.go
Outdated
| if err == nil { | ||
| if commStorenodeID == peerID { | ||
| return true | ||
| } |
There was a problem hiding this comment.
| if err == nil { | |
| if commStorenodeID == peerID { | |
| return true | |
| } | |
| if err == nil && commStorenodeID == peerID { | |
| return true |
| func (api *PublicAPI) DisconnectActiveMailserver() { | ||
| api.service.messenger.DisconnectActiveMailserver() | ||
| } | ||
|
|
There was a problem hiding this comment.
This counts as a breaking change, please mark the commit accordingly
There was a problem hiding this comment.
Good job moving the logic to go-waku, it fits well there 👍
I believe this PR carries a high risk and should be thoroughly reviewed by QAs from both platforms before merging. I've just requested a review from the QAs. Are there any specific instructions or steps that should be followed to ensure the affected areas are tested properly?
osmaczko
requested review from
a team and
glitchminer
and removed request for
a team
@osmaczko, thank you for reaching out to the QA team. We're currently preparing to cut the release branch for the upcoming 2.31 version. Since we want to avoid including any risky changes in this release, we will be ready to test this PR once the 2.31 branch is finalized and released. @richard-ramos meanwhile, we would appreciate if you create a corresponding mobile PR pointing at these changes and providing some details on what areas are affected and should be tested in scope of this PR. Thank you! |
richard-ramos
force-pushed
the
feat/mailserver-cycle
branch
from
384310e
to
1658d4a
Compare
|
Looks like you have BREAKING CHANGES in your PR. Check-list
|
|
@igor-sirotin i think I managed to implement/fix all of your suggestions and observations. If this PR gets your blessings I'll proceed to create PRs for desktop/mobile QA. Thank you! |
igor-sirotin
changed the title
richard-ramos
force-pushed
the
feat/mailserver-cycle
branch
from
1960b97
to
c17f9ef
Compare
richard-ramos
force-pushed
the
feat/mailserver-cycle
branch
from
c17f9ef
to
12a0c7c
Compare
|
@richard-ramos is it ready to be tested? can you please update status-im/status-mobile#21494 accordingly and move PR to Thanks! |
richard-ramos
force-pushed
the
feat/mailserver-cycle
branch
from
12a0c7c
to
62d4ce9
Compare
|
@churik , done! i've just rebased the PR and moved it to the e2e column. |
Hey @richard-ramos! Thanks for the PR. In case you have missed, I have logged some issues in mobile PR status-im/status-mobile#21494 (comment) , please take a look once you have a chance. |
richard-ramos
force-pushed
the
feat/mailserver-cycle
branch
from
62d4ce9
to
e9c399b
Compare
richard-ramos
force-pushed
the
feat/mailserver-cycle
branch
from
033a445
to
7959e55
Compare
|
@richard-ramos tested on mobile in status-im/status-mobile#21494. No issues from my side. |
Requires:
Extracts the storenode cycle code to go-waku. This PR probably makes sense to be reviewed along with the one from go-waku ^. It's mostly ready to be reviewed. I'll change it to draft once go-waku's PR is complete (it is just missing the unit tests).