Repository navigation
SOLR-7550: A 500 for the versions request now counts as success when cantReachIsSuccess is set, the same as a 503 - #5139
Draft
nick-boss-tech wants to merge 8 commits into
Draft
nick-boss-tech wants to merge 8 commits into
nick-boss-tech wants to merge 8 commits into
Conversation
…ext, add changelog
ShardResponse is final, so Mockito cannot mock it and the new PeerSyncTest cases could never run as written. Build a real ShardResponse instead. That needs setException and setShardAddress callable from the update package, so widen those two setters to public; setShardRequest and setSolrResponse on the same class are already public. The Mockito imports and guards added with the tests are no longer needed and are dropped.
PeerSyncTest calls initCore from its constructor, and the test runner constructs the class once per test method, so every method added to that suite leaks a whole harness core and the suite-end ObjectReleaseTracker check fails with the leaked core graph. The two handleResponse tests therefore live in a new PeerSyncHandleResponseTest that extends SolrTestCase and calls initCore once from @BeforeClass. PeerSyncTest is back to its main-branch contents.
SolrTestCase does not carry the SolrTestCaseJ4 harness (no initCore, no static h), so the new class follows the RootFieldTest pattern: start the embedded server rule, create the standard test collection, and take the core from its container with its reference counted.
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-7550
What happens today
A replica whose core will not load can block leader election.
The ticket describes four nodes. A schema change, made while one node was down, left two nodes with cores that would not load. The two remaining nodes could not finish a leader election, because their versions requests to the broken cores got HTTP 500 (ticket). The remaining nodes ended in
recovery_failed, and the cluster went offline.A core that fails to initialize is reported through
SolrCoreInitializationException, thrown byCoreContainer. On the merge base, PeerSync treats that 500 as a failure (PeerSync.java L428-L431).What this change does
PeerSync counts a versions request that fails with a core initialization failure as success, when
cantReachIsSuccessis set. Every other 500 still fails.PeerSync checks the exception class that Solr writes into the error metadata (PeerSync.java L424-L435; ResponseUtils.java L77). The 503 and 404 rules are unchanged (L400-L422).
Leader election passes the flag as true (SyncStrategy.java L225-L232). The
/getsync request reads the flag from the caller too (RealTimeGetComponent.java L1293-L1295), so the rule applies there. A caller who can set the flag already gets success for a refused connection (PeerSync.java L381-L398), so this adds no new outcome.Two
ShardResponsesetters are now public (L81, L100), andhandleResponseis package-visible (L368-L369), for the test.Proof
The proof at this head is pending. Counts will be added after the focused run.
PeerSyncHandleResponseTesthas three tests. An initialization failure on a versions request passes. Another 500 on a versions request fails. An initialization failure on an updates request fails.The base code cannot compile the test:
handleResponseand the two setters are not visible there. With the new branch removed, the first test should fail at its assertion. The focused run will confirm it.A choice to check
Three ways to stop a broken core from blocking the election. This PR implements the first.
Was option 1 the right call, or should the status change in option 3 be made instead?
Limits
handleResponsedirectly. The path from a real broken core to this rule is not tested end to end./getsync path is not tested.ShardResponsesetters are wider than the fix needs. They exist for the test.Changelog: changelog/unreleased/SOLR-7550-peersync-500.yml
AI assistance
AI agents assisted with research, implementation, review, and drafting. Nick Shanin directed the work and takes responsibility for this contribution.