Skip to content

Commit

Permalink
Remove unused async from storage health check (#31126)
Browse files Browse the repository at this point in the history
No functional changes, removes unused async from the HealthOperator
trait.

### Checklist

- [ ] This PR has adequate test coverage / QA involvement has been duly
considered. ([trigger-ci for additional test/nightly
runs](https://trigger-ci.dev.materialize.com/))
- [ ] This PR has an associated up-to-date [design
doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md),
is a design doc
([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)),
or is sufficiently small to not require a design.
  <!-- Reference the design in the description. -->
- [ ] If this PR evolves [an existing `$T ⇔ Proto$T`
mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md)
(possibly in a backwards-incompatible way), then it is tagged with a
`T-proto` label.
- [ ] If this PR will require changes to cloud orchestration or tests,
there is a companion cloud PR to account for those changes that is
tagged with the release-blocker label
([example](MaterializeInc/cloud#5021)).
<!-- Ask in #team-cloud on Slack if you need help preparing the cloud
PR. -->
- [ ] If this PR includes major [user-facing behavior
changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note),
I have pinged the relevant PM to schedule a changelog post.

Signed-off-by: Moritz Hoffmann <mh@materialize.com>
antiguru authored Jan 21, 2025
1 parent 925f577 commit 098fc51
Showing 1 changed file with 25 additions and 36 deletions.
61 changes: 25 additions & 36 deletions src/storage/src/healthcheck.rs
Original file line number Diff line number Diff line change
@@ -232,10 +232,9 @@ impl HealthState {
/// A trait that lets a user configure the `health_operator` with custom
/// behavior. This is mostly useful for testing, and the [`DefaultWriter`]
/// should be the correct implementation for everyone.
#[async_trait::async_trait(?Send)]
pub trait HealthOperator {
/// Record a new status.
async fn record_new_status(
fn record_new_status(
&self,
collection_id: GlobalId,
ts: DateTime<Utc>,
@@ -249,7 +248,7 @@ pub trait HealthOperator {
// some use of persist. For now we just leave it and ignore it in tests.
write_namespaced_map: bool,
);
async fn send_halt(&self, id: GlobalId, error: Option<(StatusNamespace, HealthStatusUpdate)>);
fn send_halt(&self, id: GlobalId, error: Option<(StatusNamespace, HealthStatusUpdate)>);

/// Optionally override the chosen worker index. Default is semi-random.
/// Only useful for tests.
@@ -264,9 +263,8 @@ pub struct DefaultWriter {
pub updates: Rc<RefCell<Vec<StatusUpdate>>>,
}

#[async_trait::async_trait(?Send)]
impl HealthOperator for DefaultWriter {
async fn record_new_status(
fn record_new_status(
&self,
collection_id: GlobalId,
ts: DateTime<Utc>,
@@ -293,7 +291,7 @@ impl HealthOperator for DefaultWriter {
});
}

async fn send_halt(&self, id: GlobalId, error: Option<(StatusNamespace, HealthStatusUpdate)>) {
fn send_halt(&self, id: GlobalId, error: Option<(StatusNamespace, HealthStatusUpdate)>) {
self.command_tx
.borrow_mut()
.broadcast(InternalStorageCommand::SuspendAndRestart {
@@ -389,17 +387,15 @@ where
if mark_starting.contains(id) {
let status = OverallStatus::Starting;
let timestamp = mz_ore::now::to_datetime(now());
health_operator_impl
.record_new_status(
*id,
timestamp,
(&status).into(),
status.error(),
&status.hints(),
status.errors().unwrap_or(&BTreeMap::new()),
write_namespaced_map,
)
.await;
health_operator_impl.record_new_status(
*id,
timestamp,
(&status).into(),
status.error(),
&status.hints(),
status.errors().unwrap_or(&BTreeMap::new()),
write_namespaced_map,
);

state.last_reported_status = Some(status);
}
@@ -466,17 +462,15 @@ where
);

let timestamp = mz_ore::now::to_datetime(now());
health_operator_impl
.record_new_status(
id,
timestamp,
(&new_status).into(),
new_status.error(),
&new_status.hints(),
new_status.errors().unwrap_or(&BTreeMap::new()),
write_namespaced_map,
)
.await;
health_operator_impl.record_new_status(
id,
timestamp,
(&new_status).into(),
new_status.error(),
&new_status.hints(),
new_status.errors().unwrap_or(&BTreeMap::new()),
write_namespaced_map,
);

*last_reported_status = Some(new_status.clone());
}
@@ -507,7 +501,7 @@ where
halt_with, suspend_and_restart_delay
);
tokio::time::sleep(suspend_and_restart_delay).await;
health_operator_impl.send_halt(id, halt_with).await;
health_operator_impl.send_halt(id, halt_with);
}
}
}
@@ -1014,9 +1008,8 @@ mod tests {
input_mapping: BTreeMap<GlobalId, usize>,
}

#[async_trait::async_trait(?Send)]
impl HealthOperator for TestWriter {
async fn record_new_status(
fn record_new_status(
&self,
collection_id: GlobalId,
_ts: DateTime<Utc>,
@@ -1048,11 +1041,7 @@ mod tests {
});
}

async fn send_halt(
&self,
_id: GlobalId,
_error: Option<(StatusNamespace, HealthStatusUpdate)>,
) {
fn send_halt(&self, _id: GlobalId, _error: Option<(StatusNamespace, HealthStatusUpdate)>) {
// Not yet unit-tested
unimplemented!()
}

0 comments on commit 098fc51

Please sign in to comment.