Ocelot
Ocelot copied to clipboard
ThreeMammals
Generic polling mechanism for discovery services (Consul, Eureka...)
Expected Behavior / New Feature
The polling of discovery services is not specific to one type of discovery service. Therefore, polling management should not be specific to a service but generic.
So here we want to propose:
- A service manager that manages the list of microservice types (clusters)
- An instance manager that manages the list of possible destinations
Obtaining destination addresses is specific to each discovery service and must be implemented accordingly.
Actual Behavior / Motivation for New Feature
The polling of discovery services at long intervals is motivated by performance problems. Obtaining the list of services for each request can have a negative impact on gateway performance.
Polling is already implemented for Consul, but also for Kubernetes. Unfortunately, the two providers are implemented in different ways.
So the aim here is to propose a generic implementation of polling so that it can be offered for any type of discovery service (eg. Eureka).
Specifications
- Version: latest, 19.x & .NET 7
Gui, thanks for the new issue!
(Consul, Eureka...)
What about ServiceFabric? Can this upgrade be applied to ServiceFabric?
Gui, thanks for the new issue!
(Consul, Eureka...)
What about ServiceFabric? Can this upgrade be applied to ServiceFabric?
@raman-m yes, but should I add a new provider project?
@ggnaegi commented on Oct 16 should I add a new provider project?
Oh, no! It should be discussed. I don't like the approach of multiple NuGet projects which are tiny!
As ServiceFabricServiceDiscoveryProvider class, ServiceFabric provider belongs already to Ocelot core. But other providers are located in separate projects. No single design... but logically this is the same Service Discovery feature.
I have no time to discuss that design issue for now.
If we include ServiceFabric provider in this refactoring then it requires to extract it from Ocelot core, define new project, make a release, have new NuGet package... It is a pain in the neck!
Let's skip ServiceFabric provider for now, OK?
If your idea and solution will be solid, stable and helpful, we will extend this idea to ServiceFabric provider. But, as you see, it will be not easy to refactor the provider. We must merge all providers projects to one NuGet package first... This is another challenge, and it blocks ServiceFabric provider refactoring.
How a long will it take to extract ServiceFabric provider to separate project? It will be massive PR, I guess...
@raman-m We could keep it like that for now. I don't think polling has been foreseen for ServiceFabric. It's not that complicated to extract the provider though, but as you wrote, the issue is more the CD and the associated breaking changes.
@ggnaegi Aha!
But I don't see any objections to move your helpers to a common project like Ocelot.ServiceDiscovery, and make a reference in provider's project including a reference in Ocelot project, and you will be able to change/refactor ServiceFabric provider too, without movement to a separate project which requires NuGet packaging which is a headache. Sounds good?
Probably in future, Ocelot.ServiceDiscovery will be a candidate for NuGet packaging of all ServiceDiscovery providers. The goal: to decrease the number of small projects being published to NuGet as separate tiny packages.
Remember, Tom had archived all ServiceDiscovery repos in 2018 and moved to separate folders/projects inside of the main Ocelot repository.
@raman-m I will add some unit test and acceptance test and I will let you know when it's ready.
Will you be on time by November 1st? I don't think so... It seems, this feature comes to the next November's release
As discussed, the PR has been closed. I will first create a new PR adressing the issues discussed here: https://github.com/ThreeMammals/Ocelot/discussions/1910 A complete redesign will be part of a later PR.
@ggnaegi I understood the delivery! When will you have the time for this? When will you start? I guess this task should have low priority because we have other milestone tasks...
@raman-m I'm on it, the feature is already implemented, I'm writing the test cases right now. I wouldn't put low, since it's a great feature and a feature that could help us winning some new users.