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

replace all NatsRequest with array of points #406

Closed
cbrake opened this issue Aug 18, 2022 · 3 comments
Closed

replace all NatsRequest with array of points #406

cbrake opened this issue Aug 18, 2022 · 3 comments

Comments

@cbrake
Copy link
Member

cbrake commented Aug 18, 2022

in internal/pb/nats-request.proto

This change would make the wire protocol more flexible if we could simply add new points in an array when we want to modify the options in the Node query API over NATS. Long term I can see more of a query language like SQL or GraphQL. Putting this in points may seem clunky, but would allow us to easily support new query types in the future while retaining compatibility with old. Putting everything into points kind of defeats the purpose of types in protobuf. However, I think long term wire protocol compatibility is probably a bigger concern as its a difficult problem to propagate protobuf changes throughout the system and could block systems from communicating.

@bminer thoughts?

@cbrake cbrake added this to the v0.5.0 (sqlite) milestone Aug 18, 2022
@bminer
Copy link
Contributor

bminer commented Aug 23, 2022

If I'm understanding this correctly, you are considering removing NatsRequest entirely and just sending the type and includeDel options over the wire as a list of points (using the point type to distinguish between options).

TLDR; If I'm being totally honest, I don't have a preference either way.

This ultimately comes down to the question: "How should data be decoded from the wire in a forward / backward compatible way?" Protobuf's answer to this is to add unique numbers to each field, so that as fields are added / removed, compatibility between versions can be maximized. Personally, while I think that this approach has merit, I don't really like it much. ;-) Too much thinking required. It's much more natural to think about mapping / decoding data based on its "field name". This is what I chose to do in schemer. But, there is the obvious downside of ensuring that an unique field name remains compatible with all of its encoded data. Same is true for protobuf with a given field number. For example, once you set includeDel to be interpreted as a boolean value, you cannot easily change it to a struct later (because prior versions would encode it as a boolean, and how to you systematically decode that into a struct?) In schemer, I allow the end user to "weakly" decode booleans to numeric types (i.e. false auto-maps to 0, true maps to 1), but booleans to structs makes no sense.

Since we are already using protobuf, this is all probably moot. Hopefully, that makes sense, haha!

@cbrake
Copy link
Member Author

cbrake commented Aug 23, 2022

schemer looks interesting -- I've looked at it several times, but don't completely understand it yet -- do you have a code example that shows how it works? It would be neat to see how you manage changes over time.

My current approach in SIOT is that encoding is a necessary evil and do as little of it as possible, so in the wire formats, minimize the number of types and type changes. Perhaps this is a cop-out and I'm just back to essentially JSON. However, it a distributed IOT system where different pieces will be updated at different times and eventually clients will be written in different languages, you can't have the encoding formats on the wire changing a lot. As I'm working through the transition to sqlite, I think there are also parallels to SQL databases -- yes, deeply nested data structures are handy and quick, but they don't always scale and are not as flexible as embracing the constraint of flat tables/structs and finding ways to work within that constraint.

I'm currently maintaining an IOT system where every change in the config requires a new field in a struct. The struct keeps growing larger and larger. Changes, while possible with protobuf are clunky as I have to update every piece of code in the communication path so the new field is not lost. This experience has convinced me there has to be a better way.

@cbrake
Copy link
Member Author

cbrake commented Sep 20, 2022

this has been implemented as part of #404. @bminer can you review/test the JS changes? I still have not got set up to build/test that code -- maybe we could work through that sometime.

@cbrake cbrake closed this as completed Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants