Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[WIP] Feat/attachements #190
[WIP] Feat/attachements #190
Changes from all commits
dc51ccb
e084b33
be42f76
7b34eb2
d4bf467
4e14797
9cada9f
4928b11
cf215fd
53322c6
8a025e1
e24fed7
793db48
0f98378
77c00cb
d05c161
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
From a usability point of view, I was thinking about having something like this:
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.
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.
z_bytes_new
is just a constructor for a bytes view from a null terminated string, but you may constructz_bytes_t
in other ways, aliasing whatever memory you like.z_attachment_t
is what actually provides Zenoh with the data to send, and does so by being a v-tabled iterator over aliased byte views. If you like, you can provide that v-table for any type you want, allowing you to have 0 need for allocations in the whole process.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 @p-avital can you please explain why do you need a map?
What not just provide a way to provide a pointer to a header buffer ?
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.
The idea is to provide a flexible way for the user to provide metadata to be attached to the payload. Having one single pointer to a buffer forces the user to potentially allocate a buffer and serialize himself multiple metadata in it first. From a usability perspective, we believe a key-value semantics would be more flexible so as to cover more diverse use cases.
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.
This is because the abstraction for metadata is a map of byte view to byte view. Notably because we've seen the need for multiple metadata fields, and that a single buffer would force the user to serialise their metadata on their own before we serialise it again into a packet. This would be both wasteful and less user friendly. This is why we chose this specific abstraction.
You can choose to have a single buffer and have they key to it be an empty slice, the iterator for that wouldn't be hard to write, and would never need to allocate.
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.
In the
put
example I propose to havez_attachment_insert(..)
. In the sub, what about something like:We might still implement the internals of a
z_attachment_t
as az_bytes_map
, but that would be abstracted by thez_attachment_key
andz_attachment_value
. By doing so, we reserve a bit of freedom to change the internal implementation without changing the API.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.
The goal here is for
z_attachement_t
to be a generic, read-only type. This lets users pick whatever suits them best as a concrete implementation. The map is provided as the default way to make up an attachment, but that way of working lets us have lazy deserializers on the read side, and if the user want noalloc attachements, they can have them