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

feat: support MAX_DBCONN_RETRY_TIMES #8

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zhangzhw8
Copy link

Thank you for the package!
The dbconn_retry give up retry after retry once. Howerver, we found that sometimes retry more times could help!
This PR support to set MAX_DBCONN_RETRY_TIMES in django.conf, which is default to be 1

Best Wishes

Copy link

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

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

Hi there! I somehow had notifications on for this repo and found myself reviewing your PR before noticing. I hope you don't mind my comments. Generally, I'd advise adding some lines of documentation. Otherwise, people won't know about this setting. Maybe consider writing a test as well, to validate the new behavior works as expected. Best Joe!

django_dbconn_retry/__init__.py Outdated Show resolved Hide resolved
django_dbconn_retry/__init__.py Outdated Show resolved Hide resolved
@zhangzhw8 zhangzhw8 force-pushed the feat_support_retry_times branch 2 times, most recently from 96bab13 to eed4f8d Compare April 12, 2022 07:30
@zhangzhw8
Copy link
Author

Hi there! I somehow had notifications on for this repo and found myself reviewing your PR before noticing. I hope you don't mind my comments. Generally, I'd advise adding some lines of documentation. Otherwise, people won't know about this setting. Maybe consider writing a test as well, to validate the new behavior works as expected. Best Joe!

Thank you for your advice. I've add settings description in README.rst and improve the test case. Please re-review.

@vrnvorona
Copy link

😴

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

4 participants