Skip to content
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

Add pubsubTopic field to index #492

Merged
merged 1 commit into from
Mar 2, 2022
Merged

Add pubsubTopic field to index #492

merged 1 commit into from
Mar 2, 2022

Conversation

jm-clius
Copy link
Contributor

@jm-clius jm-clius commented Feb 23, 2022

Adds pubsubTopic field to Index types.

This is necessary as a single store could serve multiple pubsubTopics. A cursor/index should therefore differentiate between messages that are identical across all fields, but were published on different pubsubTopics.

nim-waku changes corresponding to this RFC change in waku-org/nwaku#864

Is this backwards compatible?

Since adding a field with a new tag value to a protobuf maintains backwards compatibility, there is no need to bump the store protocol id. However, HistoryResponses will now return the pubsubTopic as part of the cursor in PagingInfo. The full PagingInfo, including pubsubTopic, must be provided in subsequent queries to the store node. A future improvement here would be to hash the cursor into a binary blob, so that clients can remain agnostic as to its contents.

@oskarth oskarth added this to New in vac-research Feb 23, 2022
@oskarth oskarth moved this from New to Review/QA in vac-research Feb 23, 2022
@jm-clius jm-clius marked this pull request as ready for review February 23, 2022 14:48
@D4nte
Copy link
Contributor

D4nte commented Feb 28, 2022

The full PagingInfo, including pubsubTopic, must be provided in subsequent queries to the store node.

This makes it not backward compatible as the client needs to be aware of the pubsubTopic field to ensure it's encoded and send to traverse the pages. So yes, the first query would work but not the following ones.

If we want to make it backward compatible, then I suggest to default to the default pubsub topiic when not present.

@jm-clius
Copy link
Contributor Author

jm-clius commented Feb 28, 2022

This makes it not backward compatible as the client needs to be aware of the pubsubTopic field to ensure it's encoded and send to traverse the pages. So yes, the first query would work but not the following ones.

Agreed. Sorry, my description should have been more clear. I meant that protobuf encoding/decoding won't break outright, which I hoped would be enough reason not to introduce another protocol id bump (trying to avoid this as it forces tightly synchronised client/fleet updates). The protocol itself will indeed need to be updated on clients for subsequent queries to work as expected, which makes the change not backwards compatible.

If we want to make it backward compatible, then I suggest to default to the default pubsub topiic when not present.

I'd prefer to avoid this in the long run, as it essentially requires awkwardly injecting data into a deserialised object and could wrongly lead to pubsubTopic being used as if it's a type of optional query criteria. The cursor should in fact be treated more as a blob of data by clients that they must use as-is in subsequent queries than a transparent object with mutable fields (in fact, I hope we can convert it to just a byte string in future). That said, if we'd rather not have a transition period where subsequent queries fail for clients we could temporarily make such a change in nwaku and just remove it after clients have been updated.

@D4nte
Copy link
Contributor

D4nte commented Mar 1, 2022

I believe this change is straightforward and can be implemented by store clients easily, even before it's implemented by nim-waku.

Cc @richard-ramos

@jm-clius jm-clius merged commit a241d3c into master Mar 2, 2022
@jm-clius jm-clius deleted the index-extension branch March 2, 2022 13:15
vac-research automation moved this from Review/QA to Done Mar 2, 2022
kaiserd added a commit that referenced this pull request Jul 16, 2022
* master:
  RFC16: add version call (#505)
  fix(noise): update RFC to implementation (#508)
  fixup: 37/WAKU2-NOISE fix images paths (#506)
  New RFC: 37/WAKU2-NOISE-SESSIONS (#504)
  36/WAKU2-BINDINGS-API (#501)
  docs(16/WAKU2-RPC): add ENR to waku info (#502)
  Adding 35/WAKU2-NOISE to menu (#500)
  add RFC33 to index (#499)
  feat: 32/RLN raw spec
  New RFC: 35/WAKU2-NOISE (#496)
  Update on the rln registration figure to match the current spec (#497)
  33/WAKU-DISCV5: Add first raw version (#487)
  Add pubsubTopic field to index (#492)
  Fix markdown links (#493)
  Categorize 22 & 31 (#490)
  Changed PB Timestamp index to 10 (#491)
  13/14/16/21: Change in timestamp format (#483)
  add: RFC31 copyright statement (#489)
  17/WAKU-RLN-RELAY: Revise spec for its draft version (#484)
kaiserd added a commit that referenced this pull request Jul 25, 2022
* master:
  RFC16: add version call (#505)
  fix(noise): update RFC to implementation (#508)
  fixup: 37/WAKU2-NOISE fix images paths (#506)
  New RFC: 37/WAKU2-NOISE-SESSIONS (#504)
  36/WAKU2-BINDINGS-API (#501)
  docs(16/WAKU2-RPC): add ENR to waku info (#502)
  Adding 35/WAKU2-NOISE to menu (#500)
  add RFC33 to index (#499)
  feat: 32/RLN raw spec
  New RFC: 35/WAKU2-NOISE (#496)
  Update on the rln registration figure to match the current spec (#497)
  33/WAKU-DISCV5: Add first raw version (#487)
  Add pubsubTopic field to index (#492)
  Fix markdown links (#493)
  Categorize 22 & 31 (#490)
  Changed PB Timestamp index to 10 (#491)
  13/14/16/21: Change in timestamp format (#483)
  add: RFC31 copyright statement (#489)
  17/WAKU-RLN-RELAY: Revise spec for its draft version (#484)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants