Copilot commented on code in PR #146:
URL: https://github.com/apache/solr-orbit/pull/146#discussion_r4382996114
##########
solrorbit/builder/solr_provisioner.py:
##########
@@ -436,11 +440,27 @@ def _cluster_config_env_flags(self) -> list:
flags += ["-e", f"SOLR_OPTS={solr_opts}"]
return flags
- def stop(self) -> None:
- """Stop and remove the Solr container."""
- cmd = ["docker", "rm", "-f", self.container_name]
+ def stop(self, log_dest: str = None) -> None:
+ """
+ Stop the Solr container, copy its log directory to *log_dest* (if
given), and remove it.
+
+ Stopping first lets the JVM flush files written on exit, such as the
JFR recording.
+ """
logger.info("Stopping Solr container '%s'", self.container_name)
- result = subprocess.run(cmd, capture_output=True, text=True)
+ result = subprocess.run(["docker", "stop", self.container_name],
capture_output=True, text=True)
+ if result.returncode != 0:
+ logger.warning("docker stop returned non-zero: %s", result.stderr
or result.stdout)
+ elif log_dest:
+ os.makedirs(log_dest, exist_ok=True)
+ # The trailing "/." copies the directory's contents rather than
the directory itself.
+ source = f"{self.container_name}:{self.CONTAINER_LOG_DIR}/."
+ result = subprocess.run(["docker", "cp", source, log_dest],
capture_output=True, text=True)
+ if result.returncode != 0:
+ logger.warning("Could not copy Solr logs from the container:
%s", result.stderr or result.stdout)
+ else:
+ logger.info("Copied Solr logs from the container to %s",
log_dest)
+
+ result = subprocess.run(["docker", "rm", "-f", self.container_name],
capture_output=True, text=True)
Review Comment:
The official Solr images declare `/var/solr` as a volume, so replacing
`--rm` with `docker rm -f` leaves its anonymous volume behind after each
benchmark. Retained indexed data, logs and heap dumps can exhaust the Docker
host's disk across repeated runs. Add `-v` to removal after log export; apply
the same change to stale-container removal in `start()` at line 387.
##########
solrorbit/builder/solr_provisioner.py:
##########
@@ -436,11 +440,27 @@ def _cluster_config_env_flags(self) -> list:
flags += ["-e", f"SOLR_OPTS={solr_opts}"]
return flags
- def stop(self) -> None:
- """Stop and remove the Solr container."""
- cmd = ["docker", "rm", "-f", self.container_name]
+ def stop(self, log_dest: str = None) -> None:
+ """
+ Stop the Solr container, copy its log directory to *log_dest* (if
given), and remove it.
+
+ Stopping first lets the JVM flush files written on exit, such as the
JFR recording.
+ """
logger.info("Stopping Solr container '%s'", self.container_name)
- result = subprocess.run(cmd, capture_output=True, text=True)
+ result = subprocess.run(["docker", "stop", self.container_name],
capture_output=True, text=True)
+ if result.returncode != 0:
+ logger.warning("docker stop returned non-zero: %s", result.stderr
or result.stdout)
+ elif log_dest:
+ os.makedirs(log_dest, exist_ok=True)
Review Comment:
If creating the log destination raises (for example, `PermissionError` or
`ENOSPC`), execution never reaches `docker rm`. Since `--rm` has been removed,
this leaves the container and its filesystem behind; the pipeline only logs the
exception and does not retry cleanup. Put removal in a `finally` block so
filesystem or subprocess exceptions cannot bypass it.
##########
solrorbit/test_run_orchestrator.py:
##########
@@ -607,14 +657,17 @@ def solr_docker(cfg):
cc_instance = _load_cluster_config(cfg)
solr_modules = cfg.opts("solr", "modules", mandatory=False,
default_value="")
- launcher = SolrDockerLauncher(port=port, cluster_config=cc_instance,
solr_modules=solr_modules)
+ jvm_devices = _create_jvm_telemetry_devices(cfg,
container_log_root=SolrDockerLauncher.CONTAINER_LOG_DIR)
+ launcher = SolrDockerLauncher(port=port, cluster_config=cc_instance,
solr_modules=solr_modules,
+ telemetry_devices=jvm_devices)
Review Comment:
Enabling `jit` now passes `-XX:+PrintAssembly` to Docker, but this launcher
still uses its 60-second startup timeout. The reported `solr:9` runs take
93–143 seconds with that flag, so the normal `--pipeline docker --telemetry
jit` command times out and tears down Solr before the benchmark starts. Please
make assembly logging opt-in, or provide a sufficient startup timeout for
JIT-enabled runs, before enabling this path.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
