chore: waku sync full topic support #57
Conversation
There was a problem hiding this comment.
LGTM. Comment to address before merging below - we have to be explicit about "topic" as being pubsub topic, content topic or both. Suggestion is to limit spec only to behaviour for pubsub topic, which will allow it to undergo full testing and QA separately to reach draft status while we experiment with content topics in parallel (which IIUC may necessitate more comprehensive storage changes).
standards/core/sync.md
Outdated
| The `reconciliation` protocol follows the following heuristic: | ||
| 1. The requestor chooses a time range to sync. | ||
| The `reconciliation` protocol operate on the following heuristic: | ||
| 1. The requestor chooses a sync range including time, cluster id and topics. |
There was a problem hiding this comment.
Presumably, with "topics" here you refer to both pubsub topics and content topics? We should be explicit about this here and elsewhere, but I wonder if we keep the spec limited to pubsub topics (only)? Even if in implementation we add experimental content topics support, my guess is that we'll mature/test the pubsub topics implementation before validating that it works for content topics. At that point, the spec can be upgraded to draft, while a new raw version can include content topics. If you agree, the suggested change then is to be explicit about "pubsub topic" throughout.
There was a problem hiding this comment.
Presumably, with "topics" here you refer to both pubsub topics and content topics?
I agree being explicit is better.
This PR reflects the changes in this waku-org/nwaku#3275
Both should be merged together ounce we have tested the current version (which include shard support).
Since both pubsub and content topic use the exact same mechanism testing separately doesn't make much sense.
There was a problem hiding this comment.
Not sure I agree, at least in terms of how we verify that this is "production-ready" - the logic might be the same, but the application will be very different. In "normal" use cases, we should see syncing of up to ~10 shards. It would be good to test the upper limit here. In contrast, content topics will have no upper bound and regular use cases would routinely try to sync 100 content topics. For QA to pass on Waku Sync, only the relatively modest use cases for pubsub topics need to work.
update to the Waku sync spec to include full topic support.