[
https://issues.apache.org/jira/browse/SOLR-18505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18123536#comment-18123536
]
Nick Shanin commented on SOLR-18505:
------------------------------------
AI-generated, human-approved text below.
Correction and update to my previous comment. The trigger wording there was
wrong:
TieredMergePolicy does not admit a segment for exceeding deletesPctAllowed on
its
own. The budget is index-wide: a natural merge is scheduled when the index's
total
deletions exceed deletesPctAllowed (20 percent by default) of its total
documents. In
this test's end state (five documents, one deletion) the allowed count is 1, so
no
merge is selected there; the observed merge was admitted at a four-document
state
with the deletion already counted (allowed count 0), reached when the seed's
flush
settings split the second batch into more than one segment. That admission
point is
inferred from the policy's arithmetic and the merged segment in the failing
runs, not
from a recorded log. The ticket description has been corrected to match.
The same review round also found that the wrapper as first shipped left one
entry
point open: commits and searcher opens consult findFullFlushMerges, which
bypassed
the wrapper. PR #5028 now blocks that entry point too, at head 8ca33200e37; the
gate
(tidy, Error Prone compile, check) and the seed battery (five forced runs with
seed
71E7F210A62A9B8C and two random-seed runs, all 7 of 7) were re-run green at
that head,
and the PR description carries the corrected wording.
> DirectUpdateHandlerTest.testExpungeDeletes is flaky: a deletion-driven merge
> races the post-commit searcher sample
> ------------------------------------------------------------------------------------------------------------------
>
> Key: SOLR-18505
> URL: https://issues.apache.org/jira/browse/SOLR-18505
> Project: Solr
> Issue Type: Bug
> Reporter: Nick Shanin
> Priority: Minor
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> AI-generated, human-approved text below.
> DirectUpdateHandlerTest fails intermittently in CI (seen on a Solr Tests via
> Crave run for an unrelated PR). With the run's seed, 71E7F210A62A9B8C, it
> fails deterministically in local forced re-runs: 6 of 6 across current main
> and the parent of the SOLR-18317 merge, so it predates those changes and is
> not caused by them.
> Failure signature
> - testExpungeDeletes: AssertionError "maxDoc !> numDocs ... expected some
> deletions" at DirectUpdateHandlerTest.java:494 (one run showed the sibling
> variant "expected:<5> but was:<4>" at line 501).
> - testDeleteRollback: AssertionError in teardown (knock-on, see below).
> - classMethod: ObjectTracker reports unreleased objects (knock-on).
> Root cause
> testExpungeDeletes adds a duplicate document, creating one deletion. The test
> class
> pins TieredMergePolicy, which schedules a natural merge when the index's
> deleted
> documents exceed deletesPctAllowed (20 percent by default in Lucene 10.4) of
> its total
> documents; the budget is index-wide, not per segment. In the end state the
> test
> describes (two segments, five documents, one deletion) the allowed count is
> 1, so no
> merge is selected there. The merge the failing runs show was admitted at a
> moment when
> the index still held four documents with the deletion already counted, which
> is the
> state when the seed's flush settings split the second batch into more than one
> segment: there the allowed count is 0 and one deletion exceeds it. (That
> admission
> point is inferred from the policy's arithmetic together with the merged
> segment below;
> the failing runs did not record their flush settings.) The merge runs on the
> ConcurrentMergeScheduler thread. If it lands before Solr opens the post-commit
> searcher, the sample at line 494 reads the already-merged index and finds no
> deletions
> left to observe, so the assertion fails even though nothing is wrong with the
> update
> handler. In failing runs the surviving segment's diagnostics record
> source=merge and
> there is no live-docs file, confirming the deletions were removed by the
> background
> merge before the sample.
> Knock-on failures
> The failed assertion skips closing the sample request, the leaked request
> pins the core's directory, and the next test's deleteCore() then fails in
> CachingDirectoryFactory.close; that is the whole of the testDeleteRollback
> teardown failure and the ObjectTracker report. testDeleteRollback's own body
> does not fail.
> Proposed fix
> In testExpungeDeletes only: wrap the core's live merge policy for the
> duration of the
> test in a FilterMergePolicy that returns null from findMerges and from
> findFullFlushMerges (the entry point that commits and searcher opens
> consult), saving
> the live policy beforehand and restoring it in a finally block, and delegating
> findForcedDeletesMerges to the live policy so the expungeDeletes commit still
> performs
> a real expunge. That removes natural merges from the test outright, so the
> test keeps
> verifying what it verifies today. Also put the two sample requests in
> try-with-resources
> so a failed assertion can never leak a searcher again. (NoMergePolicy is not a
> substitute: its forced-deletes merge lookup returns null, which would turn
> the expunge
> into a no-op and gut the test.)
> This section originally proposed raising deletesPctAllowed to 100.0 instead.
> That value
> is illegal (the setter accepts at most 50), and although the maximum of 50
> would also
> have blocked the merge in this test, the wrapper was shipped because it does
> not rest
> on the deletes-budget arithmetic at all. The fix is implemented in PR #5028.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
