ReplicationMode -> ConsistencyMode+ReplicationFactor #750
No reviewers
Labels
No labels
action
check-aws
action
discussion-needed
action
for-external-contributors
action
for-newcomers
action
more-info-needed
action
need-funding
action
triage-required
kind
correctness
kind
ideas
kind
improvement
kind
performance
kind
testing
kind
usability
kind
wrong-behavior
prio
critical
prio
low
scope
admin-api
scope
background-healing
scope
build
scope
documentation
scope
k8s
scope
layout
scope
metadata
scope
ops
scope
rpc
scope
s3-api
scope
security
scope
telemetry
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: Deuxfleurs/garage#750
Loading…
Reference in a new issue
No description provided.
Delete branch "yuka/garage:split-consistency-mode"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Split out from #748 to make it easier to review
017dcfe703
to386476bbc6
Some minor things to fix here and there.
IMO the biggest thing to worry about is that this changes the format of the config file. See my comment on the section in config.rs, I think we should support users that continue using
replication_mode
, or at least have a safeguard that prevents them from inadvertently changing the replication factor of their cluster. Also, the reference documentation page on the config options has to be updated.@ -141,0 +144,4 @@
}
pub fn write_quorum(self: &Arc<Self>) -> usize {
self.replication_factor.read_quorum(self.consistency_mode)
Did you mean write_quorum instead of read_quorum?
@ -20,1 +18,3 @@
_ => None,
impl ConsistencyMode {
pub fn parse(s: &str) -> Option<Self> {
serde_json::from_value(serde_json::Value::String(s.to_string())).ok()
Does this work for values written in lowercase? Because the enum variants start with a capital letter.
@ -21,4 +34,4 @@
}
}
pub fn control_write_max_faults(&self) -> usize {
We don't need this function anymore, right?
@ -62,0 +56,4 @@
match consistency_mode {
ConsistencyMode::Dangerous => 1,
ConsistencyMode::Degraded | ConsistencyMode::Consistent => {
(self.replication_factor() + 1) - self.read_quorum(consistency_mode)
In degraded mode, we will have write_quorum = replication_mode, but that doesn't seem right?
Can be fixed by:
@ -63,2 +62,4 @@
}
}
impl Serialize for ReplicationFactor {
The same can be achieved with
#[derive(Serialize, Deserialize)]
+#[serde(transparent)]
@ -46,3 +39,1 @@
}
fn max_write_errors(&self) -> usize {
self.replication_factor - self.write_quorum
self.layout_manager.read_quorum()
Did you mean write_quorum instead of read_quorum ?
@ -37,2 +35,3 @@
/// - 3 is the recommended and supported setting.
// (we can add more aliases for this later)
pub replication_mode: String,
pub replication_factor: usize,
We probably want a migration path, or at least a safeguard, for people that don't update their config files and continue using the old
replication_mode
variable.Option 1, migration path : if replication_mode is set, check that replication_factor and consistency_mode are not set, and populate them from the legacy replication_mode value.
Option 2, safeguard : if replication_mode is set, throw an error when initializing Garage
@ -39,0 +40,4 @@
/// - Degraded -> Disable read quorum
/// - Dangerous -> Disable read and write quorum
#[serde(default)]
pub consistency_mode: String,
Is the default value (empty string) parsed the same as "consistent" ?
Maybe we can move the ConsistencyMode enum here and use it directly in the config struct
386476bbc6
to537bd5bb97
My original intention was for the config option to be case-sensitive, uppercase ("Degraded", "Dangerous"), and the default when no value was specified should be "Consistent". I broke the default while rebasing/splitting the commit.
We can easily change it to case-sensitive lowercase. For case-insensitive we'd have to have a custom mapping or add a new dependency (serde-aux).
Also I don't have a strong opinion whether this should live in the config module or in its own module. So far I saw other enums (like ReplicationMode before) placed outside of the config module, so I followed that.
537bd5bb97
to8f1f87e2b1
8f1f87e2b1
to56b793b959
I have changed the consistency mode setting to lower-case (still case-sensitive), restored the behavior for the default consistency mode, and added a migration path.
56b793b959
to1fb70df026
1fb70df026
to5cd9e7e36b
5cd9e7e36b
todf469ed97a
df469ed97a
to975bcc9cd0
975bcc9cd0
toe4eab91c5a
LGTM, there is just a conflict in one of the documentation files
@ -9,3 +9,3 @@
```toml
replication_mode = "3"
replication_factor = 3
For completeness we should include
consistency_mode = "consistent"
here (we want an example for all configuration lines)Done
@ -161,4 +153,3 @@
- `3`: data stored on Garage will be stored on three different nodes, if
possible each in a different zones. Garage tolerates two node failure, or
several node failures but in no more than two zones (in a deployment with at
Add a comment saying that arbitrarily large replication factors are now supported?
Added a note about replication factors higher than 3 in the listing
e4eab91c5a
to7acd75dc26
7acd75dc26
to9c6e1bf933
9c6e1bf933
to060f83edf1
ReplicationMode -> ConsistencyMode+ReplicationFactorto WIP: ReplicationMode -> ConsistencyMode+ReplicationFactor060f83edf1
to3670f61779
3670f61779
to5a12279dc1
5a12279dc1
toce042e48d4
ce042e48d4
to6bd595e3c5
6bd595e3c5
to6af4a9d4b0
WIP: ReplicationMode -> ConsistencyMode+ReplicationFactorto ReplicationMode -> ConsistencyMode+ReplicationFactorI think changing all of the config files in all of the testing scripts to use
replication_factor
instead of the oldreplication_mode
config param is going to cause issues, for example:in the Jepsen tests, if we want to run them on older Garage versions that still only have the
replication_mode
configuration variable (I think we want to keep that possibility as much as possible)in the upgrade test (
script/test-upgrade.sh
), where we are testing an upgrade path from v0.8.x to v0.9 / v0.10.In the upgrade test we have some logic to differenciate between Garage versions (0.8 vs 0.9) because some CLI commands have changed (in
dev-configure.sh
anddev-bucket.sh
). For the config file syntax change, either we add some similar logic, or we could just keep the old syntax withreplication_mode = "3"
for those tests.I've launched a full CI run that should do the upgrade test, which might fail because of this (we'll see)
6af4a9d4b0
toc1769bbe69
Thanks !