swift-server / swift-kafka-client

Apache License 2.0
81 stars 20 forks source link

Make `triggerGracefulShutdown()` public (was: De-couple Kafka from ServiceLifecycle to separate target) #125

Closed blindspotbounty closed 1 year ago

blindspotbounty commented 1 year ago

Currently ServiceLifecycle is a dependency for Kafka target. While it might be convenient when producers/consumers are used as a bunch of services, it is not really needed in most of other cases as simple task or task group is enough to run kafka consumers/producers with stop() method (triggerGracefulShutdown() in case of KafkaConsumer/Producer).

Though, ServiceLifecycle has its own dependencies and target, it may not bring value to the end user. So, it would be nice to preserve this compatibility on the one hand and not ship it with main target. Therefore, I wonder if it would be possible to move compatibility and related parts to separate target, please?

FranzBusch commented 1 year ago

Doing this opens up the same questions for other dependencies such as swift-log or swift-metrics. Those and swift-service-lifecycle are considered very foundational libraries in the ecosystem. I'm not a big fan of adding separate targets here just for conforming to Service. What we really want is something akin to Rust's optional features but we don't have that in SPM so IMO the best thing to do right now is just use the dependency.

Do you have any specific reason why you want to avoid it?

blindspotbounty commented 1 year ago

Hi Franz! I am totally agree with you that making separate targets for compliance with dependencies is weird.

TL;DR the thoughts + conclusion in the end

So, let me start with explaining our use-case. There are several things why we avoid lifecycle (2.0 in particular):

  1. Unlike log which has 1.x major version and metrics with 2.x (de-facto 1.0 with one major change with time units), lifecycle 2.0 is absolutely different library from 1.0 version
  2. We use lifecycle 1.0 as 2.0 doesn't suit our needs, plus what important it clashes with lifecycle 2.0
  3. It has target "ConcurrencyHelpers" which clash with any other target named the same way (https://github.com/apple/swift-package-manager/issues/6771) + SPM have other bugs with downstream projects when there are aliases specified.
  4. It produces a lot of infrastructure that is unused

Though, it might be useful sometimes when there is a singleton application with just one consumer or producer and main service (which is not true at least for transactions), it seems useless when there are several services running within one process or e.g. several consumers under some other abstractions.

  1. Kafka services are usually used by other services, so they might be better start and shutdown before/after them
  2. As a result, unlikely kafka services would use signals for shutdown and will be shutdown manually with serviceGroup.triggerGracefulShutdown() before the service it is used in
  3. Additionally, if separate services will have different kafka clients, it will lead to lots of service groups
  4. That might be not always a use-case, but if application uses multiple services and one of them is kafka, any 'fatal' error (meaning require restart connection/re-create kafka client) from librdkafka will require restart the whole service group but not particular service.

We are going to get rid of service lifecycle as 1.0 version is not supported anymore and 2.0 is not suitable for us. So it won't be a big problem from our side. However, it would be nice to remove enforcing service lifecycle as a must since it becomes a fat wrapper for Kafka for us.

Just to conclude:

  1. It is okay to have service lifecycle as a dependency (taking in attention that it is foundational infrastructure)
  2. Would be nice to allow use KafkaConsumer/KafkaProducer without ServiceLifecycle where it is not needed (in particular: allow to use stop()/triggerGracefulShutdown() method of KafkaConsumer/KafkaProducer)

This issue is minor, and more like 'nice to have'.

FranzBusch commented 1 year ago

Unlike log which has 1.x major version and metrics with 2.x (de-facto 1.0 with one major change with time units), lifecycle 2.0 is absolutely different library from 1.0 version

Yes, but 2.0 is going to be the de-facto standard going forward and I don't expect us to create a 3.0 anytime soon.

We use lifecycle 1.0 as 2.0 doesn't suit our needs, plus what important it clashes with lifecycle 2.0

I think at this point, you have to vendor lifecycle 1.0 yourself which was always an alpha version.

Additionally, if separate services will have different kafka clients, it will lead to lots of service groups

For most applications there should only be a single top-level service group running.

Would be nice to allow use KafkaConsumer/KafkaProducer without ServiceLifecycle where it is not needed (in particular: allow to use stop()/triggerGracefulShutdown() method of KafkaConsumer/KafkaProducer)

I think that is fair and feel free to open a PR to expose the triggerGracefulShutdown() method publicly.

blindspotbounty commented 1 year ago

Franz, thank you for the answers!

I think that is fair and feel free to open a PR to expose the triggerGracefulShutdown() method publicly.

I think, I will open PR next days. Thank you.

blindspotbounty commented 1 year ago

I believe this issue can be closed. @FranzBusch thank you for your thoughts and for merging PR!

felixschlegel commented 1 year ago

Thanks for your contribution!