Skip to content

Grpc: Use a different method for the client id#609

Merged
zkao merged 2 commits intofarcaster-project:mainfrom
sedited:grpcdClient
Aug 2, 2022
Merged

Grpc: Use a different method for the client id#609
zkao merged 2 commits intofarcaster-project:mainfrom
sedited:grpcdClient

Conversation

@sedited
Copy link
Copy Markdown
Contributor

@sedited sedited commented Jul 29, 2022

Instead of appending an id to every single request that a client can issue, this commit introduces a method to leverage the existing id infrastructure provided by rust-microservices and its ServiceId.

Grpcd now sends requests through the control bus with a special GrpcdClient Service Id as the source. Once these requests are handled in one of the other daemons, they now use a special send_client_ctl function that sends the request back to the original Grpc ServiceId and adds the GRpcClientId as the source of the request. This allows the Grpc daemon to extra the id from the source of its received requests and issue responses back to the correct grpc client.

Instead of appending an id to every single request that a client can
issue, this commit introduces a method to leverage the existing id
infrastructure provided by rust-microservices and its ServiceId.

Grpcd now sends requests through the control bus with a special
GrpcdClient Service Id as the source. Once these requests are handled in
one of the other daemons, they now use a special `send_client_ctl`
function that sends the request back to the original Grpc ServiceId and
adds the GRpcClientId as the source of the request. This  allows the
Grpc daemon to extra the id from the source of its received requests and
issue responses back to the correct grpc client.
@sedited sedited marked this pull request as ready for review July 29, 2022 15:14
Copy link
Copy Markdown
Member

@zkao zkao left a comment

Choose a reason for hiding this comment

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

i'm neutral on this one. The advantages it creates not clearly compensates for the hacky usage of headers

Comment thread src/service.rs
@sedited
Copy link
Copy Markdown
Contributor Author

sedited commented Jul 30, 2022

The advantages it creates not clearly compensates for the hacky usage of headers

Do you have an alternative?

@zkao
Copy link
Copy Markdown
Member

zkao commented Jul 30, 2022

Do you have an alternative?

maybe I did not get the problem it tries to solve correctly, could you please state it?

i'd say adding optional input parameters to send_client_ctl instead of reinterpreting it on a case by case basis, i will try to rewrite its signature

@sedited
Copy link
Copy Markdown
Contributor Author

sedited commented Aug 2, 2022

maybe I did not get the problem it tries to solve correctly, could you please state it?

I'll restate it again. The Grpc server can have many clients, or even a single client issuing multiple requests of the same kind. To track these requests internally they require the inclusion of some form of unique request id per API call. With the current method every single cli command (to ensure that the grpc server and the cli have feature parity) would have to include such an Id. This greatly pollutes the code, since we'd have to pass these id's through multiple requests and would probably even need to persist them in the microservice runtime for some calls that result in a long chain of requests.

So instead of adding this complicated handling of such an Id, I leveraged the existing esb infrastructure to convey this information when reporting results back to clients.

@zkao
Copy link
Copy Markdown
Member

zkao commented Aug 2, 2022

Thank you for bearing with me.

I'd just propose the minor diff, since the fn is called send_client_ctl

2 files changed, 1 insertion(+), 2 deletions(-)
src/farcasterd/runtime.rs | 1 -
src/service.rs            | 2 +-

modified   src/farcasterd/runtime.rs
@@ -901,7 +901,6 @@ impl Runtime {
             Request::GetInfo => {
                 debug!("farcasterd received GetInfo request");
                 self.send_client_ctl(
-                    ServiceBus::Ctl,
                     endpoints,
                     source,
                     Request::NodeInfo(NodeInfo {
modified   src/service.rs
@@ -405,11 +405,11 @@ where
 
     fn send_client_ctl(
         &mut self,
-        bus: ServiceBus,
         senders: &mut Endpoints,
         dest: ServiceId,
         request: Request,
     ) -> Result<(), Error> {
+        let bus = ServiceBus::Ctl;
         if let ServiceId::GrpcdClient(_) = dest {
             senders.send_to(bus, dest, ServiceId::Grpcd, request)?;
         } else {

@sedited
Copy link
Copy Markdown
Contributor Author

sedited commented Aug 2, 2022

Thanks for the suggestion, done in 2d9bcfa

@zkao zkao merged commit 377f156 into farcaster-project:main Aug 2, 2022
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.

2 participants