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

feat: add ids to channel and schema types #1245

Closed
wants to merge 3 commits into from

Conversation

alxhill
Copy link

@alxhill alxhill commented Sep 30, 2024

Changelog

Add id fields to the Schema and Channel structs.

Docs

None

Description

The Channel ID is needed to be able to distinguish two channels with the same topic name, which are not guaranteed to be unique. I've also added the ID to the Schema type for consistency but this is not strictly necessary for us/our use case.

One challenge here is that the Channel and Schema types are used for both read and write workflows, but for write workflows it would be ignored (e.g add_channel generates a new channel id). In this PR I ignored the additional field, but would be happy to refactor another way. Some options I came up with:

  1. make the field Option<u16> and set it on read. Simple enough, but makes the contract of when it will/won't be set (or how it'll behave if the field is set on write) unclear.
  2. create a new MessageStream that returns message header with the message (or some other new combined type)
  3. Split the Channel struct into readable/writeable versions. Seems like a fairly invasive change but may be helpful in the long-run?

@alxhill alxhill marked this pull request as draft September 30, 2024 18:38
@james-rms
Copy link
Collaborator

I like the idea, but I don't think we should accept these structs in APIs that return channel IDs for the writer. I can see if i can integrate this change into a larger one that updates the writer API.

@alxhill
Copy link
Author

alxhill commented Dec 18, 2024

agree, this doesn't feel quite right. let me know if I can assist with any of the broader API changes, we're keen to use it more moving forwards

@james-rms
Copy link
Collaborator

@alxhill #1297 is my response to this issue, please chime in and review.

@james-rms james-rms closed this Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants