Skip to content

Handle transient errors gracefully in health check#734

Merged
tomach merged 1 commit intomasterfrom
t/health-check-transient-errors
Apr 30, 2025
Merged

Handle transient errors gracefully in health check#734
tomach merged 1 commit intomasterfrom
t/health-check-transient-errors

Conversation

@tomach
Copy link
Contributor

@tomach tomach commented Apr 28, 2025

Summary of changes

This improves the ping_cratedb_status handler by catching transient errors (like ClientConnectorError and TimeoutError) and logs them as warnings instead of raising unhandled exceptions.

I also had to upgrade black to support match-case syntax.

Checklist

  • Link to issue this PR refers to: https://github.com/crate/cloud/issues/2566
  • Relevant changes are reflected in CHANGES.rst
  • Added or changed code is covered by tests
  • Documentation has been updated if necessary
  • Changed code does not contain any breaking changes (or this is a major version change)

@tomach tomach force-pushed the t/health-check-transient-errors branch from 8896615 to 0fdd343 Compare April 28, 2025 13:49
@tomach tomach requested review from plaharanne and widmogrod April 28, 2025 13:56
Copy link
Contributor

@plaharanne plaharanne left a comment

Choose a reason for hiding this comment

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

It looks good to me

Copy link

@widmogrod widmogrod left a comment

Choose a reason for hiding this comment

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

LGTM.

  • Checkout the code, test are running on mac
  • Code coverage looks good with this functionality

I have a nitpick.

  • This PR is shipping two changes - one is "black to support match-case syntax." and second is handling errors gracefully - I would suggest to split them in two PR or two commits. First appraoch allow to have more granular changes and simpler code review, since linter changes are deterministic, and they don't need to block more nuanced changes.

@tomach tomach force-pushed the t/health-check-transient-errors branch from 652105e to a64b95e Compare April 30, 2025 07:16
@tomach tomach force-pushed the t/health-check-transient-errors branch from a64b95e to e0c8893 Compare April 30, 2025 07:22
@tomach tomach merged commit b40d6e4 into master Apr 30, 2025
14 checks passed
@tomach tomach deleted the t/health-check-transient-errors branch April 30, 2025 07:26
@tomach
Copy link
Contributor Author

tomach commented Apr 30, 2025

  • This PR is shipping two changes - one is "black to support match-case syntax." and second is handling errors gracefully - I would suggest to split them in two PR or two commits. First appraoch allow to have more granular changes and simpler code review, since linter changes are deterministic, and they don't need to block more nuanced changes.

I created #735 for the upgrade of black

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants