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: support for custom (de)serializer #156

Merged
merged 9 commits into from
May 2, 2023

Conversation

J-Loudet
Copy link
Contributor

This commit introduces the following changes to Zenoh-Flow:

  1. types used within a Zenoh-Flow application do not have to implement the
    (cumbersome) ZFData trait; instead,
  2. the types used must be Send + Sync + 'static,
  3. an Output must know how to serialize the provided type T and, respectively,
    an Input must know how to deserialize bytes into T.

With these changes, any SerDe compatible data serialization format is supported
as well as, for instance, ProtoBuf.

In terms of API, the major difference is how the Input and Output are obtained:

input: inputs
    .take("in", |bytes| todo!("Provide your deserializer here"))
    .expect("No input called 'in' found"),

output: outputs
    .take("out", |data| todo!("Provide your serializer here"))
    .expect("No output called 'out' found"),

Copy link
Contributor

@gabrik gabrik left a comment

Choose a reason for hiding this comment

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

First pass, overall LGTM, a few comments, and the Debug is important for us to be used in logging, otherwise, we may never be able to understand what it is going on.

I need to give a more deep pass on it,

zenoh-flow/src/io/input.rs Outdated Show resolved Hide resolved
zenoh-flow/src/io/input.rs Show resolved Hide resolved
zenoh-flow/src/io/input.rs Outdated Show resolved Hide resolved
zenoh-flow/src/io/output.rs Outdated Show resolved Hide resolved
zenoh-flow/src/io/output.rs Show resolved Hide resolved
zenoh-flow/src/traits.rs Show resolved Hide resolved
zenoh-flow/src/io/input.rs Outdated Show resolved Hide resolved
zenoh-flow/src/types/message.rs Outdated Show resolved Hide resolved
zenoh-flow/src/types/message.rs Outdated Show resolved Hide resolved
zenoh-flow/src/types/message.rs Show resolved Hide resolved
@gabrik
Copy link
Contributor

gabrik commented Apr 12, 2023

I'm thinking if Input and Output shouldn't become traits.
Let me explain we could have a builder pattern for getting inputs and outputs (in the future we could add things like dropping policies, etc.), and based on passing or not the se/deserializer you get something that implements the trait with functions like recv<T> where T is typed if you passed the se/deserializer or Arc<Vec<u8>> if not.

Not really sure how this would translate to code.

@J-Loudet
Copy link
Contributor Author

I'm thinking if Input and Output shouldn't become traits. Let me explain we could have a builder pattern for getting inputs and outputs (in the future we could add things like dropping policies, etc.), and based on passing or not the se/deserializer you get something that implements the trait with functions like recv<T> where T is typed if you passed the se/deserializer or Arc<Vec<u8>> if not.

Not really sure how this would translate to code.

Maybe a trait is not needed, the builder pattern could be enough: take("port_id") would create an InputRaw builder, a following call with_deserializer(…) would turn in into a typed Input. Same logic for the Output.

That's what you had in mind?

@gabrik
Copy link
Contributor

gabrik commented Apr 12, 2023

Maybe a trait is not needed, the builder pattern could be enough: take("port_id") would create an InputRaw builder, a
following call with_deserializer(…) would turn in into a typed Input. Same logic for the Output.
That's what you had in mind?

Yep, just because of the take and take_raw that could be simplified, using a single take and the builder pattern, I think what you propose should work fine.

Copy link
Contributor

@gabrik gabrik left a comment

Choose a reason for hiding this comment

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

LGTM, modulo the discussion on the builder for the inputs/outputs.

@gabrik
Copy link
Contributor

gabrik commented Apr 26, 2023

This depends on: #158

This commit introduces the following changes to Zenoh-Flow:
1. types used within a Zenoh-Flow application do not have to implement the
   (cumbersome) `ZFData` trait; instead,
2. the types used must be `Send + Sync + 'static`,
3. an Output must know how to serialize the provided type `T` and, respectively,
   an Input must know how to deserialize bytes into `T`.

With these changes, any SerDe compatible data serialization format is supported
as well as, for instance, ProtoBuf.

In terms of API, the major difference is how the Input and Output are obtained:

```rust
input: inputs
    .take("in", |bytes| todo!("Provide your deserializer here"))
    .expect("No input called 'in' found"),

output: outputs
    .take("out", |data| todo!("Provide your serializer here"))
    .expect("No output called 'out' found"),
```
Instead of having the methods `take` and `take_raw` on the Inputs and Outputs,
that give, respectively, the Typed and Raw Input/Output, this commit removes
the `take_raw` variant and introduces builders.

The builders can be turned into the Typed or Raw variant using the corresponding
methods: `build_typed` and `build_raw`.
@J-Loudet J-Loudet force-pushed the feat/support-custom-de-serializer branch from b86debf to 253a8f0 Compare April 28, 2023 09:29
* feat: reuse buffer when serializing

This commit tries to minimize the number of allocations performed when
serializing data. For this end, the connector and built-in Source now create
internal `Vec<u8>` buffers that are reused whenever a message is serialized.

This change should hopefully improve performance.

* doc: fix typos
Signed-off-by: Julien Loudet <julien.loudet@zettascale.tech>
@J-Loudet J-Loudet merged commit 28d8984 into master May 2, 2023
@J-Loudet J-Loudet deleted the feat/support-custom-de-serializer branch May 2, 2023 15:21
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.

2 participants