2.0 API proposal

[tomerd]

Proposal for 2.0 version of the API, based on Swift concurrency

[tomerd]

@yim-lee @ktoso updated subscribe to return [Instance]. ptal

[tomerd]

cc @fabianfett

[fabianfett]

I think my biggest feature request for a 2.0 would be to drop the associated Service type on ServiceDiscovery. The Service types makes using ServiceDiscovery more complicated then it needs to be.

Currently proposed protocol:

public protocol ServiceDiscovery {
    associatedtype Service
    associatedtype Instance

    func lookup(_ service: Service, deadline: ContinuousClock.Instant?) async throws -> [Instance]
    func subscribe(_ service: Service) async throws -> any ServiceDiscoveryInstancesSequence<Instance>
}

This means a type that want's to use ServiceDiscovery needs to be generic over three types. Let's assume we want to build a RedisClient that uses ServiceDiscovery:

protocol RedisInstance: Hashable {}

final class RedisClient<Discovery: ServiceDiscovery, Service: Hashable, Instance: RedisInstance > {
  init(
    discovery: Discovery,
    service: Service
  )

  func run() {
    for try await instances in self.discovery.subscribe(self.service) {
      // do something with the results
    }
  }
}

Personally I think that users should inject the Service into their ServiceDiscovery object on creation. For most usecases you are only interested in one service. And for instances where you are interested in more Services you are always free to use multiple ServiceDiscovery instances.

Changed protocol:

public protocol ServiceDiscovery {
    associatedtype Instance
    associatedtype Subscription: AsyncSequence where Subscription.Element == [Instance]

    func lookup() async throws -> [Instance]
    func subscribe() async throws -> Subscription
}

Other changes:

1. I removed the deadline form the lookup call. I think users should enforce deadlines in their code and express a hit deadline through cancelling the task. Libraries should not implement this over and over again. We might want to push for a stdlib addition for this eventually. withDeadline {}
2. I changed the protocol to be fully generic.

The really cool thing now is the following: Library authors might not need to depend on ServiceDiscovery at all. They can express their requirement completely through AsyncSequences:

protocol RedisInstance {}

final class RedisClient<DiscoverySubscription: AsyncSequence> where DiscoverySubscription.Element: Sequence<some RedisInstance> {
  init(subscription: DiscoverySubscription)

  func run() {
    for try await instances in self.subscription {
      // do something with the results
    }
  }
}

@tomerd wdyt?

[fabianfett]

HostPort should live in Tests only.

[tomerd]

@tomerd wdyt?

I like the idea of clients not needing to depend on it explicitly -- not that we have AsyncSequence the API is really light.

@yim-lee wdyt?

[yim-lee]

@fabianfett @tomerd +1

[tomerd]

the one thing I am less sure about is removing Service since it makes for a less flexible API for more complex use cases. @yim-lee what use cases did you have in mind for it originally?

[yim-lee]

For most usecases you are only interested in one service.

This is true.

Personally I think that users should inject the Service into their ServiceDiscovery object on creation....And for instances where you are interested in more Services you are always free to use multiple ServiceDiscovery instances.

In v1 ServiceDiscovery knows about instances of all services. What this comment suggests is that in v2 each ServiceDiscovery would know about instances for one service only, in which case it would make sense to drop Service from ServiceDiscovery since neither lookup nor subscribe requires service param anymore.

I think we can drop lookup as well, because in most if not all cases people use subscribe only.

what use cases did you have in mind for it originally?

As described above, the proposed model is very different from that of v1. If we agree on the new model, then yes, I think we can drop Service from ServiceDiscovery.

[tomerd]

okay update the proposal @yim-lee @fabianfett. did end up keeping the ServiceDiscoveryInstancesSequence protocol instead of a associated type as suggested since without it I found I would need to make the internal implementation of the async sequence public which is not desirable IMO. thoughts?

[tomerd]

thanks for the ideas and PR @fabianfett, applied!

[fabianfett]

Another round of reviews

[fabianfett]

I guess this one is Sendable?

[fabianfett]

Do we want to require that the DiscoverySequence is Sendable? This will make usage sites much simpler.

[fabianfett]

Protocol has the same name as module. ‼️🚨 For using namespaces? Module rename to ServiceDiscoveryModule?

[yim-lee]

Or maybe rename one of them to just Discovery?

[hassila]

Protocols that describe a capability should be named using the suffixes able, ible, or ing (e.g. Equatable, ProgressReporting).

So ServiceDiscovering?

[weissi]

This is a thing that can discover services, maybe ServiceDiscoverer?

[ser-0xff]

Hi!
Had a look at suggested new API for service discovery.
It is not clear to me how is it supposed to cancel a subscription?
For example user called subscribe(), got a sequence and then iterate it...
And then by some external event it should cancel the iterating loop, which is just waiting in the iterator.next() call.
What options to do it with a suggested API?

[fabianfett]

Hi!
Had a look at suggested new API for service discovery.
It is not clear to me how is it supposed to cancel a subscription?
For example user called subscribe(), got a sequence and then iterate it...
And then by some external event it should cancel the iterating loop, which is just waiting in the iterator.next() call.
What options to do it with a suggested API?

@ser-0xff the iterator.next() call must support task cancellation. This means that those iterators will throw a CancellationError or return nil from the iterator.next() call. Most services will implement using withTaskCancellationHandler. Is that explanation sufficient?

[ser-0xff]

Thank you for the reply.

[tomerd]

@yim-lee @fabianfett this has been updated per our discussion.

the one remaining issue I am aware of is #45 (comment). I dont have a good name atm

[yim-lee]

Or maybe rename one of them to just Discovery?

[fabianfett]

This protocol is a bit weird. I assume your intention is much closer to this:

tomerd#2

Let me know what you think :)