[
https://issues.apache.org/jira/browse/SOLR-18341?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
David Smiley updated SOLR-18341:
--------------------------------
Description:
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.
> 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: Major
>
> 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
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
