nick-boss-tech opened a new pull request, #5139: URL: https://github.com/apache/solr/pull/5139
🤖 *AI text below* 🤖 *(posted on behalf of Nick Shanin)* https://issues.apache.org/jira/browse/SOLR-7550 ## What happens today **During leader election, an HTTP 500 from a replica for the versions request is treated as a failure.** The ticket describes a four node cluster. A schema change made while one node was down left that node's core unable to load. The leader was restarted later, so two nodes had cores that would not load. The remaining two nodes tried to become leader. PeerSync got HTTP 500 for the versions request from the two broken cores, so neither election could finish. The ticket reports that the remaining nodes ended in `recovery_failed` and the cluster went offline ([ticket](https://issues.apache.org/jira/browse/SOLR-7550)). PeerSync already counts a 503 or a 404 from that request as success when `cantReachIsSuccess` is set ([PeerSync.java L398-L420 at the base commit](https://github.com/nick-boss-tech/solr/blob/14c7aac0d151402b00259e2fb9bf5eed7049ec5d/solr/core/src/java/org/apache/solr/update/PeerSync.java#L398-L420)). ## What this change does **A 500 for the versions request now counts as success when `cantReachIsSuccess` is set, the same as a 503.** The rule checks two things: the request is a versions request, and the status is 500 ([PeerSync.java L423-L433](https://github.com/nick-boss-tech/solr/blob/687165651f9210e333a03c566a27f59f40527174/solr/core/src/java/org/apache/solr/update/PeerSync.java#L423-L433)). It does not look at the error text or the cause, so every 500 for that request counts, not only the failed core case. A 500 for an update request still fails. The log line says the replica counted as success. Two smaller changes support the test. `ShardResponse.setException` and `setShardAddress` are now public ([L81](https://github.com/nick-boss-tech/solr/blob/687165651f9210e333a03c566a27f59f40527174/solr/core/src/java/org/apache/solr/handler/component/ShardResponse.java#L81), [L100](https://github.com/nick-boss-tech/solr/blob/687165651f9210e333a03c566a27f59f40527174/solr/core/src/java/org/apache/solr/handler/component/ShardResponse.java#L100)), so a test in another package can build a response. `PeerSync.handleResponse` is package visible for the same test ([L367-L368](https://github.com/nick-boss-tech/solr/blob/687165651f9210e333a03c566a27f59f40527174/solr/core/src/java/org/apache/solr/update/PeerSync.java#L367-L368)). ## Proof **This proof is inconclusive by construction. The new tests do not compile on the base commit, so they cannot show a failure there.** - [`PeerSyncHandleResponseTest`](https://github.com/nick-boss-tech/solr/blob/687165651f9210e333a03c566a27f59f40527174/solr/core/src/test/org/apache/solr/update/PeerSyncHandleResponseTest.java#L50-L74) 2 of 2 passed at `687165651f9`. - `PeerSyncTest` 1 of 1 passed at the same head. - On the base commit, `handleResponse` is private and the two `ShardResponse` setters are package private. The new test class therefore does not compile there. The first test sends a 500 for a versions request with `cantReachIsSuccess` set and expects success. The second sends a 500 for an update request and expects failure. ## A choice to check **The 500 rule covers every 500 for a versions request, not only a core that failed to load.** Options considered: 1. Count every 500 for a versions request as success, as implemented. This is the reporter's proposal. The cost: a replica that returns 500 because of a runtime fault, while it holds newer updates, is also skipped. The leader may then miss updates that only that replica holds. 2. Count a 500 as success only when the failure is a core that did not load. The cost: the code has to recognize that case, either by matching the error text or by a marker from the server. Both are fragile. The implementation uses option 1. Was the broad rule the right call, or should it be limited to the failed core case? ## Limits **The tests call the code directly. They do not start a replica with a failed core.** - The path from a real HTTP 500 to this rule is not tested end to end. - The 503 and 404 handling is unchanged. - A narrower rule, which counts only a 500 from a core that did not load, is not implemented. Recognizing that case needs error-text matching or a server marker, and both are fragile. It can be a follow-up on request. - The two `ShardResponse` setters are wider than the fix needs. They exist so the test can build a response. Changelog: [changelog/unreleased/SOLR-7550-peersync-500.yml](https://github.com/nick-boss-tech/solr/blob/687165651f9210e333a03c566a27f59f40527174/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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
