Repository navigation
SOLR-7394: Clear recovering shard-terms entry when recovery is abandoned - #5003
Open
nick-boss-tech wants to merge 11 commits into
Open
nick-boss-tech wants to merge 11 commits into
nick-boss-tech wants to merge 11 commits into
Conversation
- RecoveryStrategy.testing_maxRetriesOverride: allows tests to exhaust recovery retries quickly (follows the in-file precedent of testing_beforeReplayBufferingUpdates). - TestInjection.failRecovery + hook in RecoveryStrategy: allows tests to force recovery failure deterministically after RECOVERING is published.
…ng shard terms ZkShardTermsRecoveryFailureTest verifies that when a recovering replica exhausts its recovery attempts, RecoveryStrategy clears the replica's _recovering shard-terms entry and resets its term to 0, restoring its eligibility to win a future leader election.
If clearing the failed recovery state from shard terms throws, the terminal state publication must still happen. A replica stuck showing RECOVERING with no recovery running is worse than a stale shard-term marker on a replica that has reached RECOVERY_FAILED. The cleanup failure is logged, not hidden. Also remove the branch-local testing handoff; it must not ship upstream.
recoveryFailed ran the shard terms cleanup and the RECOVERY_FAILED publication in the same try block, so a failure in the cleanup skipped the publication and left the replica without its terminal state, the opposite of the failure policy this change states. The cleanup failure is now caught and logged locally, the publication always runs, and close() stays in finally. RecoveryStrategyTest forces the cleanup to throw and verifies the terminal state is still published and the listener notified.
recoveryFailed wrote term 0, discarding the term that startRecovering saved for leader election. A failed recovery never rolls back the replica index, so the saved term is a sound lower bound on the replica data; writing 0 made a replica one write behind indistinguishable from a brand-new empty replica, and two failed replicas tied at 0 let election order pick the staler one. The saved term is now written back, and ShardTermsTest covers the case of two failed replicas with different saved terms. Also from the re-review: injectFailRecovery follows the TestInjection convention (the hook throws when armed and is called under assert, with the true:100 arming form), the retry-limit override moves from RecoveryStrategy into TestInjection so TestInjection.reset() clears it, RecoveryStrategyTest calls the now package-private recoveryFailed directly instead of through reflection, and the cloud test configures its cluster in @BeforeClass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI text below 🤖 (posted on behalf of Nick Shanin)
https://issues.apache.org/jira/browse/SOLR-7394
What happens today
When a leader-eligible replica exhausts its recovery retries,
RecoveryStrategypublishesRECOVERY_FAILEDbut leaves the replica's_recoveringentry in the shard terms in place. That entry permanently excludes the replica from leader election, even though no recovery is running anymore: the replica is stuck out of the election for good, not just until it catches up.What this change does
On the retry-exhaustion path,
RecoveryStrategynow clears the replica's_recoveringshard-terms entry and writes back the term the replica had before recovery began (the valuestartRecoveringsaved under that marker) before publishingRECOVERY_FAILED. The failed replica cannot outrank a replica that stayed current, since its restored term is the pre-recovery one, and election among failed replicas keeps their pre-recovery ordering. The shard-terms cleanup no longer shares a try block with the terminal-state publication: if the cleanup itself throws, the failure is logged andRECOVERY_FAILEDis still published, withclose()infinally. The change adds two test-only seams for deterministic coverage,TestInjection.failRecoveryandTestInjection.recoveryMaxRetriesOverride, both cleared byTestInjection.reset().Proof
Verified at head 9857d9e on 2026-10-06 (changelog YAML parse clean, tidy clean, Error Prone compile clean,
:solr:core:check -x testgreen).ZkShardTermsRecoveryFailureTest(1/1) is a cloud-level test: it exhausts retries through the seams, asserts the_recoveringentry is gone, the term is restored to the pre-recovery value (0 for this brand-new replica), and the replica can win an election once the higher-term replicas are gone. It fails on the unpatched base with "failed replica must not retain a _recovering shard-terms entry".RecoveryStrategyTest(1/1) forces the shard-terms cleanup to throw and verifiesRECOVERY_FAILEDis still published and the listener still notified; it fails against the previous shared-try structure.ShardTermsTest(6/6) covers the term restore directly, including two failed replicas with different saved terms, where only the fresher one stays leader-eligible. Run against the reset-to-0 code this change replaces, exactly the threerecoveryFailedtests in the class fail.TestTestInjection(5/5) pins the hook contract and the reset coverage for both seams.A choice to check
If the shard-terms cleanup fails, this change logs the failure and publishes the terminal state anyway, on the reasoning that a replica stuck showing RECOVERING with no recovery running is worse than a stale marker on a terminal replica. The alternative is to let a cleanup failure mask the
RECOVERY_FAILEDpublication. Was publishing anyway the right call?A second call in this change: on failure, the term
startRecoveringsaved under the_recoveringmarker is written back, rather than being discarded and reset to 0. The saved term is a lower bound on the replica's data: a failed recovery never rolls back the index (peer sync only adds data, and a replication fetch replaces the index only when it succeeds), so the replica still holds at least the updates its saved term reflects. Restoring keeps the pre-recovery ordering among failed replicas; resetting to 0 would tie two failed replicas and let election order pick the staler one, and the fresher replica's extra updates would then be discarded when it syncs from that leader. The cost of restoring: a replica whose index was damaged by whatever made recovery fail still carries its old term, and term comparisons cannot see index damage. Was restoring the saved term the right call?Limits
The trade behind the restore: once the higher-term replicas stop participating, a replica whose recovery never completed can be elected leader, and replicas that return afterwards then sync to it, so the shard can end up led by a replica with older data rather than having no leader at all. The restored term keeps a failed replica behind every replica that stayed current, so this only happens when no healthier replica remains. Retry exhaustion is reached through the test-only seams; a naturally timed exhaustion after real retries in a live cluster is not exercised. Election ordering after the restore is covered for two failed replicas in
ShardTermsTest; a live election race among several recovering replicas at mixed terms is not covered.Changelog:
changelog/unreleased/SOLR-7394.ymlAI assistance
AI agents assisted with research, implementation, review, and drafting. Nick Shanin directed the work and takes responsibility for this contribution.