Feat/autoshard filter #723
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (106)
|
|
@jakubgs , some CI checks are failing due to 500 error. Tried re-running it, still seeing failure. |
|
Fixed. |
2 tasks
There was a problem hiding this comment.
Looks good, however, where pubsub topic is marked as Optional in the different READMEs and comments, I think we need to add add some note indicating that pubsub topics are considered mandatory when used with named and static sharding, and not when using autosharding (in the case of the READMEs), and expand more on what does it mean for the pubsub topic to be optional?
Makes me wonder if we should convert ContentFilter into an interface and implementations both for named/static sharding and autosharding, with the former requiring the PubsubTopic? i.e.:
// Names are just an example
contentFilterA := NewAutoshardingContentFilter("theContentTopic)
subs, err := node.FilterLightClient().Subscribe(ctx, contentFilterA)
contentFilterB := NewContentFilter("thePubsubTopic", []string{"theContentTopics"...}
subs, err := node.FilterLightClient().Subscribe(ctx, contentFilterB)The idea would be to allow the pubsub topic to be optional, but make it harder for the library user to use the pubsub topic incorrectly.
I left also some additional comments and questions
Will update Readme's. I do like the idea of making contentFilter in a way so that people cannot misuse. But, rather than making this change within the core filter code, it would be better to do it at API side. Otherwise it won't give us flexibility we want within core code. WDYT? |
richard-ramos
approved these changes
Sep 19, 2023
Sounds good 🚀 |
Will take care of this in a separate PR, as this PR has been open for too long and has too many merges. |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Update Filter client API to work with Autosharding logic. #673
A client user can specify the following options when subscribing/unsubscribing from filter.
Note: In order to not bloat this PR, optional pubSubTopic handing in MessagePush shall be done in separate PR.
Changes
Tests
Added new Filter tests to validate Autosharding.
Same can be executed by running below command in
waku/v2/protocol/filter directorygo test -testify.m TestAutoShard