This is an automated email from the ASF dual-hosted git repository. dsmiley pushed a commit to branch branch_10x in repository https://gitbox.apache.org/repos/asf/solr.git
commit edc5da188a9ea28745b13c3cda9106dc1b4271a0 Author: Nick <[email protected]> AuthorDate: Mon Oct 5 04:46:06 2026 -0600 SOLR-18277: Restore parent shard state on failed split (#4959) (cherry picked from commit 0cc328310f8f4162572ea5bd5256cea29543e261) --- changelog/unreleased/SOLR-18277.yml | 7 + .../solr/cloud/api/collections/SplitShardCmd.java | 95 +++++----- .../solr/cloud/api/collections/ShardSplitTest.java | 2 - .../api/collections/SplitShardCmdCleanupTest.java | 207 +++++++++++++++++++++ 4 files changed, 256 insertions(+), 55 deletions(-) diff --git a/changelog/unreleased/SOLR-18277.yml b/changelog/unreleased/SOLR-18277.yml new file mode 100644 index 00000000000..9c870578aca --- /dev/null +++ b/changelog/unreleased/SOLR-18277.yml @@ -0,0 +1,7 @@ +title: Restore the parent shard to ACTIVE after a failed split even if cluster state is stale. +type: fixed +authors: + - name: Nick Shanin +links: + - name: SOLR-18277 + url: https://issues.apache.org/jira/browse/SOLR-18277 diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java index 4787e81734c..9bdf11435a6 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java @@ -33,11 +33,9 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.NoSuchElementException; -import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import org.apache.solr.client.solrj.cloud.DistribStateManager; @@ -164,7 +162,6 @@ public class SplitShardCmd implements CollApiCmds.CollectionApiCommand { zkStateReader.forceUpdateCollection(collectionName); AtomicReference<String> slice = new AtomicReference<>(); slice.set(message.getStr(ZkStateReader.SHARD_ID_PROP)); - Set<String> offlineSlices = new HashSet<>(); RTimerTree timings = new RTimerTree(); ClusterState clusterState = zkStateReader.getClusterState(); @@ -830,12 +827,7 @@ public class SplitShardCmd implements CollApiCmds.CollectionApiCommand { } finally { if (!success) { cleanupAfterFailure( - adminCmdContext, - zkStateReader, - collectionName, - parentSlice.getName(), - subSlices, - offlineSlices); + adminCmdContext, zkStateReader, collectionName, parentSlice.getName(), subSlices); unlockForSplit(ccc.getSolrCloudManager(), collectionName, parentSlice.getName()); } } @@ -939,8 +931,7 @@ public class SplitShardCmd implements CollApiCmds.CollectionApiCommand { ZkStateReader zkStateReader, String collectionName, String parentShard, - List<String> subSlices, - Set<String> offlineSlices) { + List<String> subSlices) { log.info("Cleaning up after a failed split of {}/{}", collectionName, parentShard); // get the latest state try { @@ -960,48 +951,8 @@ public class SplitShardCmd implements CollApiCmds.CollectionApiCommand { return; } - // If parent is inactive and all sub shards are active, then rolling back - // to make the parent active again will cause data loss. - if (coll.getSlice(parentShard).getState() == Slice.State.INACTIVE) { - boolean allSubSlicesActive = true; - for (String sub : subSlices) { - if (coll.getSlice(sub).getState() != Slice.State.ACTIVE) { - allSubSlicesActive = false; - break; - } - } - if (allSubSlicesActive) { - return; - } - } - - // set already created sub shards states to CONSTRUCTION - this prevents them - // from entering into RECOVERY or ACTIVE (SOLR-9455) - final Map<String, Object> propMap = new HashMap<>(); - boolean sendUpdateState = false; - propMap.put(Overseer.QUEUE_OPERATION, OverseerAction.UPDATESHARDSTATE.toLower()); - propMap.put(ZkStateReader.COLLECTION_PROP, collectionName); - for (Slice s : coll.getSlices()) { - if (!subSlices.contains(s.getName())) { - continue; - } - propMap.put(s.getName(), Slice.State.CONSTRUCTION.toString()); - sendUpdateState = true; - } - - // if parent is inactive activate it again - Slice parentSlice = coll.getSlice(parentShard); - if (parentSlice.getState() == Slice.State.INACTIVE) { - sendUpdateState = true; - propMap.put(parentShard, Slice.State.ACTIVE.toString()); - } - // plus any other previously deactivated slices - for (String sliceName : offlineSlices) { - propMap.put(sliceName, Slice.State.ACTIVE.toString()); - sendUpdateState = true; - } - - if (sendUpdateState) { + Map<String, Object> propMap = buildCleanupShardStateUpdates(coll, parentShard, subSlices); + if (propMap != null) { try { ZkNodeProps m = new ZkNodeProps(propMap); if (ccc.getDistributedClusterStateUpdater().isDistributedStateUpdate()) { @@ -1053,6 +1004,44 @@ public class SplitShardCmd implements CollApiCmds.CollectionApiCommand { } } + /** + * Returns the changes needed to undo a failed split, or {@code null} if the split has already + * finished. When it returns changes, they set the parent shard to {@code ACTIVE}, even if the + * cluster state has changed since it was read. + */ + static Map<String, Object> buildCleanupShardStateUpdates( + DocCollection coll, String parentShard, List<String> subSlices) { + // If parent is inactive and all sub shards are active, then rolling back + // to make the parent active again will cause data loss. + if (coll.getSlice(parentShard).getState() == Slice.State.INACTIVE) { + boolean allSubSlicesActive = true; + for (String sub : subSlices) { + if (coll.getSlice(sub).getState() != Slice.State.ACTIVE) { + allSubSlicesActive = false; + break; + } + } + if (allSubSlicesActive) { + return null; + } + } + + // set already created sub shards states to CONSTRUCTION - this prevents them + // from entering into RECOVERY or ACTIVE (SOLR-9455) + final Map<String, Object> propMap = new HashMap<>(); + propMap.put(Overseer.QUEUE_OPERATION, OverseerAction.UPDATESHARDSTATE.toLower()); + propMap.put(ZkStateReader.COLLECTION_PROP, coll.getName()); + for (Slice s : coll.getSlices()) { + if (!subSlices.contains(s.getName())) { + continue; + } + propMap.put(s.getName(), Slice.State.CONSTRUCTION.toString()); + } + // Always pin parent ACTIVE; a stale snapshot can miss an in-flight INACTIVE switch. + propMap.put(parentShard, Slice.State.ACTIVE.toString()); + return propMap; + } + public static Slice getParentSlice( ClusterState clusterState, String collectionName, diff --git a/solr/core/src/test/org/apache/solr/cloud/api/collections/ShardSplitTest.java b/solr/core/src/test/org/apache/solr/cloud/api/collections/ShardSplitTest.java index a366a849a42..d421e793cff 100644 --- a/solr/core/src/test/org/apache/solr/cloud/api/collections/ShardSplitTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/api/collections/ShardSplitTest.java @@ -328,7 +328,6 @@ public class ShardSplitTest extends BasicDistributedZkTest { * <p>See SOLR-9439 */ @Test - @AwaitsFix(bugUrl = "https://issues.apache.org/jira/browse/SOLR-18277";) public void testSplitAfterFailedSplit() throws Exception { waitForThingsToLevelOut(15, TimeUnit.SECONDS); @@ -384,7 +383,6 @@ public class ShardSplitTest extends BasicDistributedZkTest { @Test @Nightly - @AwaitsFix(bugUrl = "https://issues.apache.org/jira/browse/SOLR-18277";) public void testSplitAfterFailedSplit2() throws Exception { waitForThingsToLevelOut(15, TimeUnit.SECONDS); diff --git a/solr/core/src/test/org/apache/solr/cloud/api/collections/SplitShardCmdCleanupTest.java b/solr/core/src/test/org/apache/solr/cloud/api/collections/SplitShardCmdCleanupTest.java new file mode 100644 index 00000000000..f399e9501ed --- /dev/null +++ b/solr/core/src/test/org/apache/solr/cloud/api/collections/SplitShardCmdCleanupTest.java @@ -0,0 +1,207 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.cloud.api.collections; + +import static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.time.Instant; +import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import org.apache.solr.SolrTestCase; +import org.apache.solr.client.solrj.cloud.DistribStateManager; +import org.apache.solr.client.solrj.cloud.SolrCloudManager; +import org.apache.solr.cloud.Overseer; +import org.apache.solr.cloud.overseer.OverseerAction; +import org.apache.solr.cloud.overseer.SliceMutator; +import org.apache.solr.cloud.overseer.ZkWriteCommand; +import org.apache.solr.common.cloud.ClusterState; +import org.apache.solr.common.cloud.DocCollection; +import org.apache.solr.common.cloud.DocRouter; +import org.apache.solr.common.cloud.Slice; +import org.apache.solr.common.cloud.ZkNodeProps; +import org.apache.solr.common.cloud.ZkStateReader; +import org.apache.solr.common.util.TimeSource; +import org.apache.solr.handler.admin.ConfigSetsHandler; +import org.junit.BeforeClass; +import org.junit.Test; + +/** In-memory coverage for {@link SplitShardCmd} failed-split state rollback. */ +public class SplitShardCmdCleanupTest extends SolrTestCase { + + private static final String COLLECTION = "collection1"; + private static final String PARENT = "shard1"; + private static final String CHILD_0 = "shard1_0"; + private static final String CHILD_1 = "shard1_1"; + private static final List<String> CHILDREN = List.of(CHILD_0, CHILD_1); + + @BeforeClass + public static void beforeClass() { + assumeWorkingMockito(); + } + + @Test + public void testCleanupIncludesParentWhenSnapshotStillShowsActive() { + DocCollection coll = + collection( + slice(PARENT, Slice.State.ACTIVE), + slice(CHILD_0, Slice.State.CONSTRUCTION), + slice(CHILD_1, Slice.State.RECOVERY)); + + Map<String, Object> updates = + SplitShardCmd.buildCleanupShardStateUpdates(coll, PARENT, CHILDREN); + + assertNotNull(updates); + assertEquals(Slice.State.ACTIVE.toString(), updates.get(PARENT)); + assertEquals(Slice.State.CONSTRUCTION.toString(), updates.get(CHILD_0)); + assertEquals(Slice.State.CONSTRUCTION.toString(), updates.get(CHILD_1)); + } + + @Test + public void testCleanupRestoresInactiveParentWhenChildrenAreNotAllActive() { + DocCollection coll = + collection( + slice(PARENT, Slice.State.INACTIVE), + slice(CHILD_0, Slice.State.ACTIVE), + slice(CHILD_1, Slice.State.CONSTRUCTION)); + + Map<String, Object> updates = + SplitShardCmd.buildCleanupShardStateUpdates(coll, PARENT, CHILDREN); + + assertNotNull(updates); + assertEquals(Slice.State.ACTIVE.toString(), updates.get(PARENT)); + assertEquals(Slice.State.CONSTRUCTION.toString(), updates.get(CHILD_0)); + assertEquals(Slice.State.CONSTRUCTION.toString(), updates.get(CHILD_1)); + } + + @Test + public void testCleanupSkipsCommittedSwitchOver() { + DocCollection coll = + collection( + slice(PARENT, Slice.State.INACTIVE), + slice(CHILD_0, Slice.State.ACTIVE), + slice(CHILD_1, Slice.State.ACTIVE)); + + assertNull(SplitShardCmd.buildCleanupShardStateUpdates(coll, PARENT, CHILDREN)); + } + + @Test + public void testStaleCleanupWithoutParentLeavesParentInactive() { + ClusterState afterSuccess = apply(initialState(), successSwitch()); + assertSame( + Slice.State.INACTIVE, afterSuccess.getCollection(COLLECTION).getSlice(PARENT).getState()); + assertSame( + Slice.State.ACTIVE, afterSuccess.getCollection(COLLECTION).getSlice(CHILD_0).getState()); + + ClusterState afterStaleCleanup = apply(afterSuccess, staleCleanupWithoutParent()); + assertSame( + "omitting parent=active after the success switch is the race", + Slice.State.INACTIVE, + afterStaleCleanup.getCollection(COLLECTION).getSlice(PARENT).getState()); + assertSame( + Slice.State.CONSTRUCTION, + afterStaleCleanup.getCollection(COLLECTION).getSlice(CHILD_0).getState()); + } + + @Test + public void testFixedCleanupRestoresParentAfterQueuedSuccessSwitch() { + DocCollection staleSnapshot = initialState().getCollection(COLLECTION); + Map<String, Object> cleanup = + SplitShardCmd.buildCleanupShardStateUpdates(staleSnapshot, PARENT, CHILDREN); + assertNotNull(cleanup); + assertEquals(Slice.State.ACTIVE.toString(), cleanup.get(PARENT)); + + ClusterState afterSuccess = apply(initialState(), successSwitch()); + ClusterState afterCleanup = apply(afterSuccess, cleanup); + + assertSame( + Slice.State.ACTIVE, afterCleanup.getCollection(COLLECTION).getSlice(PARENT).getState()); + assertSame( + Slice.State.CONSTRUCTION, + afterCleanup.getCollection(COLLECTION).getSlice(CHILD_0).getState()); + assertSame( + Slice.State.CONSTRUCTION, + afterCleanup.getCollection(COLLECTION).getSlice(CHILD_1).getState()); + } + + private static ClusterState initialState() { + return cluster( + collection( + slice(PARENT, Slice.State.ACTIVE), + slice(CHILD_0, Slice.State.CONSTRUCTION), + slice(CHILD_1, Slice.State.CONSTRUCTION))); + } + + private static Map<String, Object> successSwitch() { + Map<String, Object> props = baseUpdate(); + props.put(PARENT, Slice.State.INACTIVE.toString()); + props.put(CHILD_0, Slice.State.ACTIVE.toString()); + props.put(CHILD_1, Slice.State.ACTIVE.toString()); + return props; + } + + private static Map<String, Object> staleCleanupWithoutParent() { + Map<String, Object> props = baseUpdate(); + props.put(CHILD_0, Slice.State.CONSTRUCTION.toString()); + props.put(CHILD_1, Slice.State.CONSTRUCTION.toString()); + return props; + } + + private static Map<String, Object> baseUpdate() { + Map<String, Object> props = new HashMap<>(); + props.put(Overseer.QUEUE_OPERATION, OverseerAction.UPDATESHARDSTATE.toLower()); + props.put(ZkStateReader.COLLECTION_PROP, COLLECTION); + return props; + } + + private static ClusterState apply(ClusterState state, Map<String, Object> update) { + SolrCloudManager cloudManager = mock(SolrCloudManager.class); + when(cloudManager.getDistribStateManager()).thenReturn(mock(DistribStateManager.class)); + when(cloudManager.getTimeSource()).thenReturn(TimeSource.NANO_TIME); + ZkWriteCommand cmd = + new SliceMutator(cloudManager).updateShardState(state, new ZkNodeProps(update)); + return new ClusterState(Set.of(), Map.of(COLLECTION, cmd.collection)); + } + + private static ClusterState cluster(DocCollection collection) { + return new ClusterState(Set.of(), Map.of(COLLECTION, collection)); + } + + private static Slice slice(String name, Slice.State state) { + return new Slice( + name, Map.of(), Map.of(ZkStateReader.STATE_PROP, state.toString()), COLLECTION); + } + + private static DocCollection collection(Slice... slices) { + Map<String, Slice> map = new LinkedHashMap<>(); + for (Slice slice : slices) { + map.put(slice.getName(), slice); + } + return DocCollection.create( + COLLECTION, + map, + Map.of(ZkStateReader.CONFIGNAME_PROP, ConfigSetsHandler.DEFAULT_CONFIGSET_NAME), + DocRouter.DEFAULT, + 1, + Instant.EPOCH, + null); + } +}
