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

The blackListedServiceInstances in SpringCloudCommandRouter should be reevaluated from time to time #6

Closed
dhermanns opened this issue Apr 26, 2019 · 8 comments · Fixed by #168

Comments

@dhermanns
Copy link
Contributor

It looks to me like a once blacklisted service will stay blacklisted. It will be removed in class SpringCloudCommandRouter here:

    private void cleanBlackList(List<ServiceInstance> instances) {
        blackListedServiceInstances.removeIf(
                blackListedInstance -> instances.stream().noneMatch(instance -> equals(instance, blackListedInstance))
        );
    }

only, if the serviceInstance isn't discovered anymore at all.

This could be problematic, if you are using the SpringCloudHttpBackupCommandRouter.
It blacklists a service just because the method:

private Optional<MessageRoutingInformation> requestMessageRoutingInformation(ServiceInstance serviceInstance) {

failed. This could be the case during the bootup phase: Consul already discovers the service, but the HTTP Endpoint isn't reachable. In our case, the service was blacklisted for all times as a result.

Suggestion: Reevaluate the blacklisted services from time to time. That would work around the race condition during service startup. If you like, I could add a solution to my already existing PR #4.

@smcvb
Copy link
Member

smcvb commented May 6, 2019

Fair suggestion @dhermanns and definitely something we've been thinking about.
A pull request for this would be much welcomed!
But we'd prefer this to be a separate pull request from #4.

What was your suggested plan of attack to reevaluate the black-listed ServiceInstances?

@dhermanns
Copy link
Contributor Author

dhermanns commented May 8, 2019

Simplest way - and that is how I implemented it so far, is adding the possibility to disable blacklisting using the SpringCloudCommandRouter.Builder.

@smcvb
Copy link
Member

smcvb commented May 9, 2019

Some setting through the builder indeed makes a lot of sense.
From what you're sharing however, this simply sounds like a boolean specifying whether to black list yes or no.

I however thought along the lines of reevaluating like stated in the title.
Thus cleaning the list or verifying the list at set intervals.

So, maybe a two fold setting, (1) specifying the interval and (2) the operation to perform in regards to reevaluating.
The time interval could in that scenario be set to -1 for example so that it will never trigger.

Regardless, disabling altogether is also a feature request of course.

What do you think @dhermanns? Would you prefer a feature to disable black listing entirely?

@dhermanns
Copy link
Contributor Author

dhermanns commented May 9, 2019 via email

@smcvb smcvb closed this as completed May 13, 2019
@smcvb smcvb reopened this May 13, 2019
@smcvb
Copy link
Member

smcvb commented May 13, 2019

Ah, didn't mean to close this issue just yet, sorry for that..

However, it might come to that in a couple.
Based on the other issue you've opened recently @dhermanns, more specifically #8, we think we might have a good suggestion to resolve both your suggestions in one change.

The current implementation will black list a ServiceInstance under the circumstances that (1) the ServiceInstance#getMetadata didn't contain any Command Filter information, and (2) the SpringCloudHttpBackupCommandRouter implementation was unable to retrieve information from the other instance.
Both of these operations are part of deducing what the MessageRoutingInformation of a given ServiceInstance is.

The alternative solution I suggested just now in #8, which will likely come down to configuring a Function<ServiceInstance, MessageRoutingInformation> in the Builder to decide how to map a ServiceInstance to a MessageRoutingInformation, could just as well be a trigger point to black list a node. Ideally, this black listing operation would also be configurable through the Builder, just as the deduction of MessageRoutingInformation.

The defaulted functions would, for both the SpringCloudCommandRouter and the SpringCloudHttpBackupCommandRouter, keep black listing a node in the above described scenarios.
For your use case however, you'd drop the black listing entirely.

What do you think @dhermanns, would that work?

@dhermanns
Copy link
Contributor Author

Sounds great! I think that should help in both use cases I have raised.

@smcvb
Copy link
Member

smcvb commented May 14, 2019

Great! Then I think we should be able to move forward with this issue under the description I've provided.
Thanks for your feedback on this @dhermanns!

smcvb added a commit that referenced this issue Dec 23, 2020
- Store the entire ServiceInstance instead of just the identifier. Doing
 so will uncover reuse of id's, with different host/port combinations
- To match ServiceInstances, reuse the custom equals method which was
present in the SpringCloudCommandRouter. Doing so ensure we cover for
scenarios where the ServiceInstance does not implement equals
- Clean up the javadoc accordingly
- Remove the cleanIgnoredInstanceSet operation. Any conceivable
solutions in this space reflect the intent of issue #6. To not pull in
that issue too, that logic should be removed to that issue for a
following release. Furthermore, the old approach of clean up, which
based itself on the entire set of discoverable ServiceInstances no
longer works, as we fetch capabilities per instances, not all of them at
once

#1647
@smcvb smcvb added this to the Release 4.5 milestone Jan 15, 2021
@smcvb
Copy link
Member

smcvb commented Jan 15, 2021

I've adjusted the priority and changed the milestone of this issue.
As with the inclusion of issue #23, thus the introduction of several CapabilityDiscoveryMode implementations, allowing for a "blacklist reevaluation" solution becomes more straightforward.

For the time being, I think this issue will require us to adjust the IgnoreListingDiscoveryMode (the component which replaces the old blacklisting logic) to remove instances from its ignored list.
Several approaches would be imaginable here, like "after x invocation" or "after y time" we reevaluate a given ServiceInstance to find its capabilities.

As #23 has been introduced into release 4.4, I think it only fair to start working on this issue for milestone 4.5.

@smcvb smcvb self-assigned this Jan 27, 2022
smcvb added a commit that referenced this issue Jan 27, 2022
The IgnoreListingDiscoveryMode should let ignored ServiceInstances
expire after some time. This expiry time should be configurable to allow
 users to choose when to evict entries.

#6
smcvb added a commit that referenced this issue Jan 27, 2022
Adjust coverage run approach

#6
smcvb added a commit that referenced this issue Jan 27, 2022
Adjust coverage run approach for the pull request task

#6
smcvb added a commit that referenced this issue Jan 28, 2022
Adjust the threshold from long to Duration

#6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants