nick-boss-tech commented on PR #5253: URL: https://github.com/apache/solr/pull/5253#issuecomment-6489721489
🤖 *AI text below* 🤖 *(reviewed by Nick Shanin)* @SpookyJumpyBeans, three preservation regressions fail on your current head (`32b91612edde`) with the new tests and a failure-injection hook. I prepared follow-up fixes for those cases and an assertion-failure path: - Dropping the target core's whole buffer can discard acknowledged updates and deletes forwarded by an earlier successful migration. The fix replays the shared buffer after restoring routing state. - Removing the whole routing rule can also remove ranges from an earlier successful transfer or invocation. The fix restores the previous rule only if the current rule still matches the one this step installed. - If routing restoration fails, cleanup stops before replaying the buffer. An `AssertionError` also runs guarded cleanup and is reported as a failed command, preventing the Overseer from retrying against a still-buffering target. Fatal JVM errors still propagate. Replaying preserves those acknowledged writes, but it can leave partial data from the failed migration on the target. The tests and Reference Guide document that limit. This replaces the discard action added in this PR; there is no net addition or removal of that action relative to upstream. [Diff with fixes, tests and documentation](https://github.com/nick-boss-tech/solr/compare/32b91612edde057c19bf1a7963e8276b816551df...21804641d0ebbb5199a1c205f031af47c9424290). <details> <summary>Linux verification at 21804641d0eb</summary> - `MigrateRouteKeyTest`, `MigrateCmdCleanupTest` and `SliceMutatorRoutingRuleTest`: 18 tests passed with seed `5253C0DE`, zero failures or skips. The four new cloud regressions also passed at `5253BEEF` and `5253CAFE`. - Neighbour coverage: 61 tests ran, with one failure in `CoreAdminHandlerTest.testDeleteInstanceDirAfterCreateFailure`. The same method fails on upstream `e59ce3c97d` with the same seed. - Tidy left a clean tree; changelog parsing, `check -x test` and `:solr:solr-ref-guide:checkSiteLinks` passed. Error Prone was enabled separately for core production and test compilation with `-Pvalidation.errorprone=true`; both passed with zero warnings. Fail-before checks reproduced all three preservation failures on your original head, all four cleanup-prerequisite failures with the prerequisite guard removed, and the assertion-retry failure before its fix. [Verification details and corrections](https://github.com/nick-boss-tech/solr/blob/9b674191fb2a38f0deae3eacc807977df28271e0/receipts/PR-5253-cleanup-repair-round-5-closed.md). </details> To take the tested changes into your PR branch: ```sh git switch migrate-cleanup-on-failure git fetch https://github.com/nick-boss-tech/solr.git solr-18601-review git merge --ff-only 21804641d0ebbb5199a1c205f031af47c9424290 ``` You're welcome to take these commits or adapt the changes. I recommend preserving established forwarding streams and documenting partial target data as the failure contract. Does that contract and the regression coverage meet the intended scope here? -- 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]
