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

[WIP] Feat/attachements #190

Closed
wants to merge 16 commits into from
Closed

[WIP] Feat/attachements #190

wants to merge 16 commits into from

Conversation

p-avital
Copy link
Contributor

@p-avital p-avital commented Nov 14, 2023

TODO:

  • Fix attachment typoed as attachement
  • Provide z_attachment_get(this, key) which shall do linear search in the attachment

@p-avital p-avital changed the title Feat/attachements [WIP] Feat/attachements Nov 14, 2023
Cargo.toml Show resolved Hide resolved
@@ -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();
Copy link
Member

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)) {
Copy link
Member

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.

Copy link
Contributor Author

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"));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Is providing raw bytes supported or should it be only strings?
  2. is it possible to provide the buffer and not allocate buffers ? i would like to reduce the number of allocations during run time

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yes, z_bytes_new is just a constructor for a bytes view from a null terminated string, but you may construct z_bytes_t in other ways, aliasing whatever memory you like.
  2. 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.

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 ?

Copy link
Member

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.

Copy link
Contributor Author

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.

sashacmc added a commit to ZettaScaleLabs/zenoh-c that referenced this pull request Nov 27, 2023
Based on eclipse-zenoh#190 with minor
changes:
 - attachement(s) -> attachment renaming
 - conflicts resolving
 - formatting
sashacmc added a commit to ZettaScaleLabs/zenoh-c that referenced this pull request Nov 29, 2023
Based on eclipse-zenoh#190 with minor
changes:
 - attachement(s) -> attachment renaming
 - conflicts resolving
 - formatting
sashacmc added a commit to ZettaScaleLabs/zenoh-c that referenced this pull request Dec 4, 2023
Based on eclipse-zenoh#190 with minor
changes:
 - attachement(s) -> attachment renaming
 - conflicts resolving
 - formatting
sashacmc added a commit to ZettaScaleLabs/zenoh-c that referenced this pull request Dec 4, 2023
Based on eclipse-zenoh#190 with minor
changes:
 - attachement(s) -> attachment renaming
 - conflicts resolving
 - formatting
@milyin
Copy link
Contributor

milyin commented Dec 11, 2023

Replaced with #203

@milyin milyin closed this Dec 11, 2023
sashacmc added a commit to ZettaScaleLabs/zenoh-c that referenced this pull request Dec 11, 2023
Based on eclipse-zenoh#190 with minor
changes:
 - attachement(s) -> attachment renaming
 - conflicts resolving
 - formatting
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

Successfully merging this pull request may close these issues.

5 participants