Repository navigation
SOLR-18391: Fail collection creation cleanly when the collection is missing from cluster state during replica assignment - #4997
nick-boss-tech wants to merge 6 commits into
Conversation
…Test Mockito's thenReturn() cannot accept PlacementPluginFactory<?> for a method returning PlacementPluginFactory<?> because the two wildcard captures never unify. Use doReturn(), which takes Object, instead.
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
I appreciate the fix; it's better than the status quo. Although the race condition indicated suggests the fix is to resolve the race.
As for the tests... there's so much mock setup that I think the value proposition of retaining them isn't worth it.
The fix is a small null check that is obviously correct by inspection; the elaborate Mockito scaffolding costs more to maintain than it verifies.
nick-boss-tech
left a comment
nick-boss-tech
left a comment
There was a problem hiding this comment.
Dropped both test files.
|
I'm hesitant to merge this. It replaces an NPE with an error, but the null/race shouldn't have happened in the first place. We still have a TODO/problem to be resolved eventually. The fact that the collection is in a failed state is bad; this fixes that for this very narrow case. If you improved collection creation error detection to be graceful -- that would be welcome, and would also address the NPE here and also other bugs TBD. A colleague at my last company did that but didn't contribute it :-( |
|
🤖 AI text below 🤖 (posted on behalf of Nick Shanin) As suggested here, the broader route is now up as #5027: collection creation cleans up after itself when it fails partway, instead of only handling the one NullPointerException case. This draft is superseded by that PR; please review there. |
2 participants
🤖 AI text below 🤖 (posted on behalf of Nick Shanin)
https://issues.apache.org/jira/browse/SOLR-18391
What happens today
Collection creation can fail with a
NullPointerExceptionand leave a zombie collection behind. During a create, the placement plugin reads the new collection's state; because of a timing window between the state being written and the read, that lookup can return null, andPlacementPluginAssignStrategydereferences it. The NPE is not an exception type the create flow handles, so the failure escapes without the cleanup that a handled failure gets: the half-created collection stays in the cluster state, unusable, until an operator removes it by hand.What this change does
PlacementPluginAssignStrategychecks the lookup and, when the collection is not found, throws anAssign.AssignmentExceptionnaming the collection instead of dereferencing null. That is the exception typeCreateCollectionCmdalready catches: its existing handling runsDeleteCollectionCmdcleanup and returns a BAD_REQUEST, so the create now fails cleanly and can be retried. The production change is 17 lines in one file; the cleanup path it triggers is existing, unmodified code.Proof
No automated test ships with this PR, at the reviewer's request: the two test classes drafted for it were mock-heavy and were removed (David Smiley's review feedback; the removal is the head commit of this branch). An earlier version of the branch carried a strategy-level test showing the
AssignmentExceptionis thrown in place of the NPE.Verified at head 3000eee on 2026-10-04: the tree is tidy-clean, compiles with Error Prone enabled, and passes
:solr:core:check -x test.Limits
There is no regression test for this race, per the reviewer request above. The timing window itself is not reproduced by a test; the behavior rests on the code path, where the thrown exception type is the one the create flow already handles.
Changelog:
changelog/unreleased/SOLR-18391.yml(fixed)AI assistance
AI agents assisted with research, implementation, review, and drafting. Nick Shanin directed the work and takes responsibility for this contribution.