Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ggnaegi opened this issue Sep 29, 2023 · 13 comments
Open

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

ggnaegi opened this issue Sep 29, 2023 · 13 comments
Assignees
Labels
needs feedback Issue is waiting on feedback before acceptance proposal Proposal for a new functionality in Ocelot Service Discovery Ocelot feature: Service Discovery

Comments

@ggnaegi
Copy link
Member

ggnaegi commented Sep 29, 2023

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
@ggnaegi ggnaegi changed the title Generalized polling mechanism for discovery services (Consul, Eureka...) Generic polling mechanism for discovery services (Consul, Eureka...) Oct 3, 2023
@raman-m
Copy link
Member

raman-m commented Oct 9, 2023

Gui, thanks for the new issue!

(Consul, Eureka...)

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

@raman-m raman-m added proposal Proposal for a new functionality in Ocelot Service Discovery Ocelot feature: Service Discovery labels Oct 9, 2023
@ggnaegi ggnaegi closed this as not planned Won't fix, can't repro, duplicate, stale Oct 9, 2023
@ggnaegi ggnaegi reopened this Oct 13, 2023
@ggnaegi
Copy link
Member Author

ggnaegi commented Oct 13, 2023

@raman-m it has been reopened.

@ggnaegi
Copy link
Member Author

ggnaegi commented Oct 16, 2023

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
Copy link
Member

raman-m commented Oct 16, 2023

@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 raman-m added the needs feedback Issue is waiting on feedback before acceptance label Oct 16, 2023
@ggnaegi
Copy link
Member Author

ggnaegi commented Oct 16, 2023

@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
Copy link
Member

raman-m commented Oct 16, 2023

@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
Copy link
Member Author

ggnaegi commented Oct 28, 2023

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

@raman-m
Copy link
Member

raman-m commented Oct 28, 2023

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

@raman-m raman-m added this to the December'23 milestone Nov 25, 2023
@raman-m raman-m added the 2023 Annual 2023 release label Nov 25, 2023
@ggnaegi
Copy link
Member Author

ggnaegi commented Apr 8, 2024

As discussed, the PR has been closed. I will first create a new PR adressing the issues discussed here: #1910
A complete redesign will be part of a later PR.

@raman-m
Copy link
Member

raman-m commented Apr 8, 2024

@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 raman-m added the low Low priority label Apr 8, 2024
@ggnaegi
Copy link
Member Author

ggnaegi commented Apr 8, 2024

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

@ggnaegi ggnaegi removed the low Low priority label Apr 8, 2024
@raman-m
Copy link
Member

raman-m commented Jun 13, 2024

@ggnaegi commented on Apr 8:

🆗 When will you create a PR?

@raman-m raman-m added Oct'24 October 2024 release and removed 2023 Annual 2023 release labels Oct 9, 2024
@raman-m raman-m modified the milestones: Annual 2023, October'24 Oct 9, 2024
@raman-m raman-m removed the Oct'24 October 2024 release label Oct 26, 2024
@raman-m raman-m removed this from the October'24 milestone Oct 26, 2024
@raman-m
Copy link
Member

raman-m commented Oct 26, 2024

Gui, I've unassigned the milestone and delivery-tag because I have no idea when will you deliver this feature and how.
So, please plan to create a PR or just close it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs feedback Issue is waiting on feedback before acceptance proposal Proposal for a new functionality in Ocelot Service Discovery Ocelot feature: Service Discovery
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants