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.

Sign up for GitHub

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

Jump to bottom

[ISSUE-5705] Support for degraded health #5741

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

[ISSUE-5705] Support for degraded health #5741

wants to merge 20 commits into from

Conversation

seonWKim
Copy link
Contributor

@seonWKim seonWKim commented Jun 8, 2024 •
edited
Loading

Motivation:

Add support for degraded health.

Modifications:

  • Add HealthStatus enum which can represent the health status of a server
  • Add healthStatus() method in HealthChecker for fine-grained representation of server's status
  • Add degradedResponse which shows that the server's status is being degraded.

I think adding new health check service which returns HealthStatus(instead of boolean) might be an alternate option. But because this PR is before any reviews, I've just implemented feature on top of HealthCheckService.

Result:

Users can now receive notification for degraded server status like below

{"healthy":true, "degraded":true}

@seonWKim seonWKim mentioned this pull request Jun 8, 2024
Open
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 8, 2024 •
edited
Loading

🔍 Build Scan® (commit: 2f610ee)

Job name Status Build Scan®
build-windows-latest-jdk-21 ❌ (failure) https://ge.armeria.dev/s/qxcbednxsmnec
build-self-hosted-unsafe-jdk-8 ✅ https://ge.armeria.dev/s/dceuxrrdvtr6q
build-self-hosted-unsafe-jdk-21-snapshot-blockhound ✅ https://ge.armeria.dev/s/qpsnskxsidgze
build-self-hosted-unsafe-jdk-17-min-java-17-coverage ❌ (failure) https://ge.armeria.dev/s/xav2e224of3o4
build-self-hosted-unsafe-jdk-17-min-java-11 ✅ https://ge.armeria.dev/s/6op4bkjnmyo4u
build-self-hosted-unsafe-jdk-17-leak ✅ https://ge.armeria.dev/s/qt234vmsmwc2g
build-self-hosted-unsafe-jdk-11 ❌ (failure) https://ge.armeria.dev/s/n663movcu2n7u
build-macos-12-jdk-21 ✅ https://ge.armeria.dev/s/drjt6kebyzfr6

@seonWKim seonWKim force-pushed the feature/ISSUE-5705-degradable-health-checker branch from f20ed57 to 665ec13 Compare June 8, 2024 08:20
trustin
trustin reviewed Jun 8, 2024
View reviewed changes
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Thanks, @seonWKim! 🙇

core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckService.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckServiceBuilder.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthStatus.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/SettableHealthStatusChecker.java Outdated Show resolved Hide resolved
@seonWKim seonWKim force-pushed the feature/ISSUE-5705-degradable-health-checker branch from 52fdd7b to aed0a93 Compare June 14, 2024 01:24
jrhee17
jrhee17 reviewed Jun 14, 2024
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Looks good overall 👍 I think some client-side changes will be needed, but I prefer I do this to ensure compatibility with xDS-side requirements. Left some minor comments 🙇

core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthStatus.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckService.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckService.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckServiceBuilder.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckServiceBuilder.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckService.java Outdated Show resolved Hide resolved
trustin
trustin reviewed Jun 19, 2024
View reviewed changes
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Almost there!

core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthStatus.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthStatus.java Outdated Show resolved Hide resolved
seonWKim and others added 15 commits June 22, 2024 11:32
@seonWKim
Add support for degradable health status in HttpCheckService
d821502
@seonWKim
Fix CpuHealthChecker and add test
866d72f
@seonWKim
Remove SettableHealthStatusChecker and merge the features to `Setta…
f7db1d1 
…bleHealthChecker`
@seonWKim
Add healthStatusUpdated for more fine fine-grained health status up…
40fad94 
…dates
@seonWKim
Add UNDER_MAINTENANCE
a677109
@seonWKim
Fix getResponse
4ac6a6f
@seonWKim
Fix lint
eed2b0a
@seonWKim
Fix test name
59184fc
@seonWKim
Fix docs
8b5d8f9
@seonWKim
Add (server) STOPPING status
7a4b880
@seonWKim
Fix test
3af30d2
@seonWKim @jrhee17
Apply suggestions from code review
4d780db 
Co-authored-by: jrhee17 <guins_j@guins.org>
@seonWKim
Apply comments
26a4979
@seonWKim
Fix to code to consider backward compatibility and allow users to spe…
ebb4b6e 
…cify HealthStatus
@seonWKim
Apply comments
495ce58 
- Add javadoc on `HealthStatus`
- Checks for IF-NONE-MATCH header for specific `HealthStatus`
- Add `HealthStatusUpdateHandler`
@seonWKim seonWKim force-pushed the feature/ISSUE-5705-degradable-health-checker branch from 1548102 to 487997f Compare June 22, 2024 02:32
@seonWKim
Copy link
Contributor Author

Removed the [ISSUE-5741] prefix in the commit messages.

jrhee17
jrhee17 reviewed Jun 24, 2024
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Looks good overall 👍 Left a minor suggestion

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultHealthCheckUpdateHandler.java Outdated Show resolved Hide resolved
@seonWKim
Copy link
Contributor Author

Question) what do you think of just combining the two HealthCheckUpdateHandler and adding similar logic as HealthCheckService?

Looks better. I don't think we need to split them into different classes. 👍

@jrhee17 jrhee17 added this to the 1.30.0 milestone Jun 26, 2024
jrhee17
jrhee17 reviewed Jun 26, 2024
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

This is probably the last of my comments.
Please also address https://github.com/line/armeria/pull/5741/files#r1651918233 🙏

core/src/main/java/com/linecorp/armeria/server/healthcheck/SettableHealthChecker.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthChecker.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthChecker.java Show resolved Hide resolved
@seonWKim
Apply comments
03b5899 
- Update access modifier of `DefaultHealthCheckUpdateHandler`
- Update `ScheduledHealthChecker` to use healthStatus
- Remove unnecessary `this`
jrhee17
jrhee17 reviewed Jul 10, 2024
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Left some more nit comments 👍

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultHealthCheckUpdateHandler.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckServiceBuilder.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckUpdateListener.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthChecker.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/SettableHealthChecker.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/SettableHealthChecker.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/healthcheck/ScheduledHealthChecker.java Show resolved Hide resolved
@seonWKim
Apply comments
8489392 
- Add @UnstableApi
@seonWKim seonWKim force-pushed the feature/ISSUE-5705-degradable-health-checker branch from a13308f to 74f702d Compare July 18, 2024 00:10
@seonWKim seonWKim force-pushed the feature/ISSUE-5705-degradable-health-checker branch from 74f702d to 2f610ee Compare July 18, 2024 00:16
jrhee17
jrhee17 approved these changes Jul 19, 2024
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

@ikhoon ikhoon modified the milestones: 1.30.0, 1.31.0 Aug 2, 2024
@ikhoon ikhoon mentioned this pull request Aug 2, 2024
Merged
@github-actions github-actions bot added the Stale label Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trustin trustin trustin left review comments

@jrhee17 jrhee17 jrhee17 approved these changes

@ikhoon ikhoon Awaiting requested review from ikhoon ikhoon is a code owner

@minwoox minwoox Awaiting requested review from minwoox minwoox is a code owner

Assignees
No one assigned
Labels
new feature Stale
Projects
None yet
Milestone
1.31.0
Development

Successfully merging this pull request may close these issues.

Support for degraded health
4 participants
@seonWKim @trustin @jrhee17 @ikhoon