SpookyJumpyBeans opened a new pull request, #5253: URL: https://github.com/apache/solr/pull/5253
https://issues.apache.org/jira/browse/SOLR-18601 # Description `MigrateCmd` asks the target leader to buffer updates and adds a routing rule to the source shard before creating a temporary collection, splitting into it and merging into the target. If any of those steps fails, the exception propagates with the target leader still buffering and the routing rule still in place. Updates forwarded under the rule are buffered and never become searchable on the target, and a retry fails immediately because `REQUESTBUFFERUPDATES` requires the target's update log to be `ACTIVE`. # Solution The steps between buffering and applying the buffered updates are wrapped in a `try`. On failure, `cleanupFailedMigrate()` removes the routing rule, waits for the removal to be visible (updates forwarded until then would otherwise be indexed by the target once it stops buffering), and then asks the target leader to drop its buffered updates. Errors during cleanup are added to the original exception as suppressed exceptions; an `InterruptedException` skips cleanup and keeps the interrupt flag. ```java } catch (Exception e) { cleanupFailedMigrate( adminCmdContext, sourceCollection, sourceSlice, sourceRouter.getRouteKeyNoSuffix(splitKey) + "!", targetLeader, e); throw e; } ``` Dropping, rather than applying, the buffered updates puts the target back in its pre-MIGRATE state. The documents aren't lost: the source shard leader indexes them and sends them to its own replicas as usual, and forwarding to the MIGRATE target is an additional copy (`DistributedZkUpdateProcessor.doDistribAdd`/`doDistribDeleteById`). There was no core admin action to drop buffered updates (`UpdateLog.dropBufferedUpdates()` exists but had no callers), so this adds `REQUESTDROPBUFFEREDUPDATES`, next to `REQUESTBUFFERUPDATES` and `REQUESTAPPLYUPDATES` and modeled on them (missing core is a 400, not buffering is a 500, unexpected errors are wrapped in a 500). Notes: - Most of the `MigrateCmd` diff is re-indentation for the new `try` block; it's much smaller with whitespace ignored. - The new action is internal and v1 only, like how MIGRATE uses its two siblings. They also have v2 wrappers; I didn't add one for this action, but can if that's wanted. - A failure of the final `REQUESTAPPLYUPDATES` itself isn't covered: by then the target has the merged index, and dropping the buffer would lose updates made during the MIGRATE. - An update that resolved the routing rule just before it was removed could still reach the target after the buffer is dropped and be indexed there. Waiting for the rule removal makes this window small but doesn't close it. - If the merge actually completed but its response was lost, the target already has the merged documents; the cleanup can't tell this apart from a failed merge. - The temporary collection is left behind on failure, as before. The next MIGRATE already deletes a leftover temporary collection before it starts. - Unchanged: an async MIGRATE doesn't abort when a core admin request fails (`ShardRequestTracker.processResponses` only records the failure), so some failures still let it run to the end. # Tests Added `MigrateRouteKeyTest.testRetryAfterFailedMigrate`. It makes MIGRATE fail after the target leader started buffering: an alias refers to a leftover temporary collection, so MIGRATE can't delete it and then fails to create it. It does this once synchronously and once asynchronously, and after each checks that the target leader's update log is `ACTIVE` and the routing rule is gone. It then removes the alias, retries the MIGRATE, and checks that the target has exactly the migrated documents. Without the fix, both the synchronous and the asynchronous failure leave the target leader in `BUFFERING` (`expected:<ACTIVE> but was:<BUFFERING>`), and a retry fails with "MIGRATE failed to request node to buffer updates". With the fix, `MigrateRouteKeyTest` passes on 3 runs with different seeds. Also added tests for the new action's error handling to `CoreAdminOperationTest`. `CoreAdminOperationTest`, `CoreAdminHandlerTest` and `V2CoreAPIMappingTest` pass, and so does `./gradlew :solr:solrj:check :solr:core:check -x test`. # Checklist - [x] I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability. - [x] I have created a Jira issue and added the issue ID to my pull request title. - [x] I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation) - [x] I have developed this patch against the `main` branch. - [x] I have run `./gradlew check`. - [x] I have added tests for my changes. - [ ] I have added documentation for the Reference Guide - [x] I have added a changelog entry for my change -- 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]
