Skip to content

Support for adding services dynamically.#1408

Merged
LucioFranco merged 14 commits intohyperium:masterfrom
tsroka:add-services-dynamically
Aug 25, 2023
Merged

Support for adding services dynamically.#1408
LucioFranco merged 14 commits intohyperium:masterfrom
tsroka:add-services-dynamically

Conversation

@tsroka
Copy link
Copy Markdown
Contributor

@tsroka tsroka commented Jun 6, 2023

Motivation

This change allows more flexible creation of grpc server, specifically for use case where multiple grpc services are initiated in different parts of an application. Currently it is pretty hard to do due to Server / Router generic (and consuming value every time new svc is added).

This should also address this issue

Solution

Since Routes struct does not have any generic type it can be easily used to add new services. The only complication is that it add_service fn is taking self by value and functions are not public. In order to make it work, RoutesBuilder was added that takes mutable reference to self and can be used to add new services, and some methods were made public.

Please see the provided example. This change is backwards compatible.

Alternative

Other approach would be to get rid of generic parameter from Router and decouple it from Server so that Router can be created first and latter passed to Server once all services are added. The current coupling between Router/Server is unnecessary complex (Router can be created only through instance of Server, which is cloned value stored as a Router field) and does not seem to provide any benefits.

Co-authored-by: Andrew Hickman <andrew.hickman1@sky.com>
@killix
Copy link
Copy Markdown

killix commented Jun 20, 2023

I am looking for this feature. When can this be merged? :)

@lockbox
Copy link
Copy Markdown

lockbox commented Jun 27, 2023

This works for my uses, though it is admittedly a little clunky passing a &mut RoutesBuilder around, since it's only useful/relevant on initialization it effectively serves as a slightly more flexible builder pattern that is significantly more elegant than attempting the alternative.

I think the alternative path described in the initial comment would be better going forward, however in the interim I'm going to use this workaround until a path is decided upon by the powers that be.

@tsroka
Copy link
Copy Markdown
Contributor Author

tsroka commented Jun 27, 2023

Hello, is there something else I need to do on my side to get it merged?

@killix
Copy link
Copy Markdown

killix commented Jul 16, 2023

@andrewhickman any answer?

@andrewhickman
Copy link
Copy Markdown
Contributor

I'm not a maintainer on this repo, I don't have power to merge it

@killix
Copy link
Copy Markdown

killix commented Jul 18, 2023

I'm not a maintainer on this repo, I don't have power to merge it

Asked because look you are the reviewer on this PR.

Copy link
Copy Markdown
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Overall this looks really good. I would like to see a test that uses this code to ensure we don't break things down the line then we can merge this. Thanks!

@tsroka
Copy link
Copy Markdown
Contributor Author

tsroka commented Aug 1, 2023

Thanks for reviewing @LucioFranco ; added test, lmk if this is what you were looking for.

@tsroka tsroka requested a review from LucioFranco August 3, 2023 05:47
@tsroka
Copy link
Copy Markdown
Contributor Author

tsroka commented Aug 4, 2023

@LucioFranco I saw that a build is failing due to use of let else, I changed the test to use match instead, should be good now.

@tsroka
Copy link
Copy Markdown
Contributor Author

tsroka commented Aug 13, 2023

Fixed merge conflicts with master and compilation errors introduced by changes in master. @LucioFranco do you think it is ok to merge?

@LucioFranco LucioFranco added this pull request to the merge queue Aug 25, 2023
Merged via the queue into hyperium:master with commit 5f5ae24 Aug 25, 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.

5 participants