-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Conversation
@@ -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) { |
There was a problem hiding this comment.
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.
@ryanjbaxter this is not a bug, but an improvement. Thank you for looking into it |
No description provided.