Repository navigation
Conversation
bin/solr commands funneling through CLIUtils.getSolrClient (and several that hand-built their own HttpJettySolrClient.Builder) inherited SolrHttpConstants' 60s connect / 600s idle defaults, which are sized for long-running SolrJ use cases like bulk indexing and replica recovery. A human running a CLI command against an unreachable node wants fast failure instead. Centralizes the 15s/30s timeouts DeleteTool already used (and CreateTool used to, before a recent refactor moved it onto the shared client) into CLIUtils.CLI_CONNECTION_TIMEOUT_SECONDS / CLI_IDLE_TIMEOUT_SECONDS, and applies them everywhere a CLI tool builds an HttpJettySolrClient. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HaP4sXv7KQBw9ZEqtvwug2
|
This was claude generated, but looking hrtorugh, it seem sto make sense. I don't know if we want to have a single place to configure hte client, feels like we have gone and back and forth on this.... |
…le-timeouts # Conflicts: # solr/core/src/java/org/apache/solr/cli/DeleteTool.java
| new HttpJettySolrClient.Builder() | ||
| .withIdleTimeout(CLI_IDLE_TIMEOUT_SECONDS, TimeUnit.SECONDS) | ||
| .withConnectionTimeout(CLI_CONNECTION_TIMEOUT_SECONDS, TimeUnit.SECONDS) | ||
| .withOptionalBasicAuthCredentials(credentials); |
There was a problem hiding this comment.
any merits in extracting this HttpJettySolrClient Builder into a common class say CLIUtils & using it in all these 4 tools?
would make the code duplication little lesser.
|
@HoustonPutman I miss clicked your name on this PR when I meant a different one.. now i can't remove you as a requested reviewer, sorry about the noise. |
| */ | ||
| public static final int CLI_CONNECTION_TIMEOUT_SECONDS = 3; | ||
|
|
||
| public static final int CLI_IDLE_TIMEOUT_SECONDS = 30; |
There was a problem hiding this comment.
My local review agent was against this idea:
Here's what it quoted to me.
Shortening idle to 30s is not equivalent to “fail fast on a dead node,” because in SolrJ withRequestTimeout falls back to the idle timeout when unset (HttpSolrClient.Builder.getRequestTimeoutMillis() → getIdleTimeoutMillis()). Jetty then applies that value as the total request timeout (req.timeout(...)), not only as a quiet-socket limit.
So withIdleTimeout(30, SECONDS) without an explicit longer withRequestTimeout caps every HTTP round-trip at 30s wall-clock. That replaces SolrJ’s 600s default and will abort legitimate long CLI work that still has healthy I/O (export, streaming expressions, postlogs, large posts, package install, snapshot export, etc.).
Fail-fast against an unreachable node is a connect timeout concern — 3s there is right. Leave idle/request at SolrJ defaults (600s) so hung-connect fails quickly while long-running commands keep working. If we later want a short idle for “no bytes for N seconds,” we must set withRequestTimeout to DEFAULT_SO_TIMEOUT (or similar) explicitly so request duration is not coupled to idle.
|
I'm definilty not "feeling" this one... I'm going to close. |
|
also, thanks for the review. The PR that doesn't land is sometimes the best PR! |
2 participants
Summary
Spun out of review discussion on a
bin/solr createrefactor (SOLR-18320): CLI commands that construct aSolrClientviaCLIUtils.getSolrClient(or by hand-building anHttpJettySolrClient.Builder) currently inheritSolrHttpConstants' defaults — 60s connect / 600s idle. Those defaults are deliberately generous because they're shared by every SolrJ use case, including bulk indexing, shard-to-shard fan-out, and replica recovery, where a single request can legitimately take minutes.A human running a
bin/solrcommand at a terminal wants the opposite: fail fast against an unreachable or hung node.DeleteToolalready carried its own 15s/30s override for exactly this reason (andCreateToolused to, before a recent refactor accidentally dropped it by switching to the shared client builder).This PR centralizes that override:
CLIUtils.CLI_CONNECTION_TIMEOUT_SECONDS(15) andCLIUtils.CLI_IDLE_TIMEOUT_SECONDS(30).CLIUtils.getSolrClient's core builder and inCLIUtils.solrUrlFromConnection's builder, which covers every CLI tool that callsgetSolrClient(Healthcheck, SnapshotExport, Version, Assert, Export, Config, Create, Api, Delete, SnapshotList, Package, RunExample, Status, SnapshotCreate, SnapshotDelete).CLIUtils.getSolrClientand hand-build their ownHttpJettySolrClient.Builder:DeleteTool(replacing its hardcoded 15/30 with the shared constants),HealthcheckTool,ExportTool,PostLogsTool,StreamTool.Out of scope:
RunExampleTool's two internalwaitToSeeLiveNodes/example-bootstrapCloudSolrClientbuilds are left on the long defaults — those are local-example node-readiness polling loops with their own retry/backoff logic, not a user-facing connection attempt, so a short connect timeout there wouldn't offer the same benefit and risks interacting oddly with the poll loop.Test plan
./gradlew :solr:core:compileJava— compiles cleanly./gradlew :solr:core:test --tests DeleteToolTest --tests TestExportTool --tests StreamToolTest --tests CreateToolTest --tests HealthcheckToolTest --tests PostLogsToolTest— all pass (32 tests, 1 skipped)changelog/unreleased/🤖 Generated with Claude Code
https://claude.ai/code/session_01HaP4sXv7KQBw9ZEqtvwug2