Repository navigation
Conversation
|
Seems reasonable.. and I guess a bats test woudn't have caught this since it's windows only... |
janhoy
changed the title
epugh
left a comment
epugh
left a comment
There was a problem hiding this comment.
This all looks great. I think my one problem is that we're solving windows issues in our java code, but we don't have great testing in our windows path... I'd love, if we believe this is viable code, and it appears to be, to apply this to the linux path as well, that way if bugs crop up, the theory is we'll see and fix them tjhat much sooner. For example, we could use a bats test if we wanted...
|
This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution! |
|
Would be sad for this to die... |
…instance On Windows, 'solr.cmd start' invoked 'status --max-wait-secs' without identifying the instance just started, causing the status tool to scan the PID directory and wait for every running Solr instance. Pass the port so only the newly started instance is waited for, matching bin/solr on Linux. The status tool resolves the URL scheme (http/https) from the running process itself. The local URL for a process now prefers -Dsolr.host.advertise when set and falls back to the bind host parsed from the process command line. With SOLR_HOST_BIND set to a non-loopback address Jetty does not listen on localhost, so probing a hardcoded localhost could never reach the instance. Wildcard and loopback bind addresses still map to localhost, preserving existing URLs for default installs and keeping SSL hostname verification working with certificates issued for localhost. The -p wait re-scans for the process until the deadline instead of failing if the newly started process is not yet visible. Sysprop parsing is collapsed into one helper, dropping the unreachable -Dsolr.jetty.host fallback (10.x scripts always translate the deprecated name to solr.host.bind before launch), and the process wait loop moves into SolrProcessManager.waitForProcessOnPort, reusing the TimeOut utility.
… args On Windows the process arguments are recovered by splitting the raw command line, so quotes that the OS would have removed may still surround an argument or its value. Strip one surrounding quote pair, and match the -D prefix at the start of the argument rather than anywhere in it.
The JVM uses the last occurrence of a -D property, and bin/solr appends user SOLR_OPTS after the properties it generates itself. Picking the first match therefore ignored a user override of e.g. the advertised host.
Rewriting an advertised 127.0.0.1 or ::1 to localhost could break TLS hostname verification when the certificate covers the advertised address but not localhost. Loopback is now only normalized in the bind-host fallback, where no name is advertised. A wildcard advertised address is not connectable, so it falls through to the bind host.
…wait Waiting for the process to appear and then waiting for it to answer HTTP each used the full --max-wait-secs, so a slow process scan could nearly double the time the start script waits. Both now draw from one deadline, and an exhausted budget is reported as a timeout.
The option is declared twice, once per parser, and only the commons-cli twin was reworded. Regenerated the ref guide page, which is built from the picocli annotations.
8ab53f0 to
1d81010
Compare
|
🤖 Rebased on current Self-review turned up one thing the rebase made stale: Verified: Worth noting for merge: this bug now has a second, independent reproduction. The new cross-platform CLI test suite (SOLR-18642, #5323) hits the same hardcoded- |
github-actions
Bot
added
the
documentation
|
Automated PR readiness judgements (LLM-generated; may be wrong; not updated on later pushes).
|
Fixes https://issues.apache.org/jira/browse/SOLR-18336
The Windows start script now passes
-p <port>toSolrCLI status, so the start wait targets only the newly started instance instead of every PID file found.The status tool resolves the instance's URL from the running process's own command line: it prefers
-Dsolr.host.advertisewhen set, falls back to a non-loopback-Dsolr.host.bind, and maps loopback/wildcard binds tolocalhost. Since the process may not be visible in the process table immediately after launch, the tool re-scans until the--max-wait-secsdeadline instead of failing on the first miss.This gives the same behavior as 9.x, which built
--solr-urlfromSOLR_HOST(the advertised host) when set, else localhost: the wait connects to the advertised name — so SSL hostname verification against certificates issued for that name keeps working — and non-loopback binds (SOLR_HOST_BIND=10.0.x.x) no longer fail with a localhost connection error. As a bonus,bin/solr statusnow prints reachable URLs for such nodes too.PS: The Linux start script does not use SolrCLI for waiting for Solr, thus it was never affected. With these hardenings, we are perhaps soon ready to migrate the linux start script over to StatusTool as well.