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

Modified Redis Health Check #36

Merged
merged 2 commits into from Oct 31, 2020
Merged

Modified Redis Health Check #36

merged 2 commits into from Oct 31, 2020

Conversation

Better-Boy
Copy link
Contributor

@Better-Boy Better-Boy commented Oct 19, 2020

Line - https://github.com/meedan/alegre/blob/develop/app/main/controller/healthcheck_controller.py#L44

# FIXME Redis database is not being tested by info()

Why the change from info() to ping() is necessary?

  1. The purpose of healthcheck is to check if the redis cluster/server is up and running. You don't need any other info than True/Exception.
  2. By default, redis has 16 databases, which can be addressed by their indexes starting from 0 and ending at 15. If any value other than None(defaults to 0) and 0 to 15 are given, ping() will raise a ResponseError: DB Index is out of range which is handled in the except block
  3. The return value of ping() is True if the connection is fine with checking database details. No need to assign True explicitly

Changed health check for `info` to `ping`
If redis health check is successful, redis ping will return True else Exception
@Better-Boy Better-Boy mentioned this pull request Oct 19, 2020
@Better-Boy
Copy link
Contributor Author

Better-Boy commented Oct 19, 2020

@huslage @caiosba Can any of you guys review this PR?

@caiosba
Copy link
Contributor

caiosba commented Oct 19, 2020

@huslage will this change work for the health checker?

@Better-Boy
Copy link
Contributor Author

@caiosba @huslage Any Update on this PR?

@caiosba
Copy link
Contributor

caiosba commented Oct 30, 2020

@huslage @dmou @sonoransun any of you know if that change is valid for the AWS healthchecker?

@sonoransun
Copy link
Contributor

sonoransun commented Oct 30, 2020

Looking at the change, using r.ping() instead of r.info() seems perfectly appropriate. There is additional information in info(), however, it doesn't look like any of that is necessary - only a server liveness check.

Regarding AWS, the Redis ElasticCache service for both QA and Live, qa-redis and live-redis respectively, are monitored via AWS, so there is no concern regarding ping() vs. info() in the context of AWS service health.

EDIT: one other note - if testing the database is desired (e.g. to check for data in it, rather than just that the service is alive) then it might be useful to use the dbsize command to check for records, if a healthy service should always have greater than zero entries.

@Better-Boy
Copy link
Contributor Author

@sonoransun Thank you for reviewing the PR.

@caiosba Now that the PR is reviewed, can it be approved and merged?

@caiosba caiosba merged commit 0b69782 into meedan:develop Oct 31, 2020
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.

None yet

3 participants