David Smiley created SOLR-18402:
-----------------------------------
Summary: Consolidate wasRequestUnsent / wasCommError (retry) logic
Key: SOLR-18402
URL: https://issues.apache.org/jira/browse/SOLR-18402
Project: Solr
Issue Type: Improvement
Components: SolrJ
Reporter: David Smiley
h2. Problem
"Is this failure retriable, and did the request reach the server?" is answered
independently in at least four places, each with a different and drifting set
of exception types:
* {{CloudSolrClient.wasCommError}} -- {{SocketException}},
{{UnknownHostException}}, {{RequestNotSentException}}
* {{LBSolrClient.isConnectException}} -- {{ConnectException}},
{{HttpConnectTimeoutException}}, plus a class-*name* match
* {{LBAsyncSolrClient}} -- a near-copy of the above
* {{SolrCmdDistributor.isRetriableException}} -- {{SocketException}},
{{SocketTimeoutException}} (chain-walked since SOLR-18346)
Only the transport actually knows whether bytes reached the network, yet none
of these can ask it. {{LBSolrClient}} resorts to matching on the class name:
{code:java}
// Check for common connection timeout exceptions by name to avoid hard
dependencies on
// specific HTTP client libraries (e.g., Jetty or Apache HttpClient).
return t != null && t.getClass().getName().endsWith("ConnectTimeoutException");
{code}
That is the module boundary being worked around with string comparison, and it
is the clearest sign the knowledge lives in the wrong place. The practical
result is that each site drifts: SOLR-18401 found that a closed pooled
connection was invisible to {{CloudSolrClient}} and {{LBSolrClient}}, and the
same blind spot still exists in {{SolrCmdDistributor}} and in
{{HttpJdkSolrClient}}.
h2. Proposal
Separate two questions that are currently conflated.
*(a) Did the request reach the server?* Purely transport knowledge. Push it
down to the client as a first-class answer.
*(b) Should this caller retry?* Depends on the request and on the caller's
policy, which legitimately differs -- {{LBSolrClient}} weighs replay safety,
while {{SolrCmdDistributor}} weighs sending a replica into recovery. Keep this
at the caller, but express it in terms of (a) instead of guessed exception
types.
Sketch, defined on {{HttpSolrClient}} and overridden per transport
({{HttpJettySolrClient}} knows {{EofException}} / {{ClosedChannelException}} /
HTTP/2 "session closed"; {{HttpJdkSolrClient}} has its own set):
{code:java}
/** Whether the failure proves the request never reached the server. */
public boolean wasRequestUnsent(Throwable t)
/** Whether this is a transport-level communication failure at all. */
public boolean wasCommError(Throwable t)
{code}
Then {{CloudSolrClient}} delegates to its underlying client, {{LBSolrClient}}
asks via {{getClient(endpoint)}}, and {{ConcurrentUpdateBaseSolrClient}} asks
its delegate. A {{false}} default on {{SolrClient}} avoids an {{instanceof}}
check in the LB. The class-name match goes away.
h2. Notes
* For the streaming clients, consolidate the *classification* but not the
*decision*. {{ConcurrentUpdateJettySolrClient}} streams many documents over one
long-lived POST, so once committed a failure has delivered an unknown prefix;
"unsent" is only meaningful for the pre-commit window there.
* {{SolrCmdDistributor.isRetriableException}} only recognises
{{SocketException}} / {{SocketTimeoutException}}, so a dead pooled connection
from a leader to a replica would trigger recovery rather than a retry. Not
verified against a live reproduction.
* {{LBSolrClient.doRequest}} has no {{IOException}} catch, so a transport that
throws one directly -- as {{HttpJdkSolrClient}} does -- falls into the
catch-all and gets neither failover nor zombie marking. {{LBAsyncSolrClient}}
already handles this. Consolidation should remove the asymmetry rather than
patch it in both places.
* Classification becomes unit-testable per transport, instead of only reachable
through racy integration tests.
SOLR-18401 is a first step in this direction: {{RequestNotSentException}} is
already the transport stating the answer rather than the caller inferring it.
This issue generalises that from one exception type to the whole predicate.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
