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

Catch Connection Error when redis isn't up #22 #24

Merged
merged 7 commits into from
Aug 28, 2016
Merged

Catch Connection Error when redis isn't up #22 #24

merged 7 commits into from
Aug 28, 2016

Conversation

0verbyte
Copy link
Contributor

This handles the ConnectionError both when starting the server and if the redis server goes away during the request lifecycle.

@@ -21,15 +23,27 @@
'hour': 3600
}

def check_redis_alive(force_exit = False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe PEP8 style says no spaces around the =.

@jparise
Copy link
Collaborator

jparise commented Aug 21, 2016

Thanks, this is helpful! Three small bits of feedback:

  1. Could you include some unit tests that cover this change?
  2. Are there other places this should be checked?
  3. Would it be clearer to implement this check as a function decorator or context manager?

(I think the answer to (2) is "no" but wanted to make sure.)

@0verbyte
Copy link
Contributor Author

  1. Since travis handles the redis-server. What is the preferred way to handle the mock connections and simulate a failure bringing redis offline?
  2. I don't believe any other routes should be checked at this time.
  3. Has been converted to use a decorator. The decorator handles whether the connection is checked at server start or within a request.

try:
redis_client.ping()
except ConnectionError as e:
print(e)
Copy link
Collaborator

@nichochar nichochar Aug 22, 2016

Choose a reason for hiding this comment

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

Nice, how about also printing a friendly error message? I'm debating if we need only a pretty error message like "Failed to connect to redis".
I don't mind using message + stacktrace, but let's make it user friendly.

@nichochar
Copy link
Collaborator

nichochar commented Aug 22, 2016

Awesome! Thanks for making the change @voidpirate

I also had some minor suggestions in the comments.

Please address before merge

  • try calling the function in the try/except rather than the ping()

Won't block merge

  • would be nice to have a custom error template
  • tests

@jparise the reason I put the testing in the optional here is that I think we should actually mock the redis server entirely. I created an issue

redis_client.ping()
except ConnectionError as e:
print(e)
if fn.__name__ == "main":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor nit: can we switch this to single quotes? I haven't done it yet, but I am going to write a coding style for this repo, and strings favor single quotes if possible.

@0verbyte
Copy link
Contributor Author

0verbyte commented Aug 23, 2016

I've made the changes. Thank you very much for the feedback :)

The fails on travis are actually not due to any of the code changes, but failed to install Python 3.5.
screen shot 2016-08-22 at 10 05 59 pm

@nichochar
Copy link
Collaborator

Kicked the travis job back, and it's now successful.
Thanks for making all the changes @voidpirate !

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