Rename 'GRPCClient.run()'

[glbrntt]

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

[FranzBusch]

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]

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]

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?

[glbrntt]

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?

I'd rather we just did it now to be honest. Presumably traits would require us to have multiple package manifest as well which is something I'd like to avoid. We can always support via traits later as well.

[gjcairo]

LGTM overall but left a question about the rename

[gjcairo]

Minor:
I (think I) understand why maintainConnection instead of connect - because it's async thus a long-running task and the act of just connecting doesn't imply it runs for a long period of time. But I find the naming still a bit weird. Did you consider something like connectAndRun()?

[glbrntt]

That's fair, I wasn't 100% happy with the name.

Broadly speaking this function is:

• long running
• meant to be run in the background
• will manage any underlying connections

I think maintainConnections does an okay job at capturing that. Some alternatives:

• connectAndRun
• runBackgroundWork
• runBackgroundTasks
• manageConnections
• runConnections

WDYT?

[gjcairo]

I think runConnections, manageConnections and (I'll add a third one) startAndRunConnections achieve the three goals you listed the best.

[gjcairo]

Thanks George!