-
Notifications
You must be signed in to change notification settings - Fork 58
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
Conversation
Add liveliness samples
…ol support are needed
@@ -23,6 +23,9 @@ int main(int argc, char **argv) { | |||
if (argc > 1) keyexpr = argv[1]; | |||
if (argc > 2) value = argv[2]; | |||
|
|||
z_owned_bytes_map_t attachements = z_bytes_map_new(); |
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:
// Options.attachment is z_attachment_t and allocated on the first insert
// I would avoid copying the values on the insert since they are going to be copied anyway for serialization
z_attachment_insert(z_loan(&options.attachment), z_attachment_key("hello"), z_attachment_value(value, strlen(value)));
void data_handler(const z_sample_t *sample, void *arg) { | ||
z_owned_str_t keystr = z_keyexpr_to_string(sample->keyexpr); | ||
printf(">> [Subscriber] Received %s ('%s': '%.*s')\n", kind_to_str(sample->kind), z_loan(keystr), | ||
(int)sample->payload.len, sample->payload.start); | ||
if (z_check(sample->attachements)) { |
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 have z_attachment_insert(..)
. In the sub, what about something like:
z_attachment_value v = z_attachment_get(sample->attachment, z_attachment_key("hello"));
if (z_check(v)) {
// Do your stuff
}
We might still implement the internals of a z_attachment_t
as a z_bytes_map
, but that would be abstracted by the z_attachment_key
and z_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
@@ -23,6 +23,9 @@ int main(int argc, char **argv) { | |||
if (argc > 1) keyexpr = argv[1]; | |||
if (argc > 2) value = argv[2]; | |||
|
|||
z_owned_bytes_map_t attachements = z_bytes_map_new(); | |||
z_bytes_map_insert_by_copy(&attachements, z_bytes_new("hello"), z_bytes_new("there")); |
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.
- Is providing raw bytes supported or should it be only strings?
- is it possible to provide the buffer and not allocate buffers ? i would like to reduce the number of allocations during run time
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.
- Yes,
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. - It is possible:
- The map itself is able to alias buffers rather than copy them, but it's still a map that will allocate as part of its functioning.
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.
Based on eclipse-zenoh#190 with minor changes: - attachement(s) -> attachment renaming - conflicts resolving - formatting
Based on eclipse-zenoh#190 with minor changes: - attachement(s) -> attachment renaming - conflicts resolving - formatting
Based on eclipse-zenoh#190 with minor changes: - attachement(s) -> attachment renaming - conflicts resolving - formatting
Based on eclipse-zenoh#190 with minor changes: - attachement(s) -> attachment renaming - conflicts resolving - formatting
Replaced with #203 |
Based on eclipse-zenoh#190 with minor changes: - attachement(s) -> attachment renaming - conflicts resolving - formatting
TODO:
z_attachment_get(this, key)
which shall do linear search in the attachment