[
https://issues.apache.org/jira/browse/SOLR-18341?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
David Smiley updated SOLR-18341:
--------------------------------
Priority: Critical (was: Major)
> SolrJ: let each SolrRequest declare whether it may be retried
> -------------------------------------------------------------
>
> Key: SOLR-18341
> URL: https://issues.apache.org/jira/browse/SOLR-18341
> Project: Solr
> Issue Type: Improvement
> Components: SolrCloud, SolrJ
> Reporter: David Smiley
> Priority: Critical
>
> h2. Problem
> {{CloudSolrClient}} and {{LBSolrClient}} both retry requests when they
> believe the server never received them. Each infers retry-ability separately,
> from a different signal, and the two disagree:
> * {{LBSolrClient}} derives it from the request type —
> {{req.request.getRequestType() == SolrRequestType.UPDATE || isAdmin}} — and
> then restricts update failover to connect-class failures.
> * {{CloudSolrClient}} uses {{wasCommError()}}, a top-level {{instanceof
> SocketException || UnknownHostException}}, which includes connection resets
> and broken pipes. Those occur *after* the request bytes were written, so the
> update may well have been applied.
> Neither asks what the request actually contains. The result is that an atomic
> {{inc}} caught by the {{CloudSolrClient}} comm-error path is replayed and
> silently double-applies — wrong data, no error. And lately (since the switch
> to Jetty HttpClient with HTTP/2), some communication errors seen by
> {{LBSolrClient}} defeat retry-ability that used to happen (with HTTP/1.1).
> Using instanceof checks is a code smell for the request is a code smell. It
> prevents customization. There are four update request classes
> ({{AbstractUpdateRequest}}, {{UpdateRequest}},
> {{ContentStreamUpdateRequest}}, {{StreamingUpdateRequest}}) and only one of
> them holds a materialized document map. The rest wrap opaque streams that
> SolrJ cannot introspect *and* cannot re-send.
> h2. Proposal
> Add a virtual method to {{SolrRequest}}:
> {code:java}
> /**
> * Whether this request may be re-sent when the client does not know if the
> server received it.
> * Only consulted on the failure path.
> */
> public boolean isRetriable() { ... }
> {code}
> Each request answers for itself. No casts, no {{instanceof}}, and the
> requests that cannot introspect their own payload declare themselves
> non-retriable rather than being silently misclassified.
> h3. Base implementation
> Derived from {{getRequestType()}}:
> * {{QUERY}} — retriable
> * {{ADMIN}}, {{SECURITY}}, {{UNSPECIFIED}} — not retriable (unchanged from
> today's behavior)
> * {{UPDATE}} — not retriable at the base, so the {{UpdateRequest}} override
> is the only thing that can permit it
> *The HTTP method is deliberately not consulted.* "GET implies safe" is the
> natural assumption _and it is false in this codebase_, sadly:
> {{SolrRequest:120}} declares {{private METHOD method = METHOD.GET}}, so GET
> is the default value rather than a declaration, and every V1 admin family is
> GET-by-construction — {{CollectionAdminRequest:87}},
> {{CoreAdminRequest:536,540}}, {{ConfigSetAdminRequest:54,58}}. CREATE,
> DELETE, SPLITSHARD and UNLOAD are all issued as HTTP GET today! Correcting
> those verbs is worth doing and will be filed separately; this issue doesn't
> depend on it.
> h3. UpdateRequest override
> {{UpdateRequest}} inspects its own contents. Nearly everything is safe to
> replay:
> ||Operation||Verdict||Rationale||
> |Full document add|permit|Absolute. Replay re-stamps {{_version_}}; final
> document is identical.|
> |Delete by id|permit|Absolute. Second delete is a no-op.|
> |Delete by query|permit|Strictly idempotent — replay against an unchanged
> index deletes nothing.|
> |Commit / optimize|permit|Idempotent by construction.|
> |Atomic {{set}}|permit|Absolute value. Includes set-to-null.|
> |Atomic {{inc}}|*reject*|Relative arithmetic. Replay double-applies.|
> |Atomic {{add}} of scalar values|*reject*|Appends without a distinctness
> check. Replay duplicates values.|
> |Atomic {{add}} of child documents|permit|Upsert keyed by child id, not an
> append — see below.|
> |Atomic {{add-distinct}}|permit|Adds only if absent; second application is a
> no-op.|
> |Atomic {{remove}}|permit|Value is already gone on replay; second application
> is a no-op.|
> |Atomic {{removeregex}}|permit|Same as {{remove}}.|
> The reject set is two cases: {{inc}}, and {{add}} of scalar values. Operators
> are enumerated in {{AtomicUpdateDocumentMerger:162-188}}.
> {{add-distinct}}, {{remove}} and {{removeregex}} are worth calling out
> because they read as "relative" but are idempotent — each is defined against
> the field's current contents, so a second application finds nothing left to
> do.
> Requests carrying a document iterator ({{UpdateRequest.setDocIterator}})
> return false: the iterator is exhausted after the first attempt, so a replay
> would silently carry fewer documents. {{getRoutes()}} already returns null
> for these ({{UpdateRequest:228-231}}), so they take the general streaming
> path.
> h3. Nested documents
> Child documents are field values inside a {{SolrInputDocument}}
> ({{AtomicUpdateDocumentMerger:693}}), and nested atomic updates merge the
> child's operator map recursively ({{:115-130}}, {{:528}}). An operator map
> can therefore be buried arbitrarily deep — {{\{"add": \{child doc with
> \{"inc": 1\}\}\}}} presents {{add}} at the top level and hides an {{inc}}
> inside. *The check must walk the document tree*, not scan top-level fields.
> h2. Exemptions
> Two cases skip the question entirely. Note the split: the caller decides
> whether to ask, the request answers what it contains.
> * *Connect-class failures* — TCP connect never completed, so the server never
> saw the request and replay is provably safe. A property of the failure, not
> the request, so this stays in the caller. {{LBSolrClient}} already uses this
> rule ({{isConnectException}}, {{:686-693}}).
> * *Operations carrying {{_version_} > 0}* — a replay after a successful first
> attempt fails with 409 CONFLICT, which is already the desired behavior. A
> property of the request, so it lives in the override.
> h2. Callers
> Both retry layers consult it:
> * {{LBSolrClient}} — endpoint failover within one shard's replica list.
> * {{CloudSolrClient.requestWithRetryOnStaleState}} — whole-request replay on
> stale cluster state. Still needs the check: a leader move does not tell you
> the pre-move attempt did not apply.
> Two callers at different layers asking one question is an additional argument
> for putting it on the request. Today each has its own divergent answer, and
> consolidating them is the same fix.
> h2. Testing
> * Unit tests over hand-built requests covering every row of the table, both
> exemptions, the {{docIterator}} case, and an operator map buried inside a
> child document.
> * Integration tests injecting mid-request faults with {{SocketProxy}} to
> confirm the check fires on a real client-generated retry, and does *not* fire
> on connect-refused failover.
> h2. Related
> * SOLR-9355 — the same "decide retriability from the wrong place" defect at
> the leader-to-replica hop. Its fix scans the cause chain rather than checking
> one fixed position; {{CloudSolrClient.wasCommError}} ({{:210-212}}) and
> {{LBSolrClient.isConnectException}} ({{:686-693}}) have exactly that bug and
> should share the extracted helper.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
