Fix/default to maindb if read replica errs #1221
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #1221 +/- ##
===========================================
- Coverage 78.18% 77.95% -0.24%
===========================================
Files 48 48
Lines 1985 1996 +11
Branches 314 315 +1
===========================================
+ Hits 1552 1556 +4
- Misses 433 440 +7 ☔ View full report in Codecov by Sentry. |
| let found = await this.replicationRequestRepository.findByCid(cid) | ||
| let found | ||
| try { | ||
| found = await this.replicationRequestRepository.findByCid(cid) |
There was a problem hiding this comment.
My preference would be to expect the replica to work if it exists, but default the config to main if there is none defined. I think we should expect it to work if it's defined, but it should be optional.
There was a problem hiding this comment.
In terms of implementation, I'd expect we either set up a RO connection pool, or we just use the main pool and the app still calls replicationRequestRepository for all reads because it doesn't require the writer, without needing to know anything about the actual DB used.
This also avoids doing double reads for things that don't actually exist and potentially getting hung trying to talk to a non-exists RO instance.
There was a problem hiding this comment.
yeah, it all around a better design to make the read replication optional. FYI, currently all our reads do not got to the replica. If the configuration for the read replica is not specified or if any of the config params are missing, we default to the maindb. Will add a commit for this and tag you, let me know if this might fix it or there might be something else I missed
There was a problem hiding this comment.
I like the changes you made, do we still need this fallback? if it fails now, it seems like it's failing for a reason so there's not sure we need to try another database.
There was a problem hiding this comment.
yup, in the case of the read replica not having the request. If sync b/w the db's is slow
There was a problem hiding this comment.
Yeah, what I was trying to say is that if the RR doesn't have the request, it's unlikely the main DB does so there's no reason to ask. If for some reason we're far enough behind on replicating (i.e. minutes), we probably don't want to load the main database more and should investigate. Otherwise, we can tolerate the client retrying again in a few seconds and the RR will respond with data.
| @@ -71,7 +71,7 @@ export async function createDbConnection(dbConfig: Db = config.db): Promise<Knex | |||
|
|
|||
| export async function createReplicaDbConnection( | |||
| replica_db_config: Replicadb = config.replica_db | |||
| ): Promise<Knex> { | |||
| ): Promise<{ connection: Knex; type: string }> { | |||
There was a problem hiding this comment.
@dav1do this change makes the readReplica optional and by introducing a connectionType we have captured the two modes it can work in.
The type is also useful to avoid misleading logs in the connection failure mode giving devs more insight into what is happening in the background
| let found = await this.replicationRequestRepository.findByCid(cid) | ||
| let found | ||
| try { | ||
| found = await this.replicationRequestRepository.findByCid(cid) |
There was a problem hiding this comment.
I like the changes you made, do we still need this fallback? if it fails now, it seems like it's failing for a reason so there's not sure we need to try another database.
[Replace Me With Meaningful Name] - #[Issue]
Description
Include a summary of the change and which issue it addresses in the title of the PR.
Include relevant motivation, context, brief description and impact of the change(s). List follow-up tasks here.
How Has This Been Tested?
Describe the tests that you ran to verify your changes. Provide instructions for reproduction.
Definition of Done
Before submitting this PR, please make sure:
References:
Please list relevant documentation (e.g. tech specs, articles, related work etc.) relevant to this change, and note if the documentation has been updated.