-
Notifications
You must be signed in to change notification settings - Fork 51
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
fix: introduce new field for shards in metadata protocol #2511
Conversation
…ing change and deprecate original field
You can find the image built from this PR at
Built from b3a2e29 |
Is there a way to fix this on go-waku side? If go-waku is going to be deprecated I would prefer to add "this burden" on that side rather than in nwaku. |
@alrevuelta that would be the suggestion # 1:
The resulting protobuffer would look like this, and would need to be also updated in both the RFC and in waku-proto repo as its current definition does not match the behavior nwaku has.
Doing the change in go-waku is easy, but has a drawback: It would require coordination so when infra deploys a release with version >= 0.25.0, all contributors also update their desktop version to something that uses this protobuffer. At the end of the day, it would depend on how status would feel about introducing this breaking change. |
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.
Thanks!
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! Thanks!
I wonder if we could add a new field, version
, that will help to properly switch logic according to the version received/sent. That could help to better separate the per-version-logic.
Also, having a separate logic per each version will help to prune the code in the future when no longer supporting a particular version.
In general, when working with protobuf, it's good to assume a similar mindset as with working with JSON or any other unordered key-value mechanism: you should expect "unknown" fields in the stream and do as much as you can with the fields you do understand, degrading gracefully if there's something critical missing (for example because "future" versions stopped sending it) - it is "normal" to add and remove fields in protobuf and the application should generally degrade gracefully - a version field tends to complicate this "natural" be-lenient-in-what-you-accept approach (at that point, you might as well send a separate message entirely, on a new endpoint). In particular, the problem is that if you receive a version you don't recognise, you can do nothing with this information - generally, the outcome is better if instead you try to decode the fields you know in this situation. Future versions can then exploit the graceful-degradation logic to create "overlapping-versions" support like in this PR. It's entirely fine and "natural" to add fields like this as the product evolves - one potential improvement is indeed to document the versions support in the protobuf and describe how that version handles the field, ie |
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.
Thanks!
Description
This is the solution #3 described in #2509
Add a new field for the shards and deprecate field 2, and for at least 2 releases support both fields i.e.:
We would use getPackedRepeatedField on field 3, and if it's not populated, use getPackedRepeatedField on field2, and if it's empty, use getRepeatedField. This would solve the problem of nwaku not understanding go-waku's packed field.
For writing, we'd still use write3 for field 2 and writePacked for field3.
Currently go-waku does not care about the format of the field for reading data since it seems to try decoding both packed and unpacked data. go-waku should also implement the same solution, but for the time being this would allow users of different nwaku and go-waku versions to connect with each other and not introduce breaking changes