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

improve number of method calls to k8s api server for endpoints catalog watcher #1393

Merged
merged 5 commits into from
Aug 2, 2023

Conversation

wind57
Copy link
Contributor

@wind57 wind57 commented Jul 28, 2023

No description provided.

@@ -225,7 +227,7 @@ else if (!properties.namespaces().isEmpty()) {
static List<Endpoints> withFilter(List<Endpoints> initial, KubernetesDiscoveryProperties properties,
KubernetesClient client, Predicate<Service> filter) {

if (properties.filter() == null || properties.filter().isBlank()) {
if (properties.filter() == null || properties.filter().isBlank() || filter == ALWAYS_TRUE) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is interesting and got my attention purely by accident, while working on a different thing.

Both discovery and catalog watch need to search for endpoints, as such they both delegate to the same method. At the same time, because of an issue we had, when searching for endpoints, we need to filter them based on a Predicate. That predicate is a service predicate, it sounds weird - but there was a real defect for this. An example will make this may be easier to understand:

  • in discovery implementation, we search for endpoints
  • users have specified a service SPeL expression (predicate)
  • as such, after we find the needed endpoints, we then go on to find the services for these endpoints (they have to match by name), but only take those that match the predicate.
  • with these service names, we then filter the endpoints and that is the result.

In plain english, it's like we filter endpoints based on a service predicate.

On the other hand, we also need to look for endpoints in the catalog implementation. The difference is that catalog does not support a predicate.

Both catalog and discovery delegate to the same method when looking for endpoints.


And now suppose the case:

  • users have enabled some SPeL Expression
  • they also enabled catalog watcher

Suppose catalog watcher calls this method, and since it does not care about service filtering it passes x -> true as the predicate into it. This is how it looks like:

List<Endpoints> endpoints = endpoints(context.properties(), context.kubernetesClient(),
				context.namespaceProvider(), "catalog-watcher", null, x -> true);

This if statement:

if (properties.filter() == null || properties.filter().isBlank()

is false (because users have enabled a SPeL Expression), so catalog watcher will go on and start searching for services and to try and filter them out by the predicate, again, since this is what the common method does.

But it does not need to do that, since it does not support filtering anyway (we used to pass x -> true always).

So with this fix, we essentially trim down a few method calls to the API server, when not needed.

@wind57 wind57 changed the title fix improve number of method calls to k8s api server for endpoints catalog watcher Jul 28, 2023
@wind57 wind57 marked this pull request as ready for review August 2, 2023 04:54
@wind57
Copy link
Contributor Author

wind57 commented Aug 2, 2023

@ryanjbaxter this is not a bug, but an improvement. Thank you for looking into it

@ryanjbaxter ryanjbaxter added this to the 3.0.5 milestone Aug 2, 2023
@ryanjbaxter ryanjbaxter merged commit a9c072b into spring-cloud:3.0.x Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants