diff --git a/changelog/unreleased/SOLR-18367-waitforfinalstate-default-true.yml b/changelog/unreleased/SOLR-18367-waitforfinalstate-default-true.yml new file mode 100644 index 00000000000..cb8f1d459e7 --- /dev/null +++ b/changelog/unreleased/SOLR-18367-waitforfinalstate-default-true.yml @@ -0,0 +1,16 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc +title: > + `waitForFinalState` now defaults to `true` for CREATE, ADDREPLICA, CREATESHARD, SPLITSHARD, and + MOVEREPLICA, so these Collections API commands wait for affected replicas to become active before + returning. BALANCE_REPLICAS, MIGRATE_REPLICAS, and REPLACENODE keep the previous `false` default, + since they can each affect an arbitrary number of replicas cluster-wide. Set the system property + `solr.cloud.waitForFinalState.enabled` on a Solr node to override the default (either direction) + cluster-wide, for all 8 commands. +type: changed +authors: + - name: Serhiy Bzhezytskyy +links: + - name: SOLR-18367 + url: https://issues.apache.org/jira/browse/SOLR-18367 + - name: SOLR-17712 + url: https://issues.apache.org/jira/browse/SOLR-17712 diff --git a/solr/api/src/java/org/apache/solr/client/api/model/BalanceReplicasRequestBody.java b/solr/api/src/java/org/apache/solr/client/api/model/BalanceReplicasRequestBody.java index 4ebb3a317e2..d7458d16df0 100644 --- a/solr/api/src/java/org/apache/solr/client/api/model/BalanceReplicasRequestBody.java +++ b/solr/api/src/java/org/apache/solr/client/api/model/BalanceReplicasRequestBody.java @@ -24,9 +24,8 @@ public class BalanceReplicasRequestBody { public BalanceReplicasRequestBody() {} - public BalanceReplicasRequestBody(Set nodes, Boolean waitForFinalState, String async) { + public BalanceReplicasRequestBody(Set nodes, String async) { this.nodes = nodes; - this.waitForFinalState = waitForFinalState; this.async = async; } @@ -36,20 +35,6 @@ public BalanceReplicasRequestBody(Set nodes, Boolean waitForFinalState, @JsonProperty(value = "nodes") public Set nodes; - /** - * @deprecated Solr is moving toward always waiting for final state, with no option to opt out; - * once that happens, this parameter will have no effect and will likely be removed. See - * SOLR-17712. - */ - @Schema( - description = - "If true, the request will complete only when all affected replicas become active. " - + "If false, the API will return the status of the single action, which may be " - + "before the new replica is online and active.") - @JsonProperty("waitForFinalState") - @Deprecated(since = "9.10") - public Boolean waitForFinalState = false; - @Schema(description = "Request ID to track this action which will be processed asynchronously.") @JsonProperty("async") public String async; diff --git a/solr/api/src/java/org/apache/solr/client/api/model/MigrateReplicasRequestBody.java b/solr/api/src/java/org/apache/solr/client/api/model/MigrateReplicasRequestBody.java index 9622559cb54..a4e00a59561 100644 --- a/solr/api/src/java/org/apache/solr/client/api/model/MigrateReplicasRequestBody.java +++ b/solr/api/src/java/org/apache/solr/client/api/model/MigrateReplicasRequestBody.java @@ -25,10 +25,9 @@ public class MigrateReplicasRequestBody { public MigrateReplicasRequestBody() {} public MigrateReplicasRequestBody( - Set sourceNodes, Set targetNodes, Boolean waitForFinalState, String async) { + Set sourceNodes, Set targetNodes, String async) { this.sourceNodes = sourceNodes; this.targetNodes = targetNodes; - this.waitForFinalState = waitForFinalState; this.async = async; } @@ -42,20 +41,6 @@ public MigrateReplicasRequestBody( @JsonProperty public Set targetNodes; - /** - * @deprecated Solr is moving toward always waiting for final state, with no option to opt out; - * once that happens, this parameter will have no effect and will likely be removed. See - * SOLR-17712. - */ - @Schema( - description = - "If true, the request will complete only when all affected replicas become active. " - + "If false, the API will return the status of the single action, which may be " - + "before the new replicas are online and active.") - @JsonProperty - @Deprecated(since = "9.10") - public Boolean waitForFinalState = false; - @Schema(description = "Request ID to track this action which will be processed asynchronously.") @JsonProperty public String async; diff --git a/solr/api/src/java/org/apache/solr/client/api/model/ReplaceNodeRequestBody.java b/solr/api/src/java/org/apache/solr/client/api/model/ReplaceNodeRequestBody.java index f3f49a876f8..0ee145c918d 100644 --- a/solr/api/src/java/org/apache/solr/client/api/model/ReplaceNodeRequestBody.java +++ b/solr/api/src/java/org/apache/solr/client/api/model/ReplaceNodeRequestBody.java @@ -23,9 +23,8 @@ public class ReplaceNodeRequestBody { public ReplaceNodeRequestBody() {} - public ReplaceNodeRequestBody(String targetNodeName, Boolean waitForFinalState, String async) { + public ReplaceNodeRequestBody(String targetNodeName, String async) { this.targetNodeName = targetNodeName; - this.waitForFinalState = waitForFinalState; this.async = async; } @@ -36,20 +35,6 @@ public ReplaceNodeRequestBody(String targetNodeName, Boolean waitForFinalState, @JsonProperty("targetNodeName") public String targetNodeName; - /** - * @deprecated Solr is moving toward always waiting for final state, with no option to opt out; - * once that happens, this parameter will have no effect and will likely be removed. See - * SOLR-17712. - */ - @Schema( - description = - "If true, the request will complete only when all affected replicas become active. " - + "If false, the API will return the status of the single action, which may be " - + "before the new replica is online and active.") - @JsonProperty("waitForFinalState") - @Deprecated(since = "9.10") - public Boolean waitForFinalState = false; - @Schema(description = "Request ID to track this action which will be processed asynchronously.") @JsonProperty("async") public String async; diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/AddReplicaCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/AddReplicaCmd.java index 162dc555a8a..40db1a35ee5 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/AddReplicaCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/AddReplicaCmd.java @@ -27,6 +27,7 @@ import static org.apache.solr.common.params.CollectionParams.CollectionAction.ADDREPLICA; import static org.apache.solr.common.params.CommonAdminParams.TIMEOUT; import static org.apache.solr.common.params.CommonAdminParams.WAIT_FOR_FINAL_STATE; +import static org.apache.solr.common.params.CommonAdminParams.WAIT_FOR_FINAL_STATE_DEFAULT_PROP; import java.io.IOException; import java.lang.invoke.MethodHandles; @@ -113,7 +114,9 @@ List addReplica( "Collection: " + collectionName + " shard: " + shard + " does not exist"); } - boolean waitForFinalState = message.getBool(WAIT_FOR_FINAL_STATE, false); + boolean waitForFinalState = + CollectionHandlingUtils.getBoolWithEnvFallback( + message, WAIT_FOR_FINAL_STATE, WAIT_FOR_FINAL_STATE_DEFAULT_PROP, false); boolean skipCreateReplicaInClusterState = message.getBool(SKIP_CREATE_REPLICA_IN_CLUSTER_STATE, false); diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/BalanceReplicasCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/BalanceReplicasCmd.java index 743d9f5847f..6075de80318 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/BalanceReplicasCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/BalanceReplicasCmd.java @@ -55,7 +55,12 @@ public void call(AdminCmdContext adminCmdContext, ZkNodeProps message, NamedList "'nodes' was not passed as a correct type (Set/List/String): " + nodesRaw.getClass().getName()); } - boolean waitForFinalState = message.getBool(CommonAdminParams.WAIT_FOR_FINAL_STATE, false); + boolean waitForFinalState = + CollectionHandlingUtils.getBoolWithEnvFallback( + message, + CommonAdminParams.WAIT_FOR_FINAL_STATE, + CommonAdminParams.WAIT_FOR_FINAL_STATE_DEFAULT_PROP, + false); int timeout = message.getInt("timeout", 10 * 60); // 10 minutes boolean parallel = message.getBool("parallel", false); diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionHandlingUtils.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionHandlingUtils.java index 02320e1e854..d7dbde98f7b 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionHandlingUtils.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionHandlingUtils.java @@ -61,6 +61,7 @@ import org.apache.solr.common.params.CollectionAdminParams; import org.apache.solr.common.params.CoreAdminParams; import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.solr.common.util.EnvUtils; import org.apache.solr.common.util.NamedList; import org.apache.solr.common.util.SimpleOrderedMap; import org.apache.solr.common.util.StrUtils; @@ -140,6 +141,17 @@ public static EnumSet leaderEligibleReplicaTypes() { .collect(Collectors.toCollection(() -> EnumSet.noneOf(Replica.Type.class))); } + /** + * Reads a boolean request param, falling back to a node-level system property when the request + * doesn't specify one, and finally to {@code defaultValue} when neither is set. Lets an operator + * override a per-request default cluster-wide (e.g. via {@code -D=false} at node + * startup) without a client change -- same shape as {@code CreateCollectionCmd.PRS_DEFAULT_PROP}. + */ + static boolean getBoolWithEnvFallback( + ZkNodeProps message, String messageParam, String envProp, boolean defaultValue) { + return message.getBool(messageParam, EnvUtils.getPropertyAsBool(envProp, defaultValue)); + } + static boolean waitForCoreNodeGone( String collectionName, String shard, diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java index 14291d7b941..70705233548 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java @@ -24,6 +24,7 @@ import static org.apache.solr.common.params.CollectionParams.CollectionAction.DELETE; import static org.apache.solr.common.params.CommonAdminParams.ASYNC; import static org.apache.solr.common.params.CommonAdminParams.WAIT_FOR_FINAL_STATE; +import static org.apache.solr.common.params.CommonAdminParams.WAIT_FOR_FINAL_STATE_DEFAULT_PROP; import static org.apache.solr.common.params.CommonParams.NAME; import static org.apache.solr.common.util.StrUtils.formatString; import static org.apache.solr.handler.admin.ConfigSetsHandler.DEFAULT_CONFIGSET_NAME; @@ -111,7 +112,9 @@ public void call(AdminCmdContext adminCmdContext, ZkNodeProps message, NamedList ClusterState clusterState = adminCmdContext.getClusterState(); final Aliases aliases = ccc.getZkStateReader().getAliases(); final String collectionName = message.getStr(NAME); - final boolean waitForFinalState = message.getBool(WAIT_FOR_FINAL_STATE, false); + final boolean waitForFinalState = + CollectionHandlingUtils.getBoolWithEnvFallback( + message, WAIT_FOR_FINAL_STATE, WAIT_FOR_FINAL_STATE_DEFAULT_PROP, true); final String alias = message.getStr(ALIAS, collectionName); log.info("Create collection {}", collectionName); boolean prsDefault = EnvUtils.getPropertyAsBool(PRS_DEFAULT_PROP, false); diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/CreateShardCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/CreateShardCmd.java index 7c0e1633603..ae95885a2fd 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/CreateShardCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/CreateShardCmd.java @@ -50,7 +50,12 @@ public void call(AdminCmdContext adminCmdContext, ZkNodeProps message, NamedList throws Exception { String extCollectionName = message.getStr(COLLECTION_PROP); String sliceName = message.getStr(SHARD_ID_PROP); - boolean waitForFinalState = message.getBool(CommonAdminParams.WAIT_FOR_FINAL_STATE, false); + boolean waitForFinalState = + CollectionHandlingUtils.getBoolWithEnvFallback( + message, + CommonAdminParams.WAIT_FOR_FINAL_STATE, + CommonAdminParams.WAIT_FOR_FINAL_STATE_DEFAULT_PROP, + true); log.info("Create shard invoked: {}", message); if (extCollectionName == null || sliceName == null) diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/MigrateCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/MigrateCmd.java index 2869f9866b6..1f0771e01cf 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/MigrateCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/MigrateCmd.java @@ -46,6 +46,7 @@ import org.apache.solr.common.cloud.ZkNodeProps; import org.apache.solr.common.cloud.ZkStateReader; import org.apache.solr.common.params.CollectionAdminParams; +import org.apache.solr.common.params.CommonAdminParams; import org.apache.solr.common.params.CoreAdminParams; import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.solr.common.util.NamedList; @@ -308,7 +309,11 @@ private void migrateKey( CollectionAdminParams.COLL_CONF, configName, CollectionHandlingUtils.CREATE_NODE_SET, - sourceLeader.getNodeName()); + sourceLeader.getNodeName(), + // the getLeaderRetry(...) call below is this method's own wait; don't let + // CreateCollectionCmd's own wait run (and block on) first. + CommonAdminParams.WAIT_FOR_FINAL_STATE, + "false"); String internalAsyncId = null; if (adminCmdContext.getAsyncId() != null) { internalAsyncId = adminCmdContext.getAsyncId() + Math.abs(System.nanoTime()); @@ -400,6 +405,9 @@ private void migrateKey( props.put(SHARD_ID_PROP, tempSourceSlice.getName()); props.put("node", targetLeader.getNodeName()); props.put(CoreAdminParams.NAME, tempCollectionReplica2); + // the syncRequestTracker below is this method's own wait; don't let AddReplicaCmd's own + // wait run (and block on) first. + props.put(CommonAdminParams.WAIT_FOR_FINAL_STATE, "false"); // copy over property params: for (String key : message.keySet()) { if (key.startsWith(CollectionAdminParams.PROPERTY_PREFIX)) { diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/MigrateReplicasCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/MigrateReplicasCmd.java index 779ab999197..281fe54847b 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/MigrateReplicasCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/MigrateReplicasCmd.java @@ -50,7 +50,12 @@ public void call(AdminCmdContext adminCmdContext, ZkNodeProps message, NamedList ZkStateReader zkStateReader = ccc.getZkStateReader(); Set sourceNodes = getNodesFromParam(message, CollectionParams.SOURCE_NODES); Set targetNodes = getNodesFromParam(message, CollectionParams.TARGET_NODES); - boolean waitForFinalState = message.getBool(CommonAdminParams.WAIT_FOR_FINAL_STATE, false); + boolean waitForFinalState = + CollectionHandlingUtils.getBoolWithEnvFallback( + message, + CommonAdminParams.WAIT_FOR_FINAL_STATE, + CommonAdminParams.WAIT_FOR_FINAL_STATE_DEFAULT_PROP, + false); if (sourceNodes.isEmpty()) { throw new SolrException( SolrException.ErrorCode.BAD_REQUEST, "sourceNodes is a required param"); diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/MoveReplicaCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/MoveReplicaCmd.java index 635bae1e682..a9bbf0f61fd 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/MoveReplicaCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/MoveReplicaCmd.java @@ -25,6 +25,7 @@ import static org.apache.solr.common.params.CommonAdminParams.IN_PLACE_MOVE; import static org.apache.solr.common.params.CommonAdminParams.TIMEOUT; import static org.apache.solr.common.params.CommonAdminParams.WAIT_FOR_FINAL_STATE; +import static org.apache.solr.common.params.CommonAdminParams.WAIT_FOR_FINAL_STATE_DEFAULT_PROP; import java.lang.invoke.MethodHandles; import java.util.ArrayList; @@ -72,7 +73,9 @@ private void moveReplica( CollectionHandlingUtils.checkRequired(message, COLLECTION_PROP, CollectionParams.TARGET_NODE); String extCollection = message.getStr(COLLECTION_PROP); String targetNode = message.getStr(CollectionParams.TARGET_NODE); - boolean waitForFinalState = message.getBool(WAIT_FOR_FINAL_STATE, false); + boolean waitForFinalState = + CollectionHandlingUtils.getBoolWithEnvFallback( + message, WAIT_FOR_FINAL_STATE, WAIT_FOR_FINAL_STATE_DEFAULT_PROP, false); boolean inPlaceMove = message.getBool(IN_PLACE_MOVE, true); int timeout = message.getInt(TIMEOUT, 10 * 60); // 10 minutes @@ -368,7 +371,11 @@ private void moveNormalReplica( CoreAdminParams.NAME, newCoreName, ZkStateReader.REPLICA_TYPE, - replica.getType().name()); + replica.getType().name(), + // this method has its own watcher/latch below, conditioned on waitForFinalState; + // don't let AddReplicaCmd's own wait run (and block on) first. + WAIT_FOR_FINAL_STATE, + "false"); NamedList addResult = new NamedList<>(); SolrCloseableLatch countDownLatch = new SolrCloseableLatch(1, ccc.getCloseableToLatchOn()); diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/ReplaceNodeCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/ReplaceNodeCmd.java index 26a3730ca10..3b61b0ccc35 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/ReplaceNodeCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/ReplaceNodeCmd.java @@ -47,7 +47,12 @@ public void call(AdminCmdContext adminCmdContext, ZkNodeProps message, NamedList ZkStateReader zkStateReader = ccc.getZkStateReader(); String source = message.getStr(CollectionParams.SOURCE_NODE); String target = message.getStr(CollectionParams.TARGET_NODE); - boolean waitForFinalState = message.getBool(CommonAdminParams.WAIT_FOR_FINAL_STATE, false); + boolean waitForFinalState = + CollectionHandlingUtils.getBoolWithEnvFallback( + message, + CommonAdminParams.WAIT_FOR_FINAL_STATE, + CommonAdminParams.WAIT_FOR_FINAL_STATE_DEFAULT_PROP, + false); if (source == null) { throw new SolrException( SolrException.ErrorCode.BAD_REQUEST, "sourceNode is a required param"); diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/ReplicaMigrationUtils.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/ReplicaMigrationUtils.java index be08e6fc573..79ee6677d84 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/ReplicaMigrationUtils.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/ReplicaMigrationUtils.java @@ -35,6 +35,7 @@ import org.apache.solr.common.cloud.ZkNodeProps; import org.apache.solr.common.cloud.ZkStateReader; import org.apache.solr.common.params.CollectionParams; +import org.apache.solr.common.params.CommonAdminParams; import org.apache.solr.common.params.CoreAdminParams; import org.apache.solr.common.util.NamedList; import org.apache.zookeeper.KeeperException; @@ -106,7 +107,10 @@ static boolean migrateReplicas( sourceReplica .toFullProps() .plus("parallel", String.valueOf(parallel)) - .plus(CoreAdminParams.NODE, targetNode); + .plus(CoreAdminParams.NODE, targetNode) + // this method has its own watcher/latch below, conditioned on waitForFinalState; + // don't let AddReplicaCmd's own wait run (and block on) first. + .plus(CommonAdminParams.WAIT_FOR_FINAL_STATE, "false"); NamedList nl = new NamedList<>(); final ZkNodeProps addedReplica = new AddReplicaCmd(ccc) diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/RestoreCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/RestoreCmd.java index dabaf64420e..89e1d3f3628 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/RestoreCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/RestoreCmd.java @@ -65,6 +65,7 @@ import org.apache.solr.common.cloud.ZkStateReader; import org.apache.solr.common.params.CollectionAdminParams; import org.apache.solr.common.params.CollectionParams; +import org.apache.solr.common.params.CommonAdminParams; import org.apache.solr.common.params.CoreAdminParams; import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.solr.common.util.CollectionUtil; @@ -358,6 +359,9 @@ private void createCoreLessCollection( propMap.put( CollectionHandlingUtils.CREATE_NODE_SET, CollectionHandlingUtils.CREATE_NODE_SET_EMPTY); // no cores + // no cores are created here (see above), but keep this call's contract independent of the + // operator-configurable default regardless -- restore does its own waiting elsewhere. + propMap.put(CommonAdminParams.WAIT_FOR_FINAL_STATE, "false"); propMap.put(CollectionAdminParams.COLL_CONF, restoreConfigName); // router.* @@ -452,6 +456,9 @@ private void createSingleReplicaPerShard( propMap.put(COLLECTION_PROP, restoreCollection.getName()); propMap.put(SHARD_ID_PROP, sliceName); propMap.put(REPLICA_TYPE, numReplicas.getLeaderType().name()); + // the onComplete callback below drives this method's own countDownLatch; don't let + // AddReplicaCmd's own wait run (and block on) first. + propMap.put(CommonAdminParams.WAIT_FOR_FINAL_STATE, "false"); // Get the first node matching the shard to restore in String node; @@ -567,6 +574,9 @@ private void addReplicasToShards( propMap.put(COLLECTION_PROP, restoreCollection.getName()); propMap.put(SHARD_ID_PROP, slice.getName()); propMap.put(REPLICA_TYPE, typeToCreate.name()); + // restore does its own waiting elsewhere; don't let AddReplicaCmd's own wait + // (bounded by its default 10-minute timeout, per shard) run here instead. + propMap.put(CommonAdminParams.WAIT_FOR_FINAL_STATE, "false"); // Get the first node matching the shard to restore in String node; 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 6740c5ba046..5f008251421 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 @@ -136,7 +136,12 @@ public void call(AdminCmdContext adminCmdContext, ZkNodeProps message, NamedList public boolean split( AdminCmdContext adminCmdContext, ZkNodeProps message, NamedList results) throws Exception { - boolean waitForFinalState = message.getBool(CommonAdminParams.WAIT_FOR_FINAL_STATE, false); + boolean waitForFinalState = + CollectionHandlingUtils.getBoolWithEnvFallback( + message, + CommonAdminParams.WAIT_FOR_FINAL_STATE, + CommonAdminParams.WAIT_FOR_FINAL_STATE_DEFAULT_PROP, + true); String methodStr = message.getStr( CommonAdminParams.SPLIT_METHOD, SolrIndexSplitter.SplitMethod.REWRITE.toLower()); diff --git a/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java b/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java index 270afc24906..fabd30e31b2 100644 --- a/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java +++ b/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java @@ -1164,7 +1164,6 @@ public Map execute( final RequiredSolrParams requiredParams = req.getParams().required(); final var requestBody = new ReplaceNodeRequestBody(); requestBody.targetNodeName = params.get(TARGET_NODE); - requestBody.waitForFinalState = params.getBool(WAIT_FOR_FINAL_STATE); requestBody.async = params.get(ASYNC); final ReplaceNode replaceNodeAPI = new ReplaceNode(h.coreContainer, req, rsp); final SolrJerseyResponse replaceNodeResponse = diff --git a/solr/core/src/java/org/apache/solr/handler/admin/api/BalanceReplicas.java b/solr/core/src/java/org/apache/solr/handler/admin/api/BalanceReplicas.java index 8ee0a7e4e77..b95f9aa7908 100644 --- a/solr/core/src/java/org/apache/solr/handler/admin/api/BalanceReplicas.java +++ b/solr/core/src/java/org/apache/solr/handler/admin/api/BalanceReplicas.java @@ -17,7 +17,6 @@ package org.apache.solr.handler.admin.api; import static org.apache.solr.common.params.CollectionParams.NODES; -import static org.apache.solr.common.params.CommonAdminParams.WAIT_FOR_FINAL_STATE; import static org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM; import jakarta.inject.Inject; @@ -65,7 +64,6 @@ public ZkNodeProps createRemoteMessage(BalanceReplicasRequestBody requestBody) { final Map remoteMessage = new HashMap<>(); if (requestBody != null) { insertIfNotNull(remoteMessage, NODES, requestBody.nodes); - insertIfNotNull(remoteMessage, WAIT_FOR_FINAL_STATE, requestBody.waitForFinalState); } return new ZkNodeProps(remoteMessage); diff --git a/solr/core/src/java/org/apache/solr/handler/admin/api/MigrateReplicas.java b/solr/core/src/java/org/apache/solr/handler/admin/api/MigrateReplicas.java index ad9b9604080..90c55c299f1 100644 --- a/solr/core/src/java/org/apache/solr/handler/admin/api/MigrateReplicas.java +++ b/solr/core/src/java/org/apache/solr/handler/admin/api/MigrateReplicas.java @@ -18,7 +18,6 @@ import static org.apache.solr.common.params.CollectionParams.SOURCE_NODES; import static org.apache.solr.common.params.CollectionParams.TARGET_NODES; -import static org.apache.solr.common.params.CommonAdminParams.WAIT_FOR_FINAL_STATE; import static org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM; import jakarta.inject.Inject; @@ -73,7 +72,6 @@ public ZkNodeProps createRemoteMessage(MigrateReplicasRequestBody requestBody) { } insertIfNotNull(remoteMessage, SOURCE_NODES, requestBody.sourceNodes); insertIfNotNull(remoteMessage, TARGET_NODES, requestBody.targetNodes); - insertIfNotNull(remoteMessage, WAIT_FOR_FINAL_STATE, requestBody.waitForFinalState); } else { throw new SolrException( SolrException.ErrorCode.BAD_REQUEST, diff --git a/solr/core/src/java/org/apache/solr/handler/admin/api/ReplaceNode.java b/solr/core/src/java/org/apache/solr/handler/admin/api/ReplaceNode.java index cd162659087..f2f20455235 100644 --- a/solr/core/src/java/org/apache/solr/handler/admin/api/ReplaceNode.java +++ b/solr/core/src/java/org/apache/solr/handler/admin/api/ReplaceNode.java @@ -18,7 +18,6 @@ import static org.apache.solr.common.params.CollectionParams.SOURCE_NODE; import static org.apache.solr.common.params.CollectionParams.TARGET_NODE; -import static org.apache.solr.common.params.CommonAdminParams.WAIT_FOR_FINAL_STATE; import static org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM; import jakarta.inject.Inject; @@ -71,7 +70,6 @@ public ZkNodeProps createRemoteMessage(String nodeName, ReplaceNodeRequestBody r remoteMessage.put(SOURCE_NODE, nodeName); if (requestBody != null) { insertIfValueNotNull(remoteMessage, TARGET_NODE, requestBody.targetNodeName); - insertIfValueNotNull(remoteMessage, WAIT_FOR_FINAL_STATE, requestBody.waitForFinalState); } return new ZkNodeProps(remoteMessage); diff --git a/solr/core/src/test/org/apache/solr/cloud/AddReplicaTest.java b/solr/core/src/test/org/apache/solr/cloud/AddReplicaTest.java index 8c0750635ca..67fe7bd0759 100644 --- a/solr/core/src/test/org/apache/solr/cloud/AddReplicaTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/AddReplicaTest.java @@ -155,8 +155,6 @@ public void test() throws Exception { replicas2.removeAll(replicas); assertEquals(1, replicas2.size()); - // use waitForFinalState - addReplica.setWaitForFinalState(true); addReplica.processAsync("001", cloudClient); requestStatus = CollectionAdminRequest.requestStatus("001"); rsp = requestStatus.process(cloudClient); @@ -205,7 +203,6 @@ public void testAddReplicaWithUserDefinedProperties() throws Exception { CollectionAdminRequest.addReplicaToShard(collectionName, "shard1"); addReplica.withProperty("customProp2", "val2.1"); addReplica.withProperty("customProp3", "val3"); - addReplica.setWaitForFinalState(true); addReplica.process(cloudClient); // Verify that the new core was created with user-defined properties coming from the request diff --git a/solr/core/src/test/org/apache/solr/cloud/BalanceReplicasTest.java b/solr/core/src/test/org/apache/solr/cloud/BalanceReplicasTest.java index 79a8a80e0a4..96290c421ef 100644 --- a/solr/core/src/test/org/apache/solr/cloud/BalanceReplicasTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/BalanceReplicasTest.java @@ -140,7 +140,14 @@ public void testSomeNodes() throws Exception { postDataAndGetResponse( "/api/cluster/replicas/balance", Utils.getReflectWriter( - new BalanceReplicasRequestBody(new HashSet<>(l.subList(1, 4)), true, null))); + new BalanceReplicasRequestBody(new HashSet<>(l.subList(1, 4)), null))); + + waitForState( + "Timed out waiting for replicas to be balanced across the target nodes", + coll, + (state) -> + state.replicaStream().map(Replica::getNodeName).collect(Collectors.toSet()).size() + == 4); collection = cloudClient.getClusterState().getCollectionOrNull(coll, false); log.debug("### After balancing: {}", collection); diff --git a/solr/core/src/test/org/apache/solr/cloud/CollectionsAPISolrJTest.java b/solr/core/src/test/org/apache/solr/cloud/CollectionsAPISolrJTest.java index 44c24de9b10..846f5c28c77 100644 --- a/solr/core/src/test/org/apache/solr/cloud/CollectionsAPISolrJTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/CollectionsAPISolrJTest.java @@ -232,7 +232,6 @@ public void testCreateAndDeleteCollection() throws Exception { String collectionName = getSaferTestName(); CollectionAdminRequest.Create createREq = CollectionAdminRequest.createCollection(collectionName, "conf", 2, 2); - createREq.setWaitForFinalState(false); CollectionAdminResponse response = createREq.process(cluster.getSolrClient()); assertEquals(0, response.getStatus()); diff --git a/solr/core/src/test/org/apache/solr/cloud/MigrateReplicasTest.java b/solr/core/src/test/org/apache/solr/cloud/MigrateReplicasTest.java index 1d4d431d6df..fc5291daafb 100644 --- a/solr/core/src/test/org/apache/solr/cloud/MigrateReplicasTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/MigrateReplicasTest.java @@ -117,11 +117,15 @@ public void test() throws Exception { Map response = callMigrateReplicas( new MigrateReplicasRequestBody( - Set.of(nodeToBeDecommissioned), Set.of(emptyNode), true, null)); + Set.of(nodeToBeDecommissioned), Set.of(emptyNode), null)); assertEquals( "MigrateReplicas request was unsuccessful", 0L, ((Map) response.get("responseHeader")).get("status")); + waitForState( + "Timed out waiting for replicas to be migrated off the decommissioned node", + coll, + (state) -> state.getReplicasOnNode(nodeToBeDecommissioned).isEmpty()); ZkStateReader zkStateReader = ZkStateReader.from(cloudClient); try (SolrClient coreClient = getHttpSolrClient(zkStateReader.getBaseUrlForNodeName(nodeToBeDecommissioned))) { @@ -144,11 +148,18 @@ public void test() throws Exception { response = callMigrateReplicas( new MigrateReplicasRequestBody( - Set.of(emptyNode), Set.of(nodeToBeDecommissioned), true, null)); + Set.of(emptyNode), Set.of(nodeToBeDecommissioned), null)); assertEquals( "MigrateReplicas request was unsuccessful", 0L, ((Map) response.get("responseHeader")).get("status")); + waitForState( + "Timed out waiting for replicas to be migrated back off the empty node and become active", + coll, + (state) -> + state.getReplicasOnNode(emptyNode).isEmpty() + && state.getReplicasOnNode(nodeToBeDecommissioned).stream() + .allMatch(r -> r.getState() == Replica.State.ACTIVE)); try (SolrClient coreClient = getHttpSolrClient(zkStateReader.getBaseUrlForNodeName(emptyNode))) { @@ -250,12 +261,17 @@ public void testWithNoTarget() throws Exception { log.info("### Before decommission: {}", initialCollection); Map response = callMigrateReplicas( - new MigrateReplicasRequestBody( - new HashSet<>(nodesToBeDecommissioned), Set.of(), true, null)); + new MigrateReplicasRequestBody(new HashSet<>(nodesToBeDecommissioned), Set.of(), null)); assertEquals( "MigrateReplicas request was unsuccessful", 0L, ((Map) response.get("responseHeader")).get("status")); + waitForState( + "Timed out waiting for replicas to be migrated off the decommissioned nodes", + coll, + (state) -> + nodesToBeDecommissioned.stream() + .allMatch(node -> state.getReplicasOnNode(node).isEmpty())); DocCollection collection = cloudClient.getClusterState().getCollectionOrNull(coll, false); assertNotNull("Collection cannot be null: " + coll, collection); @@ -294,7 +310,7 @@ public void testFailOnSingleNode() throws Exception { String liveNode = cloudClient.getClusterState().getLiveNodes().iterator().next(); Map response = - callMigrateReplicas(new MigrateReplicasRequestBody(Set.of(liveNode), Set.of(), true, null)); + callMigrateReplicas(new MigrateReplicasRequestBody(Set.of(liveNode), Set.of(), null)); assertNotNull( "No error in response, when the request should have failed", response.get("error")); assertEquals( diff --git a/solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java b/solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java index bc64635d2d8..a7dd392c9b6 100644 --- a/solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java @@ -133,7 +133,6 @@ public void test() throws Exception { // let's do it back - this time wait for recoveries CollectionAdminRequest.AsyncCollectionAdminRequest replaceNodeRequest = createReplaceNodeRequest(emptyNode, nodeToBeDecommissioned, Boolean.TRUE); - replaceNodeRequest.setWaitForFinalState(true); replaceNodeRequest.processAndWait("001", cloudClient, 10); try (SolrClient coreClient = diff --git a/solr/core/src/test/org/apache/solr/cloud/SplitShardTest.java b/solr/core/src/test/org/apache/solr/cloud/SplitShardTest.java index 0b539bd39d0..7729183cb26 100644 --- a/solr/core/src/test/org/apache/solr/cloud/SplitShardTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/SplitShardTest.java @@ -192,7 +192,6 @@ public void testWithChildDocuments() throws Exception { CollectionAdminRequest.splitShard(COLLECTION_NAME) .setNumSubShards(2) .setShardName("shard1"); - splitShard.setWaitForFinalState(true); splitShard.process(solrClient); waitForState( "Waiting for 2 active shards after split", COLLECTION_NAME, activeClusterShape(2, 2)); diff --git a/solr/core/src/test/org/apache/solr/cloud/SplitShardWithNodeRoleTest.java b/solr/core/src/test/org/apache/solr/cloud/SplitShardWithNodeRoleTest.java index eb15bd44420..9530bb34ca0 100644 --- a/solr/core/src/test/org/apache/solr/cloud/SplitShardWithNodeRoleTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/SplitShardWithNodeRoleTest.java @@ -21,8 +21,15 @@ import java.util.Set; import org.apache.solr.client.solrj.impl.CloudSolrClient; import org.apache.solr.client.solrj.request.CollectionAdminRequest; +import org.apache.solr.client.solrj.request.CollectionAdminRequest.AsyncCollectionAdminRequest; import org.apache.solr.client.solrj.request.UpdateRequest; import org.apache.solr.common.SolrInputDocument; +import org.apache.solr.common.params.CollectionAdminParams; +import org.apache.solr.common.params.CollectionParams.CollectionAction; +import org.apache.solr.common.params.CommonAdminParams; +import org.apache.solr.common.params.CoreAdminParams; +import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.solr.common.params.SolrParams; import org.apache.solr.core.NodeRoles; import org.apache.solr.util.LogLevel; import org.junit.BeforeClass; @@ -85,10 +92,20 @@ public void doSplit(String collName, int shard, int nrtReplica, int pullReplica) ur.commit(client, collName); final int numSubShards = 2; - CollectionAdminRequest.SplitShard splitShard = - CollectionAdminRequest.splitShard(collName) - .setShardName("shard1") - .setNumSubShards(numSubShards); + // this method does its own explicit waitForState(...) below; pin waitForFinalState=false + // so SPLITSHARD's own new default doesn't also wait (and risk the client's HTTP timeout). + AsyncCollectionAdminRequest splitShard = + new AsyncCollectionAdminRequest(CollectionAction.SPLITSHARD) { + @Override + public SolrParams getParams() { + ModifiableSolrParams params = (ModifiableSolrParams) super.getParams(); + params.set(CollectionAdminParams.COLLECTION, collName); + params.set(CoreAdminParams.SHARD, "shard1"); + params.set("numSubShards", numSubShards); + params.set(CommonAdminParams.WAIT_FOR_FINAL_STATE, false); + return params; + } + }; splitShard.process(cluster.getSolrClient()); int totalShards = shard + (numSubShards - 1); waitForState( diff --git a/solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionHandlingUtilsTest.java b/solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionHandlingUtilsTest.java new file mode 100644 index 00000000000..ee99621b235 --- /dev/null +++ b/solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionHandlingUtilsTest.java @@ -0,0 +1,73 @@ +/* + * 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 java.util.Map; +import org.apache.solr.SolrTestCase; +import org.apache.solr.common.cloud.ZkNodeProps; +import org.junit.After; +import org.junit.Test; + +public class CollectionHandlingUtilsTest extends SolrTestCase { + + private static final String MESSAGE_PARAM = "waitForFinalState"; + private static final String ENV_PROP = "solr.cloud.waitForFinalStateEnvFallbackTest.enabled"; + + @After + public void clearProperty() { + System.clearProperty(ENV_PROP); + } + + @Test + public void testMessageUnsetEnvUnsetFallsBackToDefault() { + ZkNodeProps message = new ZkNodeProps(Map.of()); + assertTrue( + CollectionHandlingUtils.getBoolWithEnvFallback(message, MESSAGE_PARAM, ENV_PROP, true)); + assertFalse( + CollectionHandlingUtils.getBoolWithEnvFallback(message, MESSAGE_PARAM, ENV_PROP, false)); + } + + @Test + public void testMessageUnsetEnvSetFalseOverridesDefaultTrue() { + System.setProperty(ENV_PROP, "false"); + ZkNodeProps message = new ZkNodeProps(Map.of()); + assertFalse( + CollectionHandlingUtils.getBoolWithEnvFallback(message, MESSAGE_PARAM, ENV_PROP, true)); + } + + @Test + public void testMessageUnsetEnvSetTrueOverridesDefaultFalse() { + System.setProperty(ENV_PROP, "true"); + ZkNodeProps message = new ZkNodeProps(Map.of()); + assertTrue( + CollectionHandlingUtils.getBoolWithEnvFallback(message, MESSAGE_PARAM, ENV_PROP, false)); + } + + @Test + public void testExplicitMessageParamWinsOverEnvFallback() { + System.setProperty(ENV_PROP, "true"); + ZkNodeProps messageFalse = new ZkNodeProps(Map.of(MESSAGE_PARAM, "false")); + assertFalse( + CollectionHandlingUtils.getBoolWithEnvFallback( + messageFalse, MESSAGE_PARAM, ENV_PROP, false)); + + System.setProperty(ENV_PROP, "false"); + ZkNodeProps messageTrue = new ZkNodeProps(Map.of(MESSAGE_PARAM, "true")); + assertTrue( + CollectionHandlingUtils.getBoolWithEnvFallback(messageTrue, MESSAGE_PARAM, ENV_PROP, true)); + } +} diff --git a/solr/core/src/test/org/apache/solr/cloud/api/collections/WaitForFinalStateEnvFallbackTest.java b/solr/core/src/test/org/apache/solr/cloud/api/collections/WaitForFinalStateEnvFallbackTest.java new file mode 100644 index 00000000000..a86652cad90 --- /dev/null +++ b/solr/core/src/test/org/apache/solr/cloud/api/collections/WaitForFinalStateEnvFallbackTest.java @@ -0,0 +1,131 @@ +/* + * 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 java.lang.invoke.MethodHandles; +import org.apache.solr.client.solrj.SolrClient; +import org.apache.solr.client.solrj.request.CollectionAdminRequest; +import org.apache.solr.client.solrj.request.CollectionAdminRequest.AsyncCollectionAdminRequest; +import org.apache.solr.cloud.SolrCloudTestCase; +import org.apache.solr.common.cloud.DocCollection; +import org.apache.solr.common.cloud.Replica; +import org.apache.solr.common.params.CollectionParams.CollectionAction; +import org.apache.solr.common.params.CommonAdminParams; +import org.apache.solr.common.params.CoreAdminParams; +import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.solr.common.params.SolrParams; +import org.apache.solr.util.TestInjection; +import org.junit.After; +import org.junit.BeforeClass; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class WaitForFinalStateEnvFallbackTest extends SolrCloudTestCase { + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + + @BeforeClass + public static void setupCluster() throws Exception { + configureCluster(2).addConfig("conf", configset("cloud-minimal")).configure(); + } + + @After + public void releaseInjectionAndProperty() { + TestInjection.prepRecoveryOpPauseForever = null; + TestInjection.notifyPauseForeverDone(); + System.clearProperty(CommonAdminParams.WAIT_FOR_FINAL_STATE_DEFAULT_PROP); + } + + private static AsyncCollectionAdminRequest addReplicaWithTimeout( + String collection, String shard, int timeoutSeconds) { + return new AsyncCollectionAdminRequest(CollectionAction.ADDREPLICA) { + @Override + public SolrParams getParams() { + ModifiableSolrParams params = (ModifiableSolrParams) super.getParams(); + params.set(CoreAdminParams.COLLECTION, collection); + params.set(CoreAdminParams.SHARD, shard); + params.set(CommonAdminParams.TIMEOUT, timeoutSeconds); + return params; + } + }; + } + + @Test + public void testDefaultFalseSkipsWaitEvenWhenRecoveryIsStuck() throws Exception { + String collection = "envfallbackfalse"; + SolrClient client = cluster.getSolrClient(); + CollectionAdminRequest.createCollection(collection, "conf", 1, 1).process(client); + cluster.waitForActiveCollection(collection, 1, 1); + + // no system property set: ADDREPLICA's literal default is false (waiting on a replica's + // recovery is unbounded in time, unlike CREATE/CREATESHARD/SPLITSHARD's brand-new replicas) + TestInjection.prepRecoveryOpPauseForever = "true:100"; + + long start = System.nanoTime(); + addReplicaWithTimeout(collection, "shard1", 5).process(client); + long elapsedMs = (System.nanoTime() - start) / 1_000_000; + log.info("ADDREPLICA with default (false) returned after {}ms", elapsedMs); + + // proves we returned without waiting on the (permanently stuck) recovery: if the + // ActiveReplicaWatcher had been registered with the 5s timeout above, a genuine wait would + // either finish fast (if it wasn't really stuck) or throw after ~5s -- neither of which we + // want; we want no watcher registered at all, so this returns near-instantly. + assertTrue( + "expected ADDREPLICA to return promptly since waitForFinalState defaults to false, " + + "but it took " + + elapsedMs + + "ms", + elapsedMs < 5_000); + + DocCollection coll = cluster.getSolrClient().getClusterState().getCollection(collection); + boolean anyNonActive = coll.replicaStream().anyMatch(r -> r.getState() != Replica.State.ACTIVE); + assertTrue( + "expected the new replica to still be stuck in recovery (not ACTIVE) since we " + + "returned before waiting for final state", + anyNonActive); + } + + @Test + public void testEnvFallbackTrueActuallyWaitsAndTimesOutWhenRecoveryIsStuck() throws Exception { + String collection = "envfallbacktrue"; + SolrClient client = cluster.getSolrClient(); + CollectionAdminRequest.createCollection(collection, "conf", 1, 1).process(client); + cluster.waitForActiveCollection(collection, 1, 1); + + System.setProperty(CommonAdminParams.WAIT_FOR_FINAL_STATE_DEFAULT_PROP, "true"); + TestInjection.prepRecoveryOpPauseForever = "true:100"; + + long start = System.nanoTime(); + boolean threw = false; + try { + addReplicaWithTimeout(collection, "shard1", 5).process(client); + } catch (Exception e) { + threw = true; + log.info("ADDREPLICA with env fallback=true threw as expected: {}", e); + } + long elapsedMs = (System.nanoTime() - start) / 1_000_000; + log.info("ADDREPLICA with env fallback=true returned/threw after {}ms", elapsedMs); + + assertTrue( + "expected ADDREPLICA to actually wait for final state (and time out, since recovery " + + "is permanently stuck) when waitForFinalState resolves to true via the env " + + "fallback, but it returned successfully after only " + + elapsedMs + + "ms", + threw); + } +} diff --git a/solr/core/src/test/org/apache/solr/core/TestSetPropertyConfigApis.java b/solr/core/src/test/org/apache/solr/core/TestSetPropertyConfigApis.java index 54c30f7d75f..8bf024cbecd 100644 --- a/solr/core/src/test/org/apache/solr/core/TestSetPropertyConfigApis.java +++ b/solr/core/src/test/org/apache/solr/core/TestSetPropertyConfigApis.java @@ -279,7 +279,6 @@ public void testTwoCollectionsWithDifferentProps() throws Exception { private static void processAndAssertSuccess(final CollectionAdminRequest.Create op) throws Exception { - op.setWaitForFinalState(true); assertTrue(op.process(cluster.getSolrClient()).isSuccess()); } diff --git a/solr/core/src/test/org/apache/solr/handler/admin/api/MigrateReplicasAPITest.java b/solr/core/src/test/org/apache/solr/handler/admin/api/MigrateReplicasAPITest.java index 574d671f595..5c901f1c5de 100644 --- a/solr/core/src/test/org/apache/solr/handler/admin/api/MigrateReplicasAPITest.java +++ b/solr/core/src/test/org/apache/solr/handler/admin/api/MigrateReplicasAPITest.java @@ -42,8 +42,7 @@ public void setUp() throws Exception { @Test public void testCreatesValidOverseerMessage() throws Exception { MigrateReplicasRequestBody requestBody = - new MigrateReplicasRequestBody( - Set.of("demoSourceNode"), Set.of("demoTargetNode"), false, "async"); + new MigrateReplicasRequestBody(Set.of("demoSourceNode"), Set.of("demoTargetNode"), "async"); api.migrateReplicas(requestBody); @@ -51,17 +50,16 @@ public void testCreatesValidOverseerMessage() throws Exception { CollectionParams.CollectionAction.MIGRATE_REPLICAS, "async", message -> { - assertEquals(3, message.size()); + assertEquals(2, message.size()); assertEquals(Set.of("demoSourceNode"), message.get("sourceNodes")); assertEquals(Set.of("demoTargetNode"), message.get("targetNodes")); - assertEquals(false, message.get("waitForFinalState")); }); } @Test public void testNoTargetNodes() throws Exception { MigrateReplicasRequestBody requestBody = - new MigrateReplicasRequestBody(Set.of("demoSourceNode"), null, null, null); + new MigrateReplicasRequestBody(Set.of("demoSourceNode"), null, null); api.migrateReplicas(requestBody); @@ -76,10 +74,10 @@ public void testNoTargetNodes() throws Exception { @Test public void testNoSourceNodesThrowsError() { MigrateReplicasRequestBody requestBody1 = - new MigrateReplicasRequestBody(Set.of(), Set.of("demoTargetNode"), null, null); + new MigrateReplicasRequestBody(Set.of(), Set.of("demoTargetNode"), null); assertThrows(SolrException.class, () -> api.migrateReplicas(requestBody1)); MigrateReplicasRequestBody requestBody2 = - new MigrateReplicasRequestBody(null, Set.of("demoTargetNode"), null, null); + new MigrateReplicasRequestBody(null, Set.of("demoTargetNode"), null); assertThrows(SolrException.class, () -> api.migrateReplicas(requestBody2)); } } diff --git a/solr/core/src/test/org/apache/solr/handler/admin/api/ReplaceNodeAPITest.java b/solr/core/src/test/org/apache/solr/handler/admin/api/ReplaceNodeAPITest.java index 7386c4e2967..2fcfda81f2c 100644 --- a/solr/core/src/test/org/apache/solr/handler/admin/api/ReplaceNodeAPITest.java +++ b/solr/core/src/test/org/apache/solr/handler/admin/api/ReplaceNodeAPITest.java @@ -39,7 +39,7 @@ public void setUp() throws Exception { @Test public void testCreatesValidOverseerMessage() throws Exception { - final var requestBody = new ReplaceNodeRequestBody("demoTargetNode", false, "async"); + final var requestBody = new ReplaceNodeRequestBody("demoTargetNode", "async"); api.replaceNode("demoSourceNode", requestBody); @@ -47,10 +47,9 @@ public void testCreatesValidOverseerMessage() throws Exception { CollectionParams.CollectionAction.REPLACENODE, requestBody.async, message -> { - assertEquals(3, message.size()); + assertEquals(2, message.size()); assertEquals("demoSourceNode", message.get("sourceNode")); assertEquals("demoTargetNode", message.get("targetNode")); - assertEquals(false, message.get("waitForFinalState")); }); } @@ -68,7 +67,7 @@ public void testRequestBodyCanBeOmittedAltogether() throws Exception { @Test public void testOptionalValuesNotAddedToRemoteMessageIfNotProvided() throws Exception { - final var requestBody = new ReplaceNodeRequestBody("demoTargetNode", null, null); + final var requestBody = new ReplaceNodeRequestBody("demoTargetNode", null); api.replaceNode("demoSourceNode", requestBody); @@ -78,10 +77,6 @@ public void testOptionalValuesNotAddedToRemoteMessageIfNotProvided() throws Exce assertEquals(2, message.size()); assertEquals("demoSourceNode", message.get("sourceNode")); assertEquals("demoTargetNode", message.get("targetNode")); - assertFalse( - "Expected message to not contain value for waitForFinalState: " - + message.get("waitForFinalState"), - message.containsKey("waitForFinalState")); }); } } diff --git a/solr/core/src/test/org/apache/solr/handler/admin/api/WaitForFinalStateRequestBodyOmittedFieldTest.java b/solr/core/src/test/org/apache/solr/handler/admin/api/WaitForFinalStateRequestBodyOmittedFieldTest.java new file mode 100644 index 00000000000..4a9ae8edc46 --- /dev/null +++ b/solr/core/src/test/org/apache/solr/handler/admin/api/WaitForFinalStateRequestBodyOmittedFieldTest.java @@ -0,0 +1,43 @@ +/* + * 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.handler.admin.api; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.solr.SolrTestCase; +import org.apache.solr.client.api.model.CreateCollectionRequestBody; +import org.apache.solr.client.api.model.CreateReplicaRequestBody; +import org.apache.solr.client.api.model.CreateShardRequestBody; +import org.junit.Test; + +/** + * AC8 (SOLR-18367): a v2 REST request body that omits {@code waitForFinalState} must deserialize to + * {@code null}, not {@code false}, so {@code insertIfNotNull} lets the server's own EnvUtils-driven + * default apply -- the same as SolrJ's field-omitted case. Only applies to the bounded commands + * (CREATE, CREATESHARD, ADDREPLICA); BALANCE_REPLICAS/MIGRATE_REPLICAS/ REPLACENODE dropped the + * field entirely -- their default stays `false` unconditionally. + */ +public class WaitForFinalStateRequestBodyOmittedFieldTest extends SolrTestCase { + + private final ObjectMapper mapper = new ObjectMapper(); + + @Test + public void testOmittedFieldDeserializesToNullNotFalse() throws Exception { + assertNull(mapper.readValue("{}", CreateCollectionRequestBody.class).waitForFinalState); + assertNull(mapper.readValue("{}", CreateShardRequestBody.class).waitForFinalState); + assertNull(mapper.readValue("{}", CreateReplicaRequestBody.class).waitForFinalState); + } +} diff --git a/solr/solr-ref-guide/modules/deployment-guide/pages/cluster-node-management.adoc b/solr/solr-ref-guide/modules/deployment-guide/pages/cluster-node-management.adoc index 46517de459d..c5bd4ea7326 100644 --- a/solr/solr-ref-guide/modules/deployment-guide/pages/cluster-node-management.adoc +++ b/solr/solr-ref-guide/modules/deployment-guide/pages/cluster-node-management.adoc @@ -535,6 +535,7 @@ If this parameter is not provided, all live data nodes will be used. + If `true`, the request will complete only when all affected replicas become active. If `false`, the API will return when the bare minimum replicas are active, such as the affected leader replicas. +This command can affect an arbitrary number of replicas cluster-wide, so unlike other Collections API commands it keeps the pre-SOLR-18367 default; an operator can opt in cluster-wide without changing any client by setting the system property `solr.cloud.waitForFinalState.enabled=true` on each Solr node. `async`:: + @@ -724,6 +725,7 @@ If there is more than one node to migrate the replicas to, then the configured P + If `true`, the request will complete only when all affected replicas become active. If `false`, the API will return when the bare minimum replicas are active, such as the affected leader replicas. +This command can affect an arbitrary number of replicas cluster-wide, so unlike other Collections API commands it keeps the pre-SOLR-18367 default; an operator can opt in cluster-wide without changing any client by setting the system property `solr.cloud.waitForFinalState.enabled=true` on each Solr node. `async`:: + @@ -783,7 +785,6 @@ V2 API:: curl -X POST "http://localhost:8983/api/cluster/nodes/localhost:7574_solr/replace" -H 'Content-Type: application/json' -d ' { "targetNodeName": "localhost:8983_solr", - "waitForFinalState": "false", "async": "async" } ' @@ -833,6 +834,7 @@ Keep in mind that this can lead to very high network and disk I/O if the replica + If `true`, the request will complete only when all affected replicas become active. If `false`, the API will return when the bare minimum replicas are active, such as the affected leader replicas. +This command can affect an arbitrary number of replicas cluster-wide, so unlike other Collections API commands it keeps the pre-SOLR-18367 default; an operator can opt in cluster-wide without changing any client by setting the system property `solr.cloud.waitForFinalState.enabled=true` on each Solr node. `async`:: + diff --git a/solr/solr-ref-guide/modules/deployment-guide/pages/collection-management.adoc b/solr/solr-ref-guide/modules/deployment-guide/pages/collection-management.adoc index 46452619f7c..3b45611efd0 100644 --- a/solr/solr-ref-guide/modules/deployment-guide/pages/collection-management.adoc +++ b/solr/solr-ref-guide/modules/deployment-guide/pages/collection-management.adoc @@ -263,11 +263,12 @@ Altering these entries by specifying `property._name_=_value_` is an expert-leve + [%autowidth,frame=none] |=== -|Optional |Default: none +|Optional |Default: `true` |=== + If `true`, the request will complete only when all affected replicas become active. -The default is `false`, which means that the API will return the status of the single action, which may be before the new replica is online and active. +If `false`, the API will return the status of the single action, which may be before the new replica is online and active. +An operator can revert the cluster-wide default back to `false` without changing any client by setting the system property `solr.cloud.waitForFinalState.enabled=false` on each Solr node. `alias`:: + diff --git a/solr/solr-ref-guide/modules/deployment-guide/pages/replica-management.adoc b/solr/solr-ref-guide/modules/deployment-guide/pages/replica-management.adoc index 73c19a9db47..482549151ed 100644 --- a/solr/solr-ref-guide/modules/deployment-guide/pages/replica-management.adoc +++ b/solr/solr-ref-guide/modules/deployment-guide/pages/replica-management.adoc @@ -239,11 +239,12 @@ Altering these entries by specifying `property._name_=_value_` is an expert-leve + [%autowidth,frame=none] |=== -|Optional |Default: `false` +|Optional |Default: `true` |=== + If `true`, the request will complete only when all affected replicas become active. If `false`, the API will return the status of the single action, which may be before the new replica is online and active. +An operator can revert the cluster-wide default back to `false` without changing any client by setting the system property `solr.cloud.waitForFinalState.enabled=false` on each Solr node. `async`:: + diff --git a/solr/solr-ref-guide/modules/deployment-guide/pages/shard-management.adoc b/solr/solr-ref-guide/modules/deployment-guide/pages/shard-management.adoc index e623424b6aa..6f504e9e6cc 100644 --- a/solr/solr-ref-guide/modules/deployment-guide/pages/shard-management.adoc +++ b/solr/solr-ref-guide/modules/deployment-guide/pages/shard-management.adoc @@ -209,11 +209,12 @@ See the section xref:configuration-guide:core-discovery.adoc[] for details on su + [%autowidth,frame=none] |=== -|Optional |Default: `false` +|Optional |Default: `true` |=== + If `true`, the request will complete only when all affected replicas become active. If `false`, the API will return the status of the single action, which may be before the new replica is online and active. +An operator can revert the cluster-wide default back to `false` without changing any client by setting the system property `solr.cloud.waitForFinalState.enabled=false` on each Solr node. `timing`:: + @@ -423,11 +424,12 @@ See the section xref:configuration-guide:core-discovery.adoc[] for details on su + [%autowidth,frame=none] |=== -|Optional |Default: `false` +|Optional |Default: `true` |=== + If `true`, the request will complete only when all affected replicas become active. If `false`, the API will return the status of the single action, which may be before the new replica is online and active. +An operator can revert the cluster-wide default back to `false` without changing any client by setting the system property `solr.cloud.waitForFinalState.enabled=false` on each Solr node. `async`:: + diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java b/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java index ac7af67427c..7c0de597f33 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java @@ -146,7 +146,6 @@ public abstract static class AsyncCollectionAdminRequest extends CollectionAdminRequest { protected String asyncId = null; - protected boolean waitForFinalState = false; public AsyncCollectionAdminRequest(CollectionAction action) { super(action); @@ -165,16 +164,6 @@ public String getAsyncId() { return asyncId; } - /** - * @deprecated Solr is moving toward always waiting for final state, with no option to opt out; - * once that happens, this parameter will have no effect and will likely be removed. See - * SOLR-17712. - */ - @Deprecated(since = "9.10") - public void setWaitForFinalState(boolean waitForFinalState) { - this.waitForFinalState = waitForFinalState; - } - public void setAsyncId(String asyncId) { this.asyncId = asyncId; } @@ -242,9 +231,6 @@ public SolrParams getParams() { if (asyncId != null) { params.set(CommonAdminParams.ASYNC, asyncId); } - if (waitForFinalState) { - params.set(CommonAdminParams.WAIT_FOR_FINAL_STATE, waitForFinalState); - } return params; } } diff --git a/solr/solrj/src/java/org/apache/solr/common/params/CommonAdminParams.java b/solr/solrj/src/java/org/apache/solr/common/params/CommonAdminParams.java index b55162d01cd..e017252752d 100644 --- a/solr/solrj/src/java/org/apache/solr/common/params/CommonAdminParams.java +++ b/solr/solrj/src/java/org/apache/solr/common/params/CommonAdminParams.java @@ -31,6 +31,19 @@ public interface CommonAdminParams { @Deprecated(since = "9.10") String WAIT_FOR_FINAL_STATE = "waitForFinalState"; + /** + * Node-level system property controlling the default value of {@link #WAIT_FOR_FINAL_STATE} when + * a request omits it. The per-command literal default it overrides differs by command: CREATE, + * CREATESHARD and SPLITSHARD default to {@code true} -- they only ever wait on brand-new, empty + * replicas, so activation is fast and bounded. ADDREPLICA, MOVEREPLICA, BALANCE_REPLICAS, + * MIGRATE_REPLICAS and REPLACENODE keep the pre-10.1 {@code false} default: each can wait on a + * replica catching up on an arbitrary amount of existing data (a full recovery/replication), so + * the wait is unbounded in time even when the replica count is small or fixed. Setting this + * property (either value) overrides the per-command default uniformly for all 8, mirroring {@code + * CreateCollectionCmd.PRS_DEFAULT_PROP}. + */ + String WAIT_FOR_FINAL_STATE_DEFAULT_PROP = "solr.cloud.waitForFinalState.enabled"; + /** Allow in-place move of replicas that use shared filesystems. */ String IN_PLACE_MOVE = "inPlaceMove";