leekeiabstraction commented on code in PR #624:
URL: https://github.com/apache/fluss-rust/pull/624#discussion_r3415966361
##########
crates/fluss-test-cluster/src/lib.rs:
##########
@@ -165,21 +218,32 @@ impl FlussTestingClusterBuilder {
}
}
- fn all_containers_exist(&self) -> bool {
- self.container_names().iter().all(|name| {
- std::process::Command::new("docker")
- .args(["ps", "-q", "--filter", &format!("name=^{}$", name)])
- .output()
- .map(|o| !String::from_utf8_lossy(&o.stdout).trim().is_empty())
- .unwrap_or(false)
- })
+ async fn all_containers_exist(&self) -> bool {
+ let Some(docker) = docker_client().await else {
+ return false;
+ };
+ for name in self.container_names() {
+ // Anchored exact-name match; `all(false)` = running only, so a
stopped
+ // leftover counts as absent and gets recreated.
+ let mut filters = HashMap::new();
+ filters.insert("name".to_string(), vec![format!("^{name}$")]);
+ let options = ListContainersOptionsBuilder::default()
+ .all(false)
+ .filters(&filters)
+ .build();
+ match docker.list_containers(Some(options)).await {
+ Ok(list) if !list.is_empty() => continue,
+ _ => return false,
+ }
+ }
Review Comment:
nit: Seems like we're making a list_containers per-container. Anyway to make
this more efficient?
##########
crates/fluss-test-cluster/src/lib.rs:
##########
@@ -493,22 +560,47 @@ impl FlussTestingCluster {
}
pub fn stop_cluster(name: &str) {
- let prefixes = [
- format!("zookeeper-{}", name),
- format!("coordinator-server-{}", name),
- format!("tablet-server-{}-", name),
- ];
- for prefix in &prefixes {
- if let Ok(output) = std::process::Command::new("docker")
- .args(["ps", "-aq", "--filter", &format!("name={}", prefix)])
- .output()
- {
- let ids = String::from_utf8_lossy(&output.stdout);
- for id in ids.split_whitespace() {
- let _ = std::process::Command::new("docker")
- .args(["rm", "-f", id])
- .output();
- }
+ let name = name.to_string();
+ run_blocking(async move { stop_cluster_async(&name).await });
+}
+
+/// Force-removes every container of cluster `name` (matched by name prefix)
on the
+/// testcontainers daemon — the same daemon `build_detached` started them on.
+async fn stop_cluster_async(name: &str) {
+ let Some(docker) = docker_client().await else {
+ return;
+ };
+
+ // Multiple values for the `name` filter are OR'd by the daemon; these
prefixes
+ // cover zookeeper, coordinator, and any number of tablet servers.
+ let mut filters = HashMap::new();
+ filters.insert(
+ "name".to_string(),
+ vec![
+ format!("zookeeper-{name}"),
+ format!("coordinator-server-{name}"),
+ format!("tablet-server-{name}-"),
+ ],
+ );
+ let options = ListContainersOptionsBuilder::default()
+ .all(true)
+ .filters(&filters)
+ .build();
+
+ let containers = match docker.list_containers(Some(options)).await {
+ Ok(containers) => containers,
+ Err(e) => {
+ eprintln!("warning: failed to list cluster containers: {e}");
+ return;
+ }
+ };
Review Comment:
nit: This looks similar to parts of `all_containers_exist`, can these be
refactored?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
