Repository navigation
fix(sync-service): Move connection opts discovery into external process - #3118
magnetised merged 7 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## magnetised/separate-db-pools #3118 +/- ##
================================================================
- Coverage 77.31% 77.19% -0.12%
================================================================
Files 169 170 +1
Lines 9059 9074 +15
Branches 287 284 -3
================================================================
+ Hits 7004 7005 +1
- Misses 2053 2067 +14
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6af8834 to
b9da18a
Compare
|
Shall we expedite testing this in staging Cloud. @msfstef we can check what users have pooled connection enabled and test with supabase to understand if this would work with the users that have pooled connections |
msfstef
left a comment
msfstef
left a comment
There was a problem hiding this comment.
Very nice!
My main worry is why are we not starting the connection resolver as a process in the supervisor before the connection manager, so we don't have to start link and kill it every time?
We could even hibernate it if we're worried about its usage, but I'm not that worried about it personally, I would prefer it in a supervisor rather than start link and unlink and killing it etc
alco
left a comment
alco
left a comment
There was a problem hiding this comment.
Looks good. There's some confusion about shared_connection_options in the new code, I don't think we need them anymore.
It'd be nice to have an actual test for exercising IPv6 fallback working in a real connection. Why can't we have that tested here, since we're already running real Postgres for tests? Is the DNS request for AAAA on localhost going to resolve to a valid IPv6 address?
| if in_connection_opts == Keyword.fetch!(in_replication_opts, :connection_opts) do | ||
| populate_connection_opts(in_connection_opts) | ||
| end | ||
| shared_connection_opts = Keyword.fetch!(in_replication_opts, :connection_opts) |
There was a problem hiding this comment.
This completely changes the logic below. shared_connection_opts can no longer be nil so there's no point in running is_nil(shared_connection_opts) checks below.
And I don't see a reason to keep shared_connection_opts if there's now a separate validate() call for every type of connection opts. So even if connection_opts == replication_opts[:connection_opts], we'll still run separate checks for each because of different type arguments passed into validate().
There was a problem hiding this comment.
yeah, again i didn't delve too deeply into the shared_connection_opts -- just did a superficial pass to remove the obviously unnecessary calls
| # connection errors | ||
| Process.flag(:trap_exit, true) | ||
|
|
||
| Process.set_label({:connection_discovery, opts[:stack_id]}) |
There was a problem hiding this comment.
Should be :connection_resolver.
A comment from 10000 feet high: please make sure the exists don't get to Sentry. |
Yeah, in the interests of speed (on the clock with this one) I r tried not to go down too many rabbit holes.
|
|
@msfstef have restructured as a permanent sibling process of the connection manager as you suggest @alco let's push some of the cleanup around shared opts to another pr -- don't want to get into the weeds here there is a reliable failure of one of the integration tests looking at behaviour when there are insufficient connections available. must be something to do with the splitting the connections between the two pools interfering with the restart mechanism somehow. am investigating... |
Yeah that seems to be failing on the pool PR (and the failure translates to this one) - the test manually fixes the number of available connections on postgres so that the pool fails to start expecting: I assume you need to match to a different log now that you have named the pools |
6f17e00 to
480d9c4
Compare
0928c35 to
d847551
Compare
msfstef
left a comment
msfstef
left a comment
There was a problem hiding this comment.
Very nice!
I think even with the minimal latency overhead the clarity of having a separate validation step is invaluable. I think it'll help us break up the conn man in further steps
Probably needs rebasing to fix the example TS tests failing
…ternal process Fixes #3099
480d9c4 to
34bfcc7
Compare
d847551 to
67b3935
Compare
msfstef
left a comment
msfstef
left a comment
There was a problem hiding this comment.
Looks good!
I think we could still share conn opts if possible but with a simpler method but happy to keep them nice and cleanly separate at the cost of minimal latency.
Integration test failing (and it'd be good to add a regression unit test that ensures pooled conn url is used) but other than that good on my side!
| connection_opts: connection_opts, | ||
| replication_opts: replication_opts | ||
| } | ||
| %{state | connection_opts: connection_opts, replication_opts: replication_opts} |
There was a problem hiding this comment.
given that all unit tests missed the fact that we were not using a pooled connection string it would be good if we can have some sort of manager_test that checks that the pooled connection is actually used if provided
There was a problem hiding this comment.
I agree. Let me write it up as a follow-up issue.
There was a problem hiding this comment.
that is a really good point
Fixes #3099