-
Notifications
You must be signed in to change notification settings - Fork 415
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: Add support for replicas and cross region replicas #3300
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Uffizzi Preview |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3300 +/- ##
==========================================
- Coverage 95.94% 95.94% -0.01%
==========================================
Files 1077 1079 +2
Lines 33021 33149 +128
==========================================
+ Hits 31682 31804 +122
- Misses 1339 1345 +6 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with one extremely minor comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also approved with 2 minor comments.
api/app/routers.py
Outdated
logger = logging.getLogger(__name__) | ||
CONNECTION_CHECK_CACHE_TTL = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: imo these should be separated by a (blank) new line whitespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks fine my way, but I don't mind doing it your way either so I've updated it.
api/app/routers.py
Outdated
class ImproperlyConfiguredError(RuntimeError): | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love that this exception isn't defined in an exceptions
module, but perhaps it's overkill if app.exceptions
doesn't already exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to move it there.
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingdocs/
if required so people know about the feature!Changes
Support fallbacking from replicas that go down and support multiple replica options. See this ticket's comment for full details.
How did you test this code?
I hard coded a single default database. Extensive testing by hand to make sure the caches and connections worked, then later I introduced one test that verifies the connection works against the default database and in addition to that I added in several tests against the router logic with mocked logic to mimic database connection issues.