-
Notifications
You must be signed in to change notification settings - Fork 248
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
chore: use storev3 instead of v2 for history queries #5123
Conversation
|
Jenkins BuildsClick to see older builds (75)
|
|
2 similar comments
|
|
|
|
|
wakuv2/waku.go
Outdated
@@ -126,7 +127,7 @@ type Waku struct { | |||
storeMsgIDs map[gethcommon.Hash]bool // Map of the currently processing ids | |||
storeMsgIDsMu sync.RWMutex | |||
|
|||
connStatusChan chan node.ConnStatus | |||
topicHealthStatusChan chan peermanager.TopicHealthStatus |
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.
This is confusing, is this topic only relates to pubsub topic
? if yes, we better make it explicit in peermanager and here.
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.
For sure the usage of the topicHealtStatus channel needs to be improved. I only added the minimum needed code so the existing functionality is maintained since the connection status feature was removed from go-waku as it did not make sense in the context of having multiple pubsub topics.
Proper usage of this channel must be implemented, following recommendations specified in #4628
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.
👍
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.
LGTM
TODO: must be rebased against #5150 |
|
1 similar comment
|
|
602efe2
to
0df2111
Compare
|
d836289
to
b6e7ccd
Compare
Requires nwaku v0.29 |
Requires waku-org/nwaku#2745 |
56666ec
to
22c426e
Compare
NOTE: we can't merge this until the code's been dogfooded with the fleet. The fleet does not support storev3 in its current installed version.
Also notice that even though the code is using the topic health status channel, it's usage does not follow the recommendations from #4628 . The behavior on the code is similar to the Conn Status Notification channel it had before.