ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.29k stars 1.63k forks source link

Generic polling mechanism for discovery services (Consul, Eureka...) #1708

Open ggnaegi opened 11 months ago

ggnaegi commented 11 months ago

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:

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

raman-m commented 10 months ago

Gui, thanks for the new issue!

(Consul, Eureka...)

What about ServiceFabric? Can this upgrade be applied to ServiceFabric?

ggnaegi commented 10 months ago

@raman-m it has been reopened.

ggnaegi commented 10 months ago

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?

raman-m commented 10 months ago

@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...

ggnaegi commented 10 months ago

@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.

raman-m commented 10 months ago

@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.

ggnaegi commented 10 months ago

@raman-m I will add some unit test and acceptance test and I will let you know when it's ready.

raman-m commented 10 months ago

Will you be on time by November 1st? I don't think so... It seems, this feature comes to the next November's release

ggnaegi commented 4 months ago

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.

raman-m commented 4 months ago

@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...

ggnaegi commented 4 months ago

@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.

raman-m commented 2 months ago

@ggnaegi commented on Apr 8:

🆗 When will you create a PR?