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

Add publish to up-streamer rust #20

Merged
merged 1 commit into from
Aug 12, 2024
Merged

Add publish to up-streamer rust #20

merged 1 commit into from
Aug 12, 2024

Conversation

matthewd0123
Copy link
Contributor

@matthewd0123 matthewd0123 commented Jun 27, 2024 •
edited by PLeVasseur
Loading

  • Add support for tracking subscriptions and forwarding published messages in ustreamer.rs
  • Add usubscription-static-file to stub out subscriptions before usubscription is written
  • Add tests for mE (SOME/IP) subscribe/publish and uE (zenoh) subscribe/publish

@matthewd0123
Copy link
Contributor Author

#18

@matthewd0123 matthewd0123 force-pushed the feature/streamer_publish branch 2 times, most recently from 7e6d1a3 to 785ff9e Compare June 27, 2024 18:05
AnotherDaniel
AnotherDaniel approved these changes Jul 8, 2024
View reviewed changes
Copy link

@AnotherDaniel AnotherDaniel left a comment

Choose a reason for hiding this comment

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

LGTM - some minor questions, and we'll have some settling-in to do once I get usub published, but I don't see any surprises.

example-utils/usubscription-static-file/Cargo.toml Outdated Show resolved Hide resolved
example-utils/usubscription-static-file/Cargo.toml Outdated Show resolved Hide resolved
subscription-cache/src/lib.rs Show resolved Hide resolved
subscription-cache/src/lib.rs Outdated Show resolved Hide resolved
PLeVasseur
PLeVasseur requested changes Aug 6, 2024
View reviewed changes
Copy link
Contributor

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

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

Hey @matthewd0123 -- thanks for the PR!

Take a look at some suggestions 🙂

example-utils/usubscription-static-file/src/lib.rs Outdated Show resolved Hide resolved
up-streamer/src/ustreamer.rs Outdated Show resolved Hide resolved
example-utils/usubscription-static-file/src/lib.rs Outdated Show resolved Hide resolved
subscription-cache/src/lib.rs Outdated Show resolved Hide resolved
up-streamer/src/ustreamer.rs Outdated Show resolved Hide resolved
up-streamer/src/ustreamer.rs Outdated Show resolved Hide resolved
up-streamer/src/ustreamer.rs Outdated Show resolved Hide resolved
example-utils/usubscription-static-file/src/lib.rs Outdated Show resolved Hide resolved
@matthewd0123 matthewd0123 force-pushed the feature/streamer_publish branch 25 times, most recently from a9adfc9 to 8001b84 Compare August 7, 2024 20:06
PLeVasseur
PLeVasseur requested changes Aug 7, 2024
View reviewed changes
Copy link
Contributor

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

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

Hey @matthewd0123 -- take a look at some further thoughts.

up-linux-streamer/examples/uE_client.rs Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
subscription-cache/src/lib.rs Outdated Show resolved Hide resolved
subscription-cache/src/lib.rs Outdated Show resolved Hide resolved
up-linux-streamer/examples/mE_subscriber.rs Outdated Show resolved Hide resolved
up-linux-streamer/examples/uE_subscriber.rs Outdated Show resolved Hide resolved
up-linux-streamer/examples/mE_publisher.rs Outdated Show resolved Hide resolved
up-linux-streamer/examples/uE_subscriber.rs Outdated Show resolved Hide resolved
up-linux-streamer/examples/uE_publisher.rs Outdated Show resolved Hide resolved
@matthewd0123 matthewd0123 force-pushed the feature/streamer_publish branch 6 times, most recently from 0d4f0f2 to 7aefa30 Compare August 8, 2024 20:18
PLeVasseur
PLeVasseur requested changes Aug 9, 2024
View reviewed changes
Copy link
Contributor

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

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

Hey @matthewd0123 -- I like your idea of UUris to backpedal. that's a good way to go about it.

I left some requests for change, please take a look.

subscription-cache/src/lib.rs Show resolved Hide resolved
subscription-cache/src/lib.rs Show resolved Hide resolved
subscription-cache/src/lib.rs Show resolved Hide resolved
subscription-cache/src/lib.rs Show resolved Hide resolved
subscription-cache/src/lib.rs Outdated Show resolved Hide resolved
up-streamer/src/ustreamer.rs Outdated Show resolved Hide resolved
up-streamer/src/ustreamer.rs Outdated Show resolved Hide resolved
up-streamer/src/ustreamer.rs Outdated Show resolved Hide resolved
up-streamer/src/ustreamer.rs Outdated Show resolved Hide resolved
up-streamer/src/ustreamer.rs Outdated Show resolved Hide resolved
@matthewd0123 matthewd0123 force-pushed the feature/streamer_publish branch 3 times, most recently from d6de3ef to b236907 Compare August 9, 2024 15:46
@PLeVasseur PLeVasseur linked an issue Aug 9, 2024 that may be closed by this pull request
Support publish in up-streamer-rust #18
Closed
PLeVasseur
PLeVasseur requested changes Aug 9, 2024
View reviewed changes
Copy link
Contributor

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

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

We're nearing the home stretch!

subscription-cache/src/lib.rs Show resolved Hide resolved
example-utils/usubscription-static-file/src/lib.rs Outdated Show resolved Hide resolved
subscription-cache/src/lib.rs Show resolved Hide resolved
up-linux-streamer/DEFAULT_CONFIG.json5 Outdated Show resolved Hide resolved
up-linux-streamer/src/main.rs Outdated Show resolved Hide resolved
up-linux-streamer/ZENOH_CONFIG.json5 Show resolved Hide resolved
up-streamer/src/ustreamer.rs Outdated Show resolved Hide resolved
up-streamer/src/ustreamer.rs Outdated Show resolved Hide resolved
up-streamer/src/ustreamer.rs Outdated Show resolved Hide resolved
up-streamer/src/ustreamer.rs Outdated Show resolved Hide resolved
PLeVasseur
PLeVasseur requested changes Aug 9, 2024
View reviewed changes
Copy link
Contributor

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

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

Some more ideas

subscription-cache/src/lib.rs Show resolved Hide resolved
up-linux-streamer/README.md Outdated Show resolved Hide resolved
up-streamer/tests/usubscription_bad_data.rs Outdated Show resolved Hide resolved
utils/integration-test-utils/src/up_client_failing_register.rs Outdated Show resolved Hide resolved
utils/integration-test-utils/src/up_client_failing_register.rs Outdated Show resolved Hide resolved
utils/integration-test-utils/src/up_client_failing_register.rs Show resolved Hide resolved
subscription-cache/src/lib.rs Show resolved Hide resolved
@matthewd0123
Copy link
Contributor Author

To the question about what "full" means... the "status", "config", and "attributes" aren't currently being used by us in the streamer, so they're just added to the cache for tracking. Maybe in the future they'll be needed for something, but for now, I'm leaving them as optional

@matthewd0123 matthewd0123 force-pushed the feature/streamer_publish branch 3 times, most recently from 858926d to 6c2396a Compare August 12, 2024 15:35
PLeVasseur
PLeVasseur approved these changes Aug 12, 2024
View reviewed changes
Copy link
Contributor

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

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

LGTM.

Please now squash down to clean commit history. If single commit makes sense, that's fine. If you think several commits more cleanly lays out what was done, that's fine too.

matthewd0123 added a commit to matthewd0123/up-streamer-rust that referenced this pull request Aug 12, 2024
@matthewd0123
[eclipse-uprotocol#20] Add publish to up-streamer rust
ab78bfc 
* Add support for tracking subscriptions and forwarding published messages in ustreamer.rs
* Add usubscription-static-file to stub out subscriptions before usubscription is written
* Add tests for mE (SOME/IP) subscribe/publish and uE (zenoh) subscribe/publish
* Add information about pub/sub in READMEs
* Support zenoh configuration in command line file
* Add test for usubscription with bad data

Implements [eclipse-uprotocol#18]
@matthewd0123
[eclipse-uprotocol#20] Add publish to up-streamer rust
7c73d07 
* Add support for tracking subscriptions and forwarding published messages in ustreamer.rs
* Add usubscription-static-file to stub out subscriptions before usubscription is written
* Add examples for mE (SOME/IP) subscribe/publish and uE (zenoh) subscribe/publish
* Add information about pub/sub in READMEs
* Support zenoh configuration in command line file
* Add test for usubscription with bad data

Implements [eclipse-uprotocol#18]
@PLeVasseur PLeVasseur mentioned this pull request Aug 12, 2024
Merged
@PLeVasseur PLeVasseur merged commit 15ffc8b into eclipse-uprotocol:main Aug 12, 2024
8 checks passed
@PLeVasseur PLeVasseur mentioned this pull request Aug 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AnotherDaniel AnotherDaniel AnotherDaniel approved these changes

@PLeVasseur PLeVasseur PLeVasseur 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.

Support publish in up-streamer-rust
3 participants
@matthewd0123 @PLeVasseur @AnotherDaniel