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

Created Procedural Macros For Pub Sub #87

Merged
merged 27 commits into from
Mar 20, 2024
Merged

Conversation

zedgell
Copy link
Contributor

@zedgell zedgell commented Jan 1, 2023

Added a Procedural Macro for pubsub client and updated publisher and subscriber example to demo the macro.

NickLarsenNZ
NickLarsenNZ previously approved these changes Jan 1, 2023
Copy link
Contributor

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

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

Really nice!

src/lib.rs Outdated Show resolved Hide resolved
@NickLarsenNZ
Copy link
Contributor

NickLarsenNZ commented Mar 13, 2023

@zedgell you'll need to amend the commits with the DCO signoff.
I also recommend rebasing to squash/fixup or rename commits (looking at those "add feature" commits -- see my auto-comment below which I'd written for my internal repos so apologies if the wording is a bit off).


To reduce the noise in the target branch, it is a good idea to do an interactive rebase locally to tidy up the branch.

To do so,

git fetch
git rebase -i origin/main

You should see something similar to:

pick b3896031 enable the flux capacitor
pick 979393bd fix typo from previous commit

In your editor, change the first word on each commit from pick to whatever operation you desire.

An example is fixing up a typo from the parent commit:

pick b3896031 enable the flux capacitor
f 979393bd fix typo from previous commit

For the example above, you can also amend the staged typo fix into the last commit instead of creating a new commit with git commit --amend --no-message.

Save, then git push --force-with-lease.

You can also move commits around, but if their changes are near each other, you might just have to step through some conflict resolution (which can seem intimidating at first, but if you keep an eye on which commit the rebase is up to, it should be easier resolve).

@zedgell
Copy link
Contributor Author

zedgell commented Mar 18, 2023

@NickLarsenNZ Should be good now. Let me know if any other changes are needed

Copy link
Contributor

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

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

Had more of a look over the code, and noted a couple of things.

Oops, I hit Approve instead of Comment

struct #struct_name_ident;

#[tonic::async_trait]
impl AppCallback for #struct_name_ident {
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to use fully qualified names to avoid name collision.

let topic = #topic.to_string();
let pubsub_name = #pub_sub_name.to_string();

let list_subscriptions = ListTopicSubscriptionsResponse::topic(pubsub_name, topic);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this would only work for a single topic.

The ListTopicSubscriptionsResponse::topic method is a bit misleading. See #81 (comment) to see how to add multiple topics.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, my bad, I see each use of the macro creates an AppCallback implementation.

As far as I could see, this doesn't work with tonic (see my other comment here: #81 (comment)).

So I think we need to have a single implementation generated, but I don't know how that can work in this case.

Could something like this work?

struct MyService {}

#[dapr] // with this creating the single AppCallback implementation
impl MyService {

  // The pubsub_name and topic would be stored, and the method would then be used in the   `on_topic_event` trait implementation (eg: using match).
  // perhaps the function signature should have a parameter for pubsub name and topic (eg a Context struct)
  #[subscribe(pubsub_name = "pubsub", topic = "A")]
  async fn handle_message(&self, order: Order) {
    println!("{:#?}", order)
  }

}

I changed a couple of things in the macro usage (based on the python-sdk):

  • topic -> subscribe
  • pub_sub_name - > pubsub_name

Keen to hear your thoughts?

@zedgell
Copy link
Contributor Author

zedgell commented Apr 30, 2023

@NickLarsenNZ I will take a look over them.

@NickLarsenNZ
Copy link
Contributor

Hi @zedgell, did you manage to get any further?

@zedgell
Copy link
Contributor Author

zedgell commented Aug 19, 2023

@NickLarsenNZ I added a potential fix for the multiple events. I wanted them to be able to define multiple handlers for each event but not have to create a new struct for each one. I looked at the way you suggested but that poses a challenge of trying to find that method and hoping they dont have one with a similar name. What I did was Created a central AppCallbackService that users can import. It contains a vec of handlers that stores the struct that contains the handler function. I have updated the example and also below is some example code.

#[derive(Serialize, Deserialize, Debug)]
struct Order {
    pub order_number: i32,
    pub order_details: String,
}

#[topic(pub_sub_name = "pubsub", topic = "A")]
async fn handle_a_event(order: Order) {
    println!("Topic A - {:#?}", order)
}

#[topic(pub_sub_name = "pubsub", topic = "B")]
async fn handle_b_event(order: Order) {
    println!("Topic B - {:#?}", order)
}

#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
    let addr = "127.0.0.1:50051".parse().unwrap();

    let mut callback_service = AppCallbackService::new();

    callback_service.add_handler(HandleAEvent::default().get_handler());

    callback_service.add_handler(HandleBEvent::default().get_handler());

    println!("AppCallback server listening on: {}", addr);

    // Create a gRPC server with the callback_service.
    Server::builder()
        .add_service(AppCallbackServer::new(callback_service))
        .serve(addr)
        .await?;

    Ok(())
}

@zedgell
Copy link
Contributor Author

zedgell commented Oct 23, 2023

@NickLarsenNZ did you get a chance to review the above?

@mikeee mikeee added this to the 1.14 milestone Mar 17, 2024
@mikeee
Copy link
Member

mikeee commented Mar 17, 2024

@zedgell Hi Zachary, this looks amazing and will reduce the amount of boilerplate code that needs to be written! Would you be able to move this to the macros folder (and fix conflicts)? This PR has been usurped by the actors PR #99

@zedgell zedgell requested review from a team as code owners March 17, 2024 20:14
NickLarsenNZ and others added 11 commits March 17, 2024 16:47
Signed-off-by: NickLarsenNZ <nick@aptiv.co.nz>

Signed-off-by: NickLarsenNZ <nick@aptiv.co.nz>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
Signed-off-by: zachary <zacharyedgell@gmail.com>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
* Update dependencies

Signed-off-by: Hauke Jung <hauke.jung@outlook.de>

* Add grpc invoke examples

Signed-off-by: Hauke Jung <hauke.jung@outlook.de>

* Fix fmt errors

Signed-off-by: Hauke Jung <hauke.jung@outlook.de>

* Install protoc on github workflow

Signed-off-by: Hauke Jung <hauke.jung@outlook.de>

---------

Signed-off-by: Hauke Jung <hauke.jung@outlook.de>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
* workflow - install protoc

Signed-off-by: Roberto J Rojas <robertojrojas@gmail.com>

* pinning protoc version to 3.21.12

Signed-off-by: Roberto J Rojas <robertojrojas@gmail.com>

* trying a new version

Signed-off-by: Roberto J Rojas <robertojrojas@gmail.com>

* workflow - fixes rust toolchain and protoc installs

Signed-off-by: Roberto J Rojas <robertojrojas@gmail.com>

---------

Signed-off-by: Roberto J Rojas <robertojrojas@gmail.com>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
* updates grpc proto for dapr v1.10 release

Signed-off-by: Roberto J Rojas <robertojrojas@gmail.com>

* updates dependencies to most recent

Signed-off-by: Roberto J Rojas <robertojrojas@gmail.com>

---------

Signed-off-by: Roberto J Rojas <robertojrojas@gmail.com>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
elena-kolevska and others added 10 commits March 17, 2024 16:47
* Updating to dapr runtime - master

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Updates the sdk version in the README. Gives the rest of the instructions in the file some love.

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Fixes typo

Signed-off-by: Elena Kolevska <elena@kolevska.com>

---------

Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
* Update proto

Signed-off-by: Cyril Scetbon <cscetbon@gmail.com>

* Use v1.12.4 instead of master for now

Signed-off-by: Cyril Scetbon <cscetbon@gmail.com>

---------

Signed-off-by: Cyril Scetbon <cscetbon@gmail.com>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
* Updates protos from release-1.13.0

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Now from the correct version

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* uses GetMetadataRequest

Signed-off-by: Elena Kolevska <elena@kolevska.com>

---------

Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
Signed-off-by: mikeee <hey@mike.ee>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
Signed-off-by: mikeee <hey@mike.ee>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
Signed-off-by: mikeee <hey@mike.ee>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
Signed-off-by: yaron2 <schneider.yaron@live.com>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
Signed-off-by: Marc Duiker <marcduiker@users.noreply.github.com>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
Signed-off-by: Caleb Cartwright <caleb.cartwright@outlook.com>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
Signed-off-by: Caleb Cartwright <caleb.cartwright@outlook.com>
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
# Conflicts:
#	.github/workflows/ci.yml
#	Cargo.toml
#	examples/pubsub/README.md
#	examples/pubsub/subscriber.rs
#	src/client.rs
#	src/lib.rs
Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
@zedgell
Copy link
Contributor Author

zedgell commented Mar 17, 2024

@mikeee Should be good now. Excuse all the extra commits. May have reverted a little to far the first time when I forgot to sign off lol.

@mikeee
Copy link
Member

mikeee commented Mar 17, 2024

No dramas, thank you for picking this up so quickly after it being open for a long time!
The formatted has picked up a few small diffs. Could you please take a look at those?

Signed-off-by: Zachary Edgell <zacharyedgell@gmail.com>
@zedgell
Copy link
Contributor Author

zedgell commented Mar 19, 2024

@mikeee looks like it ran fine now. Let me know if you see any other potential problems.

zedgell and others added 3 commits March 20, 2024 03:46
Signed-off-by: Zachary K Edgell <zacharyedgell@gmail.com>
Signed-off-by: Mike Nguyen <hey@mike.ee>
Copy link
Member

@mikeee mikeee left a comment

Choose a reason for hiding this comment

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

lgtm, will pick up the failing pubsub validation in another issue/pr.

@mikeee mikeee merged commit f642bd8 into dapr:main Mar 20, 2024
11 of 12 checks passed
@mikeee mikeee added the P1 label Mar 20, 2024
@mikeee mikeee mentioned this pull request Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet