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

Rename 'GRPCClient.run()' #2156

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Rename 'GRPCClient.run()' #2156

wants to merge 1 commit into from

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Jan 14, 2025

Motivation:

To support swift-service-lifecycle the grpc client and server types will conform to its 'Service' protocol. This has a requirement for a method called 'run()'. Ideally this would respect graceful shutdown. We can't do this with the current client as we already have a run method. To do this we'd need to wrap the type and redeclare all of it's methods. Using a wrapper isn't a good user experience.

Modifications:

  • Rename run to 'maintainConnections()'
  • Deprecate 'run()', we'll remove this later

Result:

  • run is deprecated

Motivation:

To support swift-service-lifecycle the grpc client and server types will
conform to its 'Service' protocol. This has a requirement for a method
called 'run()'. Ideally this would respect graceful shutdown. We can't
do this with the current client as we already have a run method. To do
this we'd need to wrap the type and redeclare all of it's methods. Using
a wrapper isn't a good user experience.

Modifications:

- Rename run to 'maintainConnections()'
- Deprecate 'run()', we'll remove this later

Result:

- run is deprecated
@glbrntt glbrntt added the 🆕 semver/minor Adds new public API. label Jan 14, 2025
@glbrntt glbrntt requested a review from gjcairo January 14, 2025 16:49
@glbrntt glbrntt marked this pull request as ready for review January 14, 2025 16:49
Copy link
Collaborator

@FranzBusch FranzBusch 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 not fully following why removing run allows us to adopt service lifecycle? Why don't we keep the method and add the graceful shutdown handling in there?

@glbrntt
Copy link
Collaborator Author

glbrntt commented Jan 14, 2025

I'm not fully following why removing run allows us to adopt service lifecycle? Why don't we keep the method and add the graceful shutdown handling in there?

Because lifecycle support will be added in -extras and run() needs to know about the with-graceful-shutdown handler.

In -extras we can't reimplement run() because it already exists so any implementation would either have to forgo graceful shutdown (not good) or wrap the client (also not good).

@FranzBusch
Copy link
Collaborator

Because lifecycle support will be added in -extras and run() needs to know about the with-graceful-shutdown handler.

In -extras we can't reimplement run() because it already exists so any implementation would either have to forgo graceful shutdown (not good) or wrap the client (also not good).

I see. What do you think about using package traits to add support for swift-service-lifecycle in this package?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants