Skip to content

Conversation

mkeeter
Copy link
Contributor

@mkeeter mkeeter commented Jun 25, 2025

(staged on top of #1731)

This is a small cleanup: previously, collate called collate_inner, then asserted that nothing changed if collate_inner returned an error.

However, it turns out that collate_inner can be split into two independent pieces:

  • A fallible function taking &self which checks whether the state is valid and can fail (check_region_metadata)
  • An infallible function which begins reconciliation, given state from the checks (start_reconciliation)

This division makes "assert nothing changed" unnecessary: the checking function can't modify &self, and starting reconciliation can't fail.

@mkeeter mkeeter requested review from jmpesp and leftwo June 25, 2025 14:45
Copy link
Contributor

@leftwo leftwo left a comment

Choose a reason for hiding this comment

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

I was on the fence about the name of start_reconciliation() as we are just checking to see if we need to do reconciliation. But, really, that is part of the reconciliation process, to first determine if additional work is what we need to do, so I think I'm fine with the name.

Base automatically changed from mkeeter/better-connection-delay to main June 30, 2025 13:10
@mkeeter mkeeter force-pushed the mkeeter/collate-cleanup branch from 21a03f8 to 1b91f6c Compare June 30, 2025 13:11
@mkeeter mkeeter merged commit dd48b6a into main Jun 30, 2025
17 checks passed
@mkeeter mkeeter deleted the mkeeter/collate-cleanup branch June 30, 2025 13:51
jmpesp added a commit to oxidecomputer/omicron that referenced this pull request Aug 11, 2025
Pick up the following propolis PRs:

- Bump crucible rev to latest (oxidecomputer/propolis#922)
- Added block_size for file backends in propolis_server (workers is optional) (oxidecomputer/propolis#917)

Pick up the following crucible PRs:

- Snapshots existing already are ok! (oxidecomputer/crucible#1759)
- Less verbose logging (oxidecomputer/crucible#1756)
- Remove unused `Vec<JoinHandle>` (oxidecomputer/crucible#1754)
- Split "check reconciliation state" from "start reconciliation" (oxidecomputer/crucible#1732)
- Improve `ClientIoTask` start logic (oxidecomputer/crucible#1731)
- Use data-bearing enum variants pattern in negotiation (oxidecomputer/crucible#1727)
- Make Downstairs stoppable (oxidecomputer/crucible#1730)
- Don't log every region's metadata (oxidecomputer/crucible#1729)
- Compute reconciliation from `ClientMap` instead of three clients (oxidecomputer/crucible#1726)
- Make Offline -> Faulted transition happen without reconnecting (oxidecomputer/crucible#1725)
- Remove `WaitActive` state during negotiation (oxidecomputer/crucible#1722)
- Add explicit `UpstairsState::Disabled` (oxidecomputer/crucible#1721)
- Print version on startup for pantry and agent (oxidecomputer/crucible#1723)
- Update test mem to also show physical space used by regions. (oxidecomputer/crucible#1724)
- Add AllStopped command and endpoint for downstairs status (oxidecomputer/crucible#1718)
- Update tests to honor REGION_SETS env if provided. (oxidecomputer/crucible#1720)
- Fix panic in `set_active_request` when client is in Stopping(Replacing) state (oxidecomputer/crucible#1717)
- DTrace updates (oxidecomputer/crucible#1715)
jmpesp added a commit to oxidecomputer/omicron that referenced this pull request Aug 11, 2025
Pick up the following propolis PRs:

- Bump crucible rev to latest (oxidecomputer/propolis#922)
- Added block_size for file backends in propolis_server (workers is optional) (oxidecomputer/propolis#917)

Pick up the following crucible PRs:

- Snapshots existing already are ok! (oxidecomputer/crucible#1759)
- Less verbose logging (oxidecomputer/crucible#1756)
- Remove unused `Vec<JoinHandle>` (oxidecomputer/crucible#1754)
- Split "check reconciliation state" from "start reconciliation" (oxidecomputer/crucible#1732)
- Improve `ClientIoTask` start logic (oxidecomputer/crucible#1731)
- Use data-bearing enum variants pattern in negotiation (oxidecomputer/crucible#1727)
- Make Downstairs stoppable (oxidecomputer/crucible#1730)
- Don't log every region's metadata (oxidecomputer/crucible#1729)
- Compute reconciliation from `ClientMap` instead of three clients (oxidecomputer/crucible#1726)
- Make Offline -> Faulted transition happen without reconnecting (oxidecomputer/crucible#1725)
- Remove `WaitActive` state during negotiation (oxidecomputer/crucible#1722)
- Add explicit `UpstairsState::Disabled` (oxidecomputer/crucible#1721)
- Print version on startup for pantry and agent (oxidecomputer/crucible#1723)
- Update test mem to also show physical space used by regions. (oxidecomputer/crucible#1724)
- Add AllStopped command and endpoint for downstairs status (oxidecomputer/crucible#1718)
- Update tests to honor REGION_SETS env if provided. (oxidecomputer/crucible#1720)
- Fix panic in `set_active_request` when client is in Stopping(Replacing) state (oxidecomputer/crucible#1717)
- DTrace updates (oxidecomputer/crucible#1715)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants