[DPE-5827] Synchronous node count configuration #738
Conversation
dragomirp
commented
Jan 22, 2025
•
dragomirp
commented
- Add synchronous node configuration
- Remove on storage remove step down hook
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dpe-5827-all-sync #738 +/- ##
=====================================================
- Coverage 71.92% 71.85% -0.08%
=====================================================
Files 15 15
Lines 3477 3468 -9
Branches 534 532 -2
=====================================================
- Hits 2501 2492 -9
- Misses 845 846 +1
+ Partials 131 130 -1 ☔ View full report in Codecov by Sentry. |
| @@ -500,52 +497,6 @@ def _on_peer_relation_departed(self, event: RelationDepartedEvent) -> None: | |||
| # Update the sync-standby endpoint in the async replication data. | |||
| self.async_replication.update_async_replication_data() | |||
|
|
|||
| def _on_pgdata_storage_detaching(self, _) -> None: | |||
There was a problem hiding this comment.
Step down seems to cause stuck tests on juju 2 (db and replication). Since now we have means to reinitialise RAFT when stuck, we should be able to remove this.
tests/integration/test_db.py
Outdated
There was a problem hiding this comment.
Switched some comments into logs for debugging.
| ["primaries", "replicas"], | ||
| ["sync_standbys", "replicas"], | ||
| ["primaries", "sync_standbys"], | ||
| ["sync_standbys", "sync_standbys"], |
There was a problem hiding this comment.
No more replicas by default.
There was a problem hiding this comment.
We should still test non-default case, IMHO.
It will exist in the wild (due to the performance requirements).
There was a problem hiding this comment.
I also think this is important.
There was a problem hiding this comment.
Adding a separate test for async.
| async with ops_test.fast_forward(): | ||
| await ops_test.model.wait_for_idle(apps=[app], status="active") |
There was a problem hiding this comment.
Changing back to what is in main.
dragomirp
requested review from
taurus-forever,
marceloneppel and
lucasgameiroborges
There was a problem hiding this comment.
LGTM, sent ping to @7annaba3l and @delgod due to UI changes (new Juju config option).
| synchronous_node_count: | ||
| description: | | ||
| Sets the number of synchronous nodes to be maintained in the cluster. Should be | ||
| either "all", "majority" or a positive integer value. | ||
| type: string | ||
| default: "all" |
There was a problem hiding this comment.
For the history: it matches signed spec.
| ["primaries", "replicas"], | ||
| ["sync_standbys", "replicas"], | ||
| ["primaries", "sync_standbys"], | ||
| ["sync_standbys", "sync_standbys"], |
There was a problem hiding this comment.
We should still test non-default case, IMHO.
It will exist in the wild (due to the performance requirements).
| ["primaries", "replicas"], | ||
| ["sync_standbys", "replicas"], | ||
| ["primaries", "sync_standbys"], | ||
| ["sync_standbys", "sync_standbys"], |
There was a problem hiding this comment.
I also think this is important.
tests/integration/test_db.py
Outdated
