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

fix(cat-gateway): Correct Service Health logic #1974

Open
wants to merge 51 commits into
base: main
Choose a base branch
from

Conversation

saibatizoku
Copy link
Contributor

@saibatizoku saibatizoku commented Mar 6, 2025

Description

Logic is implemented as outlined in #1827.

Related Issue(s)

Closes #1827 .

Description of Changes

Provide a clear and concise description of what the pull request changes.

Breaking Changes

Describe any breaking changes and the impact.

Screenshots

If applicable, add screenshots to help explain your changes.

Related Pull Requests

#1921 , #1919

Please confirm the following checks

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream module

@saibatizoku saibatizoku changed the title Fix/health probe logic fix(cat-gateway): Correct Service Health logic Mar 6, 2025
@saibatizoku saibatizoku added do not merge yet PR is not ready to be merged yet review me PR is ready for review and removed draft Draft labels Mar 6, 2025
@saibatizoku saibatizoku force-pushed the fix/health-probe-logic branch from 30a233a to 3bf2daf Compare March 7, 2025 04:19
Copy link
Contributor

github-actions bot commented Mar 7, 2025

Test Report | ${\color{lightgreen}Pass: 634/634}$ | ${\color{red}Fail: 0/634}$ |

// TODO: find a better way to filter URI paths
if !req.uri().path().starts_with("/health") {
if !EventDB::connection_is_ok() {
set_index_db_liveness(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be set_event_db_liveness?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch!

Comment on lines +148 to +156
// Return 204 response if check passed initially.
if index_db_live && event_db_live {
return success_response;
}

// Otherwise, re-check, and return 204 response if all is good.
if index_db_is_live() && event_db_is_live() {
return success_response;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the first check? if index_db_live && event_db_live { Can we do just if index_db_is_live() && event_db_is_live() {?

/// Flag to determine if the service has started
static STARTED: AtomicBool = AtomicBool::new(false);

/// Flag to determine if the service has started.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Flag to determine if the service has started.
/// Flag to determine if the index db is lived.

/// Flag to determine if the service has started.
static LIVE_INDEX_DB: AtomicBool = AtomicBool::new(false);

/// Flag to determine if the service has started.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Flag to determine if the service has started.
/// Flag to determine if the event db is lived.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet PR is not ready to be merged yet enhancement New feature or request review me PR is ready for review
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

Correct behaviour of Started/Ready/Live in the Health and Other endpoints
4 participants