eclipse-uprotocol / up-streamer-rust

Generic uStreamer implementation written in Rust
Apache License 2.0
1 stars 13 forks source link

Initial implementation of `up-streamer-rust` #8

Closed PLeVasseur closed 5 months ago

PLeVasseur commented 7 months ago

Intro

Initial implementation of up-streamer-rust to connect point-to-point messages across any UTransport implementation, i.e.

The implementation of handling Publish messages will come in a second PR.

Background

Hey there :wave:

Based on @stevenhartley's stellar design, I reworked the in-progress first PR I had.

Looking for any feedback you may have :slightly_smiling_face:

Addresses #3

A rework of #7 in which UTransport is thread-safe

Also rework of #9, since in this one we used dedicated tasks for output onto a transport when forwarding for efficiency and consistent sequencing.

TODO

Performance

Seems to improve performance over #9 and in line with #7

Stats

#7
Non-Thread-Safe: With Channels
# of messages received in 1 second
#8
Thread-Safe: Straightforward Implementation
# of messages received in 1 second
#9 (this PR)
Thread-Safe: Utilizing Async Task for Sender
# of messages received in 1 second
Avg 5067 3560.6 4966.5
StdDev 79.14543575 49.19846429 80.11554156
PLeVasseur commented 7 months ago

Hi @sophokles73, @AnotherDaniel, @stevenhartley, @Mallets, @evshary --

Bringing the initial version out of draft. Ready for some feedback :slightly_smiling_face:

PLeVasseur commented 7 months ago

I haven't taken a deeper look, but your implementation doesn't depend on any up-client library now, right?

That's right. I have abstracted away completely from any specific up-client-foo-rust.

PLeVasseur commented 6 months ago

I find the code very hard to read and understand due to the lack of comments. In particular, it is still unclear to me why you need that many HashMaps for keeping all sorts of references and counters.

Started to simplify this a bit.

My gut feeling is that you could replace at least the TransportForwarder{Count} maps with just Arcs and tha fact that the forwarders will cease to exist once all Senders have gone out of scope ...

Yeah, I agree that this would work, did this part so far.

PLeVasseur commented 6 months ago

I find the code very hard to read and understand due to the lack of comments. In particular, it is still unclear to me why you need that many HashMaps for keeping all sorts of references and counters.

So, I think some of this complexity is inevitable. Given that, I tried to more clearly divide the responsibility for managing the TransportForwarders and ForwardingListeners.

Greatly looking forward to your feedback :slightly_smiling_face:

sophokles73 commented 5 months ago

@PLeVasseur do you want to squash the commits before I merge this?

PLeVasseur commented 5 months ago

@PLeVasseur do you want to squash the commits before I merge this?

Yes, I will squash the commits. Will ping you when done

PLeVasseur commented 5 months ago

Okay @sophokles73 -- I squashed the commits