[
https://issues.apache.org/jira/browse/SOLR-18401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18107599#comment-18107599
]
David Smiley commented on SOLR-18401:
-------------------------------------
Perhaps debatable but I view this as an improvement, not a bug fix. Our
ability to know when to retry is best-effort; not understood to be perfect.
> HttpJettySolrClient: detect if request wasn't sent; means retry-able
> --------------------------------------------------------------------
>
> Key: SOLR-18401
> URL: https://issues.apache.org/jira/browse/SOLR-18401
> Project: Solr
> Issue Type: Improvement
> Components: SolrJ
> Reporter: David Smiley
> Assignee: David Smiley
> Priority: Major
> Labels: pull-request-available
> Time Spent: 20m
> Remaining Estimate: 0h
>
> h2. Problem
> {{CloudSolrClient}} no longer retries a request that failed because the
> pooled HTTP connection it was handed had already been closed. The request
> never reached the server, yet the failure propagates to the caller instead of
> failing over to another replica.
> This is a regression from SOLR-18188, which removed {{CloudLegacySolrClient}}
> (Apache HttpClient) and left the Jetty-based client as the only
> implementation. The legacy client classified this case as a communication
> error:
> {code:java}
> @Override
> protected boolean wasCommError(Throwable rootCause) {
> return super.wasCommError(rootCause)
> || rootCause instanceof ConnectTimeoutException
> || rootCause instanceof NoHttpResponseException;
> }
> {code}
> {{NoHttpResponseException}} is Apache HttpClient's signal for "the server
> closed the pooled connection before reading the request" – provably never
> processed, therefore safe to replay. The Jetty transport has no equivalent,
> and the base implementation does not cover it:
> {code:java}
> /** Is this a communication error? We will retry if so. */
> protected boolean wasCommError(Throwable t) {
> return t instanceof SocketException || t instanceof UnknownHostException;
> }
> {code}
> Over the Jetty transport the same condition surfaces as
> {{{}ClosedChannelException{}}}, {{{}EofException: Connection reset by
> peer{}}}, or {{{}IOException: Broken pipe{}}}. All are plain
> {{{}IOException{}}}, none is a {{{}SocketException{}}}, so {{wasCommError}}
> returns false and {{CloudSolrClient}} gives up with retries still available:
> {noformat}
> CloudSolrClient request was not communication error it seems
> CloudSolrClient Request to collection [collection1] failed due to (0)
> java.io.IOException: Broken pipe, retry=0 maxRetries=5 commError=false
> errorCode=0
> {noformat}
> {{LBSolrClient}} also declines, because updates are non-retryable there
> unless the cause is a {{{}ConnectException{}}}:
> {code:java}
> } else if (isNonRetryable && isConnectException(rootCause)) {
> ex = (!isZombie) ? makeServerAZombie(baseUrl, e) : e;
> }
> {code}
> The outcome therefore depends on a race. If Jetty's connection pool has
> already evicted the dead connection, the next attempt gets a
> {{ConnectException}} (a {{{}SocketException{}}}), both layers retry, and the
> request succeeds. If the pool hands out the not-yet-evicted dead connection,
> the request fails hard.
> h2. User impact
> Any {{CloudSolrClient}} deployment where idle pooled connections are reaped –
> a server idle timeout, a load balancer, a firewall – now sees intermittent
> hard failures on requests that never reached Solr. Solr 10.x and earlier
> retried these.
> h2. How it was found
> {{RecoveryAfterSoftCommitTest}} went from zero CI failures to a sustained
> 3-10% failure rate. Per fucit.org weekly failure rates:
> {noformat}
> 2026-18 .. 2026-23 no failures
> 2026-24 3.4% (17/502)
> 2026-26 5.7%
> 2026-28 6.9%
> 2026-30 7.2%
> 2026-32 9.8%
> 2026-34 6.6%
> {noformat}
> ISO week 24 is 2026-06-08..14. SOLR-18188 landed 2026-06-10.
> The test partitions the non-leader replica with a {{{}SocketProxy{}}}, then
> keeps indexing. {{SolrTestCaseJ4.RandomizingCloudSolrClientBuilder}}
> randomizes {{{}shardLeadersOnly{}}}, so roughly half of runs use a client
> that sends updates to any replica – including the partitioned one. Archived
> CI stacks show the client POSTing directly to the follower:
> {noformat}
> SolrServerException: IOException occurred when talking to server at:
> http://127.0.0.1:35661/solr/collection1_shard1_replica_n3/update
> {noformat}
> Reproduced locally at a comparable rate (1 failure in 12 seeds); the failure
> is a race, not seed-deterministic.
> h2. Proposal
> Rather than broadening {{wasCommError}} to all {{IOException}} – which would
> replay updates that may have been applied – determine structurally whether
> any bytes of the request reached the wire.
> Jetty's {{Request.onRequestCommit(Request.CommitListener)}} fires from
> {{{}HttpSender.headersToCommit(){}}}, which runs in the write callback's
> {{onSuccess()}} – after the header bytes were successfully written to the
> {{{}EndPoint{}}}. If commit never fired, nothing of the request was sent, so
> the server cannot have seen it and a retry is safe even for a non-idempotent
> update. This is exactly the {{NoHttpResponseException}} invariant, recovered
> on the Jetty transport.
> Note the converse does not hold: "committed" only means the bytes entered the
> kernel send buffer, not that the server processed them. That errs
> conservative – we still do not retry – so there is no behavior regression.
> Sketch:
> * In {{{}HttpJettySolrClient{}}}, attach a commit listener when the request
> is built (alongside the existing {{req.attribute(REQ_PRINCIPAL_KEY, ...)}}
> and listener wiring) and record whether commit fired.
> * In the {{catch (ExecutionException)}} block, when the cause is an
> {{IOException}} and the request was never committed, throw a distinct
> exception rather than the generic {{{}"IOException occurred when talking to
> server at: ..."{}}}.
> * Teach {{CloudSolrClient.wasCommError}} and {{LBSolrClient}} to treat that
> exception as retryable.
> Two options for the exception type:
> # Reuse {{{}ConnectException{}}}. No changes outside {{{}solrj-jetty{}}}: it
> extends {{SocketException}} so {{wasCommError}} passes, and
> {{LBSolrClient.isConnectException}} already accepts it. Minimal, but
> semantically inaccurate – nothing refused the connection.
> # Add a marker type in solrj core, e.g. {{{}SolrRequestNotSentException
> extends IOException{}}}, and reference it from {{wasCommError}} and
> {{{}LBSolrClient{}}}. {{solrj-jetty}} already depends on solrj. Slightly
> larger diff, but it names the invariant that makes the retry safe. Preferred.
> h2. Also in scope
> * {{HttpJettySolrClient}} wraps Jetty's HTTP/2 "session closed"
> {{IllegalStateException}} in a bare {{{}IOException{}}}, which hits the same
> classification gap. A lost session is by definition a request that was never
> sent.
> * The async send path needs the same handling, or async callers keep the
> current behavior.
> h2. Not proposed
> Configuring {{{}CloudSolrClient{}}}'s internal client to use HTTP/1.1 was
> considered. Both transports pool keep-alive connections under
> {{{}setIdleTimeout(-1){}}}, so a reused dead connection yields the same
> untyped {{IOException}} either way; it changes the odds, not the failure mode.
> _(disclaimer: written by AI, of course)_
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
