Copilot commented on code in PR #4710:
URL: https://github.com/apache/solr/pull/4710#discussion_r4190616624
##########
solr/core/src/java/org/apache/solr/cli/SolrProcessManager.java:
##########
@@ -143,13 +153,47 @@ public Collection<SolrProcess> getAllRunning() {
return pidProcessMap.values();
}
- private Optional<Integer> parsePortFromProcess(ProcessHandle ph) {
- Optional<String> portStr =
- arguments(ph).stream()
- .filter(a -> a.contains("-Dsolr.port.listen="))
- .map(s -> s.split("=")[1])
- .findFirst();
- return portStr.isPresent() ? portStr.map(Integer::parseInt) :
Optional.empty();
+ /** Parses the value of the given system property from the process' command
line arguments */
+ private static Optional<String> parseSyspropFromProcess(ProcessHandle ph,
String sysprop) {
+ return arguments(ph).stream()
+ .filter(a -> a.contains("-D" + sysprop + "="))
+ .map(s -> s.split("=", 2)[1])
Review Comment:
On Windows, `arguments()` splits the raw PowerShell `CommandLine` on
whitespace without decoding quotes. A valid JVM option such as
`-Dsolr.host.advertise="myhost.example.com"` therefore leaves quotes in the
extracted host, producing an unusable status URL even though Java received the
correct hostname. Decode Windows argument quoting before extracting properties,
and test quoted host values, including a quoted whole argument.
##########
solr/core/src/java/org/apache/solr/cli/SolrProcessManager.java:
##########
@@ -143,13 +153,47 @@ public Collection<SolrProcess> getAllRunning() {
return pidProcessMap.values();
}
- private Optional<Integer> parsePortFromProcess(ProcessHandle ph) {
- Optional<String> portStr =
- arguments(ph).stream()
- .filter(a -> a.contains("-Dsolr.port.listen="))
- .map(s -> s.split("=")[1])
- .findFirst();
- return portStr.isPresent() ? portStr.map(Integer::parseInt) :
Optional.empty();
+ /** Parses the value of the given system property from the process' command
line arguments */
+ private static Optional<String> parseSyspropFromProcess(ProcessHandle ph,
String sysprop) {
+ return arguments(ph).stream()
+ .filter(a -> a.contains("-D" + sysprop + "="))
+ .map(s -> s.split("=", 2)[1])
+ .findFirst();
Review Comment:
`solr.cmd` appends user `SOLR_OPTS` after generated properties so users can
override them (lines 1113–1130), but `findFirst()` selects the earlier value.
For example, a generated `solr.host.advertise=localhost` followed by a user
hostname override makes status connect to localhost instead of the JVM's
effective advertised host, potentially failing TLS hostname verification.
Select the last matching value to follow JVM property precedence.
##########
solr/core/src/java/org/apache/solr/cli/SolrProcessManager.java:
##########
@@ -143,13 +153,47 @@ public Collection<SolrProcess> getAllRunning() {
return pidProcessMap.values();
}
- private Optional<Integer> parsePortFromProcess(ProcessHandle ph) {
- Optional<String> portStr =
- arguments(ph).stream()
- .filter(a -> a.contains("-Dsolr.port.listen="))
- .map(s -> s.split("=")[1])
- .findFirst();
- return portStr.isPresent() ? portStr.map(Integer::parseInt) :
Optional.empty();
+ /** Parses the value of the given system property from the process' command
line arguments */
+ private static Optional<String> parseSyspropFromProcess(ProcessHandle ph,
String sysprop) {
+ return arguments(ph).stream()
+ .filter(a -> a.contains("-D" + sysprop + "="))
+ .map(s -> s.split("=", 2)[1])
+ .findFirst();
+ }
+
+ /**
+ * Returns the process listening on the given port, if found, waiting up to
{@code maxWaitSecs}
+ * for it to appear. A newly started process may not be visible in the
process table right away,
+ * so the table is re-scanned once a second until the deadline.
+ */
+ public Optional<SolrProcess> waitForProcessOnPort(int port, int maxWaitSecs)
+ throws InterruptedException {
+ Optional<SolrProcess> proc = processForPort(port);
+ TimeOut timeOut = new TimeOut(maxWaitSecs, TimeUnit.SECONDS,
TimeSource.NANO_TIME);
+ while (proc.isEmpty() && !timeOut.hasTimedOut()) {
+ timeOut.sleep(1000);
+ proc = new SolrProcessManager().processForPort(port);
+ }
+ return proc;
+ }
+
+ /**
+ * Resolves the host to use when connecting locally to a Solr process. The
advertised host is
+ * preferred when set, as that is the name the node is reachable by and,
with SSL, the name its
+ * certificate is issued for. Otherwise the bind host is used if it is a
specific non-loopback
+ * address. Wildcard and loopback binds are reachable as {@code localhost}.
IPv6 literals are
+ * bracketed for use in URLs.
+ */
+ static String localConnectHost(Optional<String> advertiseHost,
Optional<String> bindHost) {
+ String host =
+ advertiseHost
+ .map(String::trim)
+ .filter(h -> !h.isEmpty())
+ .orElseGet(() -> bindHost.map(String::trim).orElse(""));
+ return switch (host) {
+ case "", "0.0.0.0", "::", "[::]", "127.0.0.1", "::1", "[::1]",
"localhost" -> "localhost";
+ default -> host.contains(":") && !host.startsWith("[") ? "[" + host +
"]" : host;
+ };
Review Comment:
This normalization also rewrites an explicitly advertised `127.0.0.1` or
`::1` to `localhost`. If the certificate covers that advertised IP but not
`localhost`, startup/status checks fail TLS hostname verification even though
Solr is healthy. Preserve nonblank advertised hosts, adding IPv6 brackets as
needed, and normalize loopback/wildcard addresses only in the bind-host
fallback.
##########
solr/core/src/java/org/apache/solr/cli/StatusTool.java:
##########
@@ -128,7 +127,8 @@ public void runImpl(CommandLine cli) throws Exception {
}
if (port != null) {
- Optional<SolrProcess> proc = processMgr.processForPort(port);
+ // When asked to wait, this also waits for a newly started process to
become visible
+ Optional<SolrProcess> proc = processMgr.waitForProcessOnPort(port,
maxWaitSecs);
Review Comment:
Process discovery can consume `maxWaitSecs`, but `printProcessStatus()` then
starts HTTP readiness polling with the original full budget. With
`--max-wait-secs 30`, finding the process after 25 seconds allows roughly 55
seconds of waiting. Use one deadline for discovery and readiness, passing only
the remaining budget to the HTTP wait and reporting a timeout if it is
exhausted.
--
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]
