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

Refactor publisher proto definition #35

Merged
merged 11 commits into from
Sep 14, 2023

Conversation

devkelley
Copy link
Contributor

Motivation and Context

This change separates out the publisher.proto file into two distinct files. This allows for publishers to implement the callback method without being forced into implementing a 'get_subscription_info' call. That call will be dependent on each publisher and may vary slightly so it has been removed from the required proto.

The 'get_subscription_info' proto definition was moved into the sample code to demonstrate how a publisher could surface the dynamically created topics to a subscriber.

Description

This PR makes the following changes:

  • Separates publisher.proto into two distinct services, 'PublisherCallback' (required to implement for a publisher) and 'SamplePublisher' (an example of how a publisher could surface dynamic topics.
  • Removes from the constants file a publisher reference and places it in the samples_settings file.
  • Adds 'samples_proto' to the samples folder, and places the 'sample_publisher.proto' file in there.
  • Samples now utilize the 'samples_proto' crate over the proto crate used by the pub sub service.
  • Samples show how to initialize a publisher with two grpc services (PublisherCallback and SamplePublisher) while sharing the topic store information.

NOTE: The 'publisher_impl.rs' files in chariott-publisher and simple-publisher are the exact same.

Additionally:

  • Modifies 'topic_store' in samples to be easier to interact with, allowing for the base 'Publisher' class to be cloned.

Copy link
Contributor

@ladatz ladatz left a comment

Choose a reason for hiding this comment

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

Please consider my comments, just a few suggestions

samples/chariott-publisher/src/publisher_impl.rs Outdated Show resolved Hide resolved
samples/simple-publisher/src/publisher_impl.rs Outdated Show resolved Hide resolved
Copy link

@ashbeitz ashbeitz left a comment

Choose a reason for hiding this comment

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

Just one comment.

samples/simple-publisher/src/main.rs Outdated Show resolved Hide resolved
Copy link

@ashbeitz ashbeitz left a comment

Choose a reason for hiding this comment

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

Approved.

@devkelley devkelley merged commit e4db66a into main Sep 14, 2023
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.

3 participants