Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify matches #1490

Merged
merged 1 commit into from
Oct 4, 2024
Merged

Simplify matches #1490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 6 additions & 13 deletions crucible-client-types/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,19 +41,12 @@ pub enum VolumeConstructionRequest {
impl VolumeConstructionRequest {
pub fn targets(&self) -> Vec<SocketAddr> {
match self {
VolumeConstructionRequest::Volume {
id: _,
block_size: _,
sub_volumes,
read_only_parent: _,
} => sub_volumes.iter().flat_map(|s| s.targets()).collect(),
VolumeConstructionRequest::Region {
block_size: _,
blocks_per_extent: _,
extent_count: _,
opts,
gen: _,
} => opts.target.clone(),
VolumeConstructionRequest::Volume { sub_volumes, .. } => {
sub_volumes.iter().flat_map(|s| s.targets()).collect()
}
VolumeConstructionRequest::Region { opts, .. } => {
opts.target.clone()
}
_ => vec![],
}
}
Expand Down
13 changes: 2 additions & 11 deletions downstairs/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1316,9 +1316,9 @@ impl ActiveConnection {
Message::ExtentFlush {
repair_id,
extent_id,
client_id: _,
flush_number,
gen_number,
..
} => {
let msg = {
debug!(
Expand Down Expand Up @@ -3680,16 +3680,7 @@ mod test {
assert!(work.completed.is_complete(dep));
}

matches!(
job,
IOop::Flush {
dependencies: _,
flush_number: _,
gen_number: _,
snapshot_details: _,
extent_limit: _,
}
)
matches!(job, IOop::Flush { .. })
};

if is_flush {
Expand Down
88 changes: 45 additions & 43 deletions protocol/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,53 +592,55 @@ impl Message {
/// Return true if this message contains an Error result
pub fn err(&self) -> Option<&CrucibleError> {
match self {
Message::HereIAm { .. } => None,
Message::YesItsMe { .. } => None,
Message::VersionMismatch { .. } => None,
Message::ReadOnlyMismatch { .. } => None,
Message::EncryptedMismatch { .. } => None,
Message::PromoteToActive { .. } => None,
Message::YouAreNowActive { .. } => None,
Message::YouAreNoLongerActive { .. } => None,
Message::UuidMismatch { .. } => None,
Message::Ruok { .. } => None,
Message::Imok { .. } => None,
Message::ExtentClose { .. } => None,
Message::ExtentReopen { .. } => None,
Message::ExtentFlush { .. } => None,
Message::ExtentRepair { .. } => None,
Message::RepairAckId { .. } => None,
Message::RegionInfoPlease { .. } => None,
Message::RegionInfo { .. } => None,
Message::ExtentVersionsPlease { .. } => None,
Message::ExtentVersions { .. } => None,
Message::LastFlush { .. } => None,
Message::LastFlushAck { .. } => None,
Message::Write { .. } => None,
Message::ExtentLiveClose { .. } => None,
Message::ExtentLiveFlushClose { .. } => None,
Message::ExtentLiveRepair { .. } => None,
Message::ExtentLiveReopen { .. } => None,
Message::ExtentLiveNoOp { .. } => None,
Message::Flush { .. } => None,
Message::ReadRequest { .. } => None,
Message::WriteUnwritten { .. } => None,
Message::Unknown(..) => None,

Message::ExtentError { error, .. } => Some(error),
Message::ErrorReport { error, .. } => Some(error),
Message::HereIAm { .. }
| Message::YesItsMe { .. }
| Message::VersionMismatch { .. }
| Message::ReadOnlyMismatch { .. }
| Message::EncryptedMismatch { .. }
| Message::PromoteToActive { .. }
| Message::YouAreNowActive { .. }
| Message::YouAreNoLongerActive { .. }
| Message::UuidMismatch { .. }
| Message::Ruok { .. }
| Message::Imok { .. }
| Message::ExtentClose { .. }
| Message::ExtentReopen { .. }
| Message::ExtentFlush { .. }
| Message::ExtentRepair { .. }
| Message::RepairAckId { .. }
| Message::RegionInfoPlease { .. }
| Message::RegionInfo { .. }
| Message::ExtentVersionsPlease { .. }
| Message::ExtentVersions { .. }
| Message::LastFlush { .. }
| Message::LastFlushAck { .. }
| Message::Write { .. }
| Message::ExtentLiveClose { .. }
| Message::ExtentLiveFlushClose { .. }
| Message::ExtentLiveRepair { .. }
| Message::ExtentLiveReopen { .. }
| Message::ExtentLiveNoOp { .. }
| Message::Flush { .. }
| Message::ReadRequest { .. }
| Message::WriteUnwritten { .. }
| Message::Unknown(..) => None,

Message::ExtentError { error, .. }
| Message::ErrorReport { error, .. } => Some(error),

Message::ExtentLiveCloseAck { result, .. } => result.as_ref().err(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this makes it any simpler, but I also don't really care one way or the other. I do think it's all part of your master plan to touch every line in Crucible :)

However, you should be able to put Message::ExtentLiveCloseAck with the rest just below, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this makes it any simpler, but I also don't really care one way or the other. I do think it's all part of your master plan to touch every line in Crucible :)

🤫

However, you should be able to put Message::ExtentLiveCloseAck with the rest just below, right?

They're actually different types Result<(), CrucibleError> versus Result<(u64, u64, bool), CrucibleError>, so you can't match on them simultaneously (even though we're only using the error).

Message::ExtentLiveRepairAckId { result, .. } => {

Message::ExtentLiveRepairAckId { result, .. }
| Message::ExtentLiveAckId { result, .. }
| Message::WriteAck { result, .. }
| Message::FlushAck { result, .. }
| Message::WriteUnwrittenAck { result, .. } => {
result.as_ref().err()
}
Message::ExtentLiveAckId { result, .. } => result.as_ref().err(),
Message::WriteAck { result, .. } => result.as_ref().err(),
Message::FlushAck { result, .. } => result.as_ref().err(),

Message::ReadResponse { header, .. } => {
header.blocks.as_ref().err()
}
Message::WriteUnwrittenAck { result, .. } => result.as_ref().err(),
}
}
}
Expand All @@ -658,7 +660,7 @@ impl std::fmt::Display for Message {
start,
..
},
data: _,
..
} => f
.debug_struct("Message::Write")
.field("upstairs_id", &upstairs_id)
Expand All @@ -678,7 +680,7 @@ impl std::fmt::Display for Message {
start,
..
},
data: _,
..
} => f
.debug_struct("Message::WriteUnwritten")
.field("upstairs_id", &upstairs_id)
Expand All @@ -696,7 +698,7 @@ impl std::fmt::Display for Message {
job_id,
blocks,
},
data: _,
..
} => f
.debug_struct("Message::ReadResponse")
.field("upstairs_id", &upstairs_id)
Expand Down
10 changes: 3 additions & 7 deletions upstairs/src/downstairs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,10 +1077,7 @@ impl Downstairs {
repair.state = LiveRepairState::Noop { noop_id, reopen_id };
self.create_and_enqueue_noop_io(gw, vec![repair_id], noop_id);
}
LiveRepairState::Noop {
noop_id: _,
reopen_id,
} => {
LiveRepairState::Noop { reopen_id, .. } => {
info!(
self.log,
"RE:{} Wait for result from reopen command {}",
Expand Down Expand Up @@ -7975,7 +7972,7 @@ pub(crate) mod test {

println!("repair op: {:?}", repair_op);
match repair_op {
IOop::ExtentLiveNoOp { dependencies: _ } => {}
IOop::ExtentLiveNoOp { .. } => {}
x => {
panic!("Incorrect work type returned: {:?}", x);
}
Expand Down Expand Up @@ -8009,11 +8006,10 @@ pub(crate) mod test {

match repair_op {
IOop::ExtentLiveRepair {
dependencies: _,
extent,
source_downstairs,
source_repair_address: _,
repair_downstairs,
..
} => {
assert_eq!(extent, ExtentId(0));
assert_eq!(source_downstairs, source);
Expand Down
8 changes: 1 addition & 7 deletions upstairs/src/dummy_downstairs_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,13 +131,7 @@ impl DownstairsHandle {
pub async fn negotiate_start(&mut self) {
let packet = self.recv().await.unwrap();
if let Message::HereIAm {
version,
upstairs_id: _,
session_id: _,
gen: _,
read_only,
encrypted: _,
alternate_versions: _,
version, read_only, ..
} = &packet
{
info!(
Expand Down
16 changes: 3 additions & 13 deletions upstairs/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1248,32 +1248,22 @@ impl IOop {
let job_type = "WriteU".to_string();
(job_type, blocks.len(), dependencies.clone())
}
IOop::Flush {
dependencies,
flush_number: _flush_number,
gen_number: _gen_number,
snapshot_details: _,
extent_limit: _,
} => {
IOop::Flush { dependencies, .. } => {
let job_type = "Flush".to_string();
(job_type, 0, dependencies.clone())
}
IOop::ExtentFlushClose {
dependencies,
extent,
flush_number: _,
gen_number: _,
repair_downstairs: _,
..
} => {
let job_type = "FClose".to_string();
(job_type, extent.0 as usize, dependencies.clone())
}
IOop::ExtentLiveRepair {
dependencies,
extent,
source_downstairs: _,
source_repair_address: _,
repair_downstairs: _,
..
} => {
let job_type = "Repair".to_string();
(job_type, extent.0 as usize, dependencies.clone())
Expand Down
5 changes: 1 addition & 4 deletions upstairs/src/volume.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1335,10 +1335,7 @@ impl Volume {
// Beyond this point zero or one target changes where found

match &compare_result {
CompareResult::Volume {
sub_compares,
read_only_parent_compare: _,
} => {
CompareResult::Volume { sub_compares, .. } => {
// Walk the cases. As some deltas are okay for a read only
// parent that are not for a sub_volume, we need to look at all
// the possible permutations Nexus supports.
Expand Down