[
https://issues.apache.org/jira/browse/SOLR-18402?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18108441#comment-18108441
]
Chan Chan commented on SOLR-18402:
----------------------------------
My understanding of the implementation: both predicates declared on
{{SolrClient}} with a {{false}} default, so {{LBSolrClient}} can call them
through {{getClient(endpoint)}} without an {{instanceof}} and the class-name
match goes away, with the real implementations in the concrete transports.
{{CloudSolrClient}} delegates to its underlying client and
{{ConcurrentUpdateBaseSolrClient}} to its own. Let me know if I've got that
backwards.
My plan is to start with a first PR covering the above for {{CloudSolrClient}}
and {{{}LBSolrClient{}}}, leaving {{SolrCmdDistributor}} and the streaming
clients as a follow-up. Let me know if you'd rather have it as one PR instead.
Would love to take this on. Could you assign it to me?
> 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
> Priority: Major
> Labels: newdev
>
> 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]
