FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

improve: informer health check should not rely on isWatching by csviri · Pull Request #3209 · operator-framework/java-operator-sdk · GitHub

/ java-operator-sdk Public

improve: informer health check should not rely on isWatching - #3209

Merged
csviri merged 1 commit into
operator-framework:nextfrom
csviri:informer-health-update
Mar 9, 2026
ConversationCommits (1)ChecksFiles changed
Merged

improve: informer health check should not rely on isWatching#3209
csviri merged 1 commit into
operator-framework:nextfrom
csviri:informer-health-update

Conversation

csviri commented Mar 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

See linked issue for details.

Signed-off-by: Attila Mészáros a_meszaros@apple.com

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 8, 2026
csviri linked an issue Mar 8, 2026 that may be closed by this pull request
Update health check for Fabric8 client 7.4.0 #2978
Closed
csviri marked this pull request as ready for review March 8, 2026 18:08
Copilot AI review requested due to automatic review settings March 8, 2026 18:08
openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 8, 2026
openshift-ci Bot requested review from metacosm and xstefank March 8, 2026 18:08
csviri requested a review from shawkins March 8, 2026 18:09
Copilot AI reviewed Mar 8, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Pull request overview

This PR improves the informer health check logic by removing the dependency on isWatching() from the getStatus() method in InformerWrapper. The health status is now determined solely by isRunning() && hasSynced(), which avoids false-unhealthy readings that could occur when isWatching() returns false transiently (e.g., during informer reconnection). The isWatching() method itself remains implemented and available in the InformerHealthIndicator interface for use by other consumers.

Changes:

  • Removed isWatching() from the health status computation in getStatus()
  • Removed isWatching() from the debug log message and its corresponding argument

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

csviri commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator Author

cc @shawkins pls take a look, based on the discussions and research, probably this is the best we can do.

shawkins commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator

cc @shawkins pls take a look, based on the discussions and research, probably this is the best we can do.

This is fine. It's effectively no different than it was prior to the update of the isWatching behavior.

csviri commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator Author

cc @shawkins pls take a look, based on the discussions and research, probably this is the best we can do.

This is fine. It's effectively no different than it was prior to the update of the isWatching behavior.

To my understanding is different in the terms that we won't have false health probe failures which would not be solved with controller restart.

shawkins commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator

To my understanding is different in the terms that we won't have false health probe failures which would not be solved with controller restart.

I mean this makes the behavior same as it used to be - as isWatching in older releases was effectively dominated by isWatching / hasSynced.

csviri commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator Author

Maybe we could have some other nice runtime information for liveness probes in the future like the reconcile executor service related checks on, but those are already covered by metrics. Will think about it.

improve: informer health check should not rely on isWatching
21db29c 
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
csviri force-pushed the informer-health-update branch from c649bdb to 21db29c Compare March 9, 2026 15:21
csviri merged commit 6396b48 into operator-framework:next Mar 9, 2026
26 checks passed
csviri added a commit that referenced this pull request Mar 13, 2026
improve: informer health check should not rely on isWatching (#3209)
543b917 
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
mark-mcclure mentioned this pull request Mar 27, 2026
Closed
ThoSap mentioned this pull request May 15, 2026
Merged
metacosm added a commit that referenced this pull request Jun 5, 2026
improve: informer health check should not rely on isWatching (#3209) (#…
b9cfe13 
…3395)

Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Co-authored-by: Attila Mészáros <a_meszaros@apple.com>
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
metacosm metacosm approved these changes
xstefank Awaiting requested review from xstefank
shawkins Awaiting requested review from shawkins

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update health check for Fabric8 client 7.4.0

4 participants

Back | FazBrowse Home | New Git URL