ensuring that the reconnect task terminates #5331
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shawkins
requested review from
manusa,
oscerd,
rohanKanojia and
sunix
as code owners
rohanKanojia
approved these changes
Jul 17, 2023
|
@shawkins : Could you please resolve minor conflicts? |
Partially addresses: fabric8io#5327
manusa
reviewed
Jul 18, 2023
Comment on lines
67
to
+69
| public Reflector(ListerWatcher<T, L> listerWatcher, SyncableStore<T> store) { | ||
| this(listerWatcher, store, Runnable::run); | ||
| } |
There was a problem hiding this comment.
We might want to completely remove this constructor or at least make it package-private.
manusa
approved these changes
Jul 18, 2023
|
Kudos, SonarCloud Quality Gate passed! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Partially addresses: #5327
There are two parts to the issue:
ensure that all async tasks terminate, which is what this commit does and is suitable for backport. Prior to this fix I think that informers created by a client, but that are not explicitly closed, will leak a task (which has a reference to the full client) in the scheduler queue indefinitely. After this fix they will eventually be expunged once the timeout is reached.
a larger change is needed to perform proactive cleanup of scheduled tasks when the client is closed - only the executor is currently passed around and we don't have a completeablefuture or other callback mechanism for registering things that closable. Most of the relevant timeouts are short and/or cleanup will be triggered by the failure of the httpclient. However the client side timeout is much longer and ignorant of client close, so at least a targeted improvement is warrented.
Type of change
test, version modification, documentation, etc.)
Checklist