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.

Sign up for GitHub

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

Jump to bottom

Initial implementation of up-streamer-rust #8

Merged
merged 1 commit into from
May 31, 2024
Merged

Initial implementation of up-streamer-rust #8

merged 1 commit into from
May 31, 2024

Conversation

PLeVasseur
Copy link
Contributor

@PLeVasseur PLeVasseur commented Apr 3, 2024 •
edited
Loading

Intro

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

  • Notification
  • Request
  • Response

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

Background

Hey there 👋

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

Looking for any feedback you may have 🙂

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

  • Rebase onto Make Utransport-related traits async-usable up-rust#79 to ensure still functional
  • Get integration tests setup to ensure in-order delivery of all sent messages
  • Troubleshoot why messages are being lost when benchmarking throughput
  • Refactored integration-test-utils to allow us to orchestrate adding and deleting rules on the fly
  • dynamic adding and removing routes integration tests: local, remote_a -> local, remote_a, remote_b -> local, remote_b (may have to refactor integration test utils)
  • Consider if / how to refactor book-keeping of routes
  • update sequence diagram to reflect current state
  • Update to latest up-rust

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 PLeVasseur force-pushed the feature/initial-pluggable-ustreamer_utransport-thread-safe branch from 08fab3d to 3c7a8a7 Compare April 4, 2024 18:32
@PLeVasseur PLeVasseur changed the title Initial implementation of up-streamer-rust - UTransport thread-safe version Initial implementation of up-streamer-rust - UTransport thread-safe version - with worker task pools Apr 5, 2024
@PLeVasseur PLeVasseur mentioned this pull request Apr 5, 2024
Closed
@PLeVasseur PLeVasseur force-pushed the feature/initial-pluggable-ustreamer_utransport-thread-safe branch from 87f9926 to 829d78a Compare April 5, 2024 19:23
@PLeVasseur PLeVasseur mentioned this pull request Apr 5, 2024
Closed
@PLeVasseur PLeVasseur changed the title Initial implementation of up-streamer-rust - UTransport thread-safe version - with worker task pools Initial implementation of up-streamer-rust Apr 5, 2024
@PLeVasseur PLeVasseur changed the title Initial implementation of up-streamer-rust Initial implementation of up-streamer-rust Apr 5, 2024
@PLeVasseur PLeVasseur mentioned this pull request Apr 5, 2024
Merged
@PLeVasseur PLeVasseur force-pushed the feature/initial-pluggable-ustreamer_utransport-thread-safe branch from 829d78a to 5a8e33e Compare April 5, 2024 19:48
PLeVasseur
PLeVasseur commented Apr 5, 2024
View reviewed changes
PLeVasseur
PLeVasseur commented Apr 5, 2024
View reviewed changes
@PLeVasseur PLeVasseur force-pushed the feature/initial-pluggable-ustreamer_utransport-thread-safe branch 11 times, most recently from b611978 to 5ed0a2c Compare April 8, 2024 20:00
@PLeVasseur PLeVasseur marked this pull request as ready for review April 8, 2024 20:01
@PLeVasseur PLeVasseur force-pushed the feature/initial-pluggable-ustreamer_utransport-thread-safe branch from 5ed0a2c to 2691e31 Compare April 8, 2024 20:02
@PLeVasseur PLeVasseur marked this pull request as draft April 8, 2024 20:04
@PLeVasseur PLeVasseur force-pushed the feature/initial-pluggable-ustreamer_utransport-thread-safe branch 2 times, most recently from 0e850f9 to 3d15678 Compare April 8, 2024 20:18
@PLeVasseur PLeVasseur marked this pull request as ready for review April 8, 2024 20:19
@PLeVasseur
Copy link
Contributor Author

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

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

evshary
evshary reviewed Apr 27, 2024
View reviewed changes
up-streamer/tests/single_local_single_remote.rs
.await;
assert!(add_forwarding_rule_res.is_ok());

// adding remote to local routing
Copy link

Choose a reason for hiding this comment

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

I'm thinking whether having an API to add both routing is a better idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps?

@stevenhartley -- do you see a use-case in which we're only wanting uni-directional routing?

Copy link

Choose a reason for hiding this comment

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

No not that i can think of right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So then replacing the currently uni-directional API for add_forwarding_rule() with a bi-directional approach is the right thing to do? Seems that way if no desire for uni-directional hookup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @stevenhartley, bumping this now that I have a bit of bandwidth to perform some revisions. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I heard back from @stevenhartley that we should be able to use a single add_forwarding_rule() to connect both directions. Tracking in #14.

sophokles73
sophokles73 reviewed Apr 29, 2024
View reviewed changes
@PLeVasseur PLeVasseur force-pushed the feature/initial-pluggable-ustreamer_utransport-thread-safe branch 2 times, most recently from 15bb6fb to ec17b99 Compare April 29, 2024 21:10
@PLeVasseur
Copy link
Contributor Author

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.

sophokles73
sophokles73 reviewed Apr 30, 2024
View reviewed changes
@PLeVasseur
Copy link
Contributor Author

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 🙂

evshary
evshary reviewed May 20, 2024
View reviewed changes
@PLeVasseur PLeVasseur force-pushed the feature/initial-pluggable-ustreamer_utransport-thread-safe branch 4 times, most recently from e7843ec to 8400294 Compare May 30, 2024 12:44
evshary
evshary approved these changes May 31, 2024
View reviewed changes
Copy link

@evshary evshary left a comment

Choose a reason for hiding this comment

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

LGTM

sophokles73
sophokles73 approved these changes May 31, 2024
View reviewed changes
Copy link

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

LGTM

@sophokles73
Copy link

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

@PLeVasseur
Copy link
Contributor Author

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

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

@PLeVasseur
[#8] Initial implementation of up-streamer-rust
ddce8f5 
    * Supports up-spec 1.5.8
    * Implemented as a generic library which can be integrated with any UPClients that implement UTransport

Implements #3
@PLeVasseur PLeVasseur force-pushed the feature/initial-pluggable-ustreamer_utransport-thread-safe branch from 8400294 to ddce8f5 Compare May 31, 2024 11:07
@PLeVasseur
Copy link
Contributor Author

Okay @sophokles73 -- I squashed the commits

@sophokles73 sophokles73 merged commit d4ab17f into eclipse-uprotocol:main May 31, 2024
5 checks passed
This was referenced Jul 17, 2024
Closed
Closed
Closed
@PLeVasseur PLeVasseur deleted the feature/initial-pluggable-ustreamer_utransport-thread-safe branch August 13, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mallets Mallets Mallets left review comments

@stevenhartley stevenhartley stevenhartley left review comments

@chasehof chasehof chasehof left review comments

@evshary evshary evshary approved these changes

@sophokles73 sophokles73 sophokles73 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@PLeVasseur @sophokles73 @evshary @Mallets @stevenhartley @chasehof