Add two new configuration options for periodic retained WAL size check in scaled down mode #3274
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3274 +/- ##
==========================================
- Coverage 69.83% 67.05% -2.79%
==========================================
Files 181 172 -9
Lines 9777 8872 -905
Branches 336 106 -230
==========================================
- Hits 6828 5949 -879
+ Misses 2947 2922 -25
+ Partials 2 1 -1
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:
|
alco
force-pushed
the
alco/wal-size-check
branch
from
35d985e to
ca3a175
Compare
alco
changed the title
alco
force-pushed
the
alco/wal-size-check
branch
2 times, most recently
from
4730f9c to
fd65b33
Compare
msfstef
left a comment
msfstef
left a comment
There was a problem hiding this comment.
Looks good but I assume this does not work since there's no querying of the wal size (and no tests which I assume is for the above reason?)
|
|
||
| defp query_retained_wal_size(_state) do | ||
| # FIXME: placeholder | ||
| 0 |
There was a problem hiding this comment.
I assume this doesn't work until this is fixed
There was a problem hiding this comment.
Yeah. I'll add the query and extend the integration test in a follow-up PR to be merged into this one.
| else | ||
| try do | ||
| with {:ok, validated_opts} <- ConnectionResolver.validate(state.stack_id, conn_opts) do | ||
| Electric.StackConfig.put(state.stack_id, config_key, validated_opts) |
There was a problem hiding this comment.
I don't love the dynamic configuration on this in the sense that the only reason to have it is for something to use it outside of the connection manager, in which case it doesn't really know when it is ready?
Also if the connection manager crashes, which could be because of a change in the connection configurations, would we not perhaps want to revalidated the connection options passed to us?
There was a problem hiding this comment.
This is a great point. The original implementation of connection option validation was modelled as a uni-directional flow: i.e. once we fall back from SSL to noSSL or from SSL to unencrypted, there was no going back. I've always thought of the revalidation when connection manager restarts as a redundant step. But you're proposing that it is, in fact, an opportunity to redo the validation?
Given that Electric.StackConfig lives under the StackSupervisor, any change in the stack configuration will erase its cached connection options and so there's no problem with having stale validated options.
From my perspective, doing validation once is the goal. We could implement it by running connection resolver for replication and pool connections upfront, before proceeding to initialize connection manager for the first time. Conceptually, this would not be as controversial, I think, but it would be at the cost of increased latency on the first connection setup.
alco
force-pushed
the
alco/scaled-down-stack-event
branch
from
92a696c to
38a1a84
Compare
alco
force-pushed
the
alco/wal-size-check
branch
from
fd65b33 to
bd2bd19
Compare
Closes #3260.
This PR is missing the query for WAL size but the surrounding logic is ready for review.
I'll implement the query in a follow-up PR because I want to build a basic synchronous interface on top of
Postgrex.SimpleConnection.