feat: waku sync full topic support #3275
Conversation
|
You can find the image built from this PR at Built from 9abc1c1 |
SionoiS
force-pushed
the
feat--sync-shard-full-support
branch
from
47e7f58 to
8b0951b
Compare
|
Future @SionoiS don't merge yet. Wait for testing. Fixes would be easier without this new code. |
There was a problem hiding this comment.
Generally direction makes sense to me. Thanks! Indeed agree that this should be merged only after the "simpler" shards support has been tested alongside basic sync functionality.
One admittedly controversial thing to consider (not for this PR): we couple the protocol very tightly to the understanding of a node being connected to a single cluster with multiple shards. This view is correct, for now, but I'm wondering if this network-level understanding of what "cluster" and "shard" represent is appropriate for lower-layer protocols. In other words, if for any reason in future we want to support multiple clusters organically, the Sync protocol will be affected. Another approach would be to simply keep using the fully qualified pubsub topic (cluster+shard or even pubsub topic hash) as dimension alongside the message ID. This will decouple the sync protocol from any particular understanding of how the network sharding design is done.
waku/waku_store_sync/transfer.nim
Outdated
| @@ -34,7 +35,7 @@ type SyncTransfer* = ref object of LPProtocol | |||
| peerManager: PeerManager | |||
|
|
|||
| # Send IDs to reconciliation protocol for storage | |||
| idsTx: AsyncQueue[SyncID] | |||
| idsTx: AsyncQueue[(SyncID, uint16)] | |||
There was a problem hiding this comment.
Not for this PR, but these uint16 makes me think that we need a type Shard* = uint16 somewhere.
Yes this a very good point! I'm almost done with the content topic support and switching to pubsub topic instead of shard is trivial. I will do that. |
SionoiS
changed the title
SionoiS
requested review from
jm-clius,
darshankabariya,
gabrielmer,
NagyZoltanPeter and
Ivansete-status
| @@ -208,29 +208,24 @@ proc mountSharding*( | |||
|
|
|||
| proc mountStoreSync*( | |||
| node: WakuNode, | |||
| cluster: uint16, | |||
| shards: seq[uint16], | |||
| contentTopics: seq[string], | |||
There was a problem hiding this comment.
Just to make sure I understand, this means that all participating nodes would need to express a bounded content topic interest to sync on content topic level? In other words, we are not yet building something that will replace a "server-client" model where the service node supports "all" content topics (on set of shards) and only the client expresses an interest over which content topics to sync?
There was a problem hiding this comment.
Empty list is a wildcard accept all topics.
edit: let me check to be sure
edit2: I fixed a logic flaw in the pre-processing of topics.
Description
When syncing take into account the topics a node is interested in. Basically sync on the topic set intersection of the 2 nodes.
Omitting topics means wildcard, use all topics.
Also, trim the time range a node will sync based on interest.