SOLR-18367: waitForFinalState now defaults to true, remove the deprecated opt-out - #4783
Open
serhiy-bzhezytskyy wants to merge 10 commits into
Open
Conversation
…nalState, default to true SOLR-17712 proposed removing the ability to opt out of waiting for final state entirely, defaulting to 'true'. The setter was deprecated in 9.10 as the first step; this is the second: the field now always resolves to true (no other code path can set it to false), so the setter -- and every test call site that was toggling it -- is gone. Removes/cleans 6 test call sites (5 were redundant .setWaitForFinalState(true) calls; CollectionsAPISolrJTest's .setWaitForFinalState(false) is dropped too, since the test doesn't depend on the non-waiting behavior).
…ands Completes SOLR-17712's proposal: 'internal logic should behave as if it's true'. Flips the server-side default from false to true in CreateCollectionCmd, AddReplicaCmd, CreateShardCmd, MoveReplicaCmd, SplitShardCmd, BalanceReplicasCmd, MigrateReplicasCmd, ReplaceNodeCmd. The V2 API request bodies (nullable Boolean fields) don't need a change -- when unset they serialize to absent, and these Cmd-level defaults are what actually apply. 72 tests across all 8 affected commands, 0 failures.
Contributor
|
This is a difficult issue; I wish I could have stopped your attempt before you began because there is past work here pending that ought to be examined closely before venturing off to do this. If this relatively simple PR actually works then I'm surprised... the CI failures likely corroborate my concern. Please consider participating in the other PR. |
dsmiley
self-requested a review
August 21, 2026 13:46
…ForFinalState default flip waitForFinalState now defaults to true and is unconditionally sent by every AsyncCollectionAdminRequest subclass, so it belongs in the expected param set for every request built on that base -- the test asserts an exact param match.
… Cmd calls Full-suite regression run found AbstractCloudBackupRestoreTestCase.testRestoreFailure now failing (confirmed passing on the pre-redesign base commit): RestoreCmd builds its own synthetic ZkNodeProps messages for CreateCollectionCmd/AddReplicaCmd calls that never set waitForFinalState, so they silently inherited the new true default instead of the old false one, breaking restore's own 'no race, return immediately' assumption and its own completion-latch-driven waiting. Audited every other internal call site that constructs one of the 8 Cmd classes directly (bypassing client-request dispatch) for the same gap. Found 2 more real correctness bugs: MoveReplicaCmd.moveNormalReplica and ReplicaMigrationUtils.migrateReplicas each implement their own ActiveReplicaWatcher/latch wait conditioned on their own waitForFinalState parameter -- the new default made AddReplicaCmd wait internally first, defeating the intended fast path when the caller didn't request a wait. And 2 latency-only cases in MigrateCmd's temp-collection creation, which already does its own leader/sync waits downstream. Every one of these callers now explicitly pins waitForFinalState=false in the message it builds, restoring the pre-redesign contract. Confirmed via testRestoreFailure, which now passes in ~5s (previously exceeded its 30s poll window).
…rdWithNodeRoleTest Both test methods now hang and hit the client's 90s HTTP timeout: in this NodeRole-constrained (data:off) cluster with a PULL replica, SPLITSHARD's new true default makes the server wait internally for full activation, which never completes within the client's timeout budget. Confirmed via baseline comparison this is a pre-existing latent bug, not introduced by this redesign: forcing waitForFinalState=true explicitly on pre-SOLR-18367 main reproduces the identical hang (still RUNNING after 300s). The default flip only newly exposes it, since it was previously unreachable with the old false default. Pinning false here is correct for the same reason as the other 7 internal call sites: this test does its own explicit waitForState(...) polling afterward and never needed the wait.
…figurable flag Adds the global flag David asked for on apache#3684, instead of hardcoding one default for all 8 commands. CREATE, ADDREPLICA, CREATESHARD, MOVEREPLICA, SPLITSHARD default to true; BALANCE_REPLICAS, MIGRATE_REPLICAS, REPLACENODE keep false, since their wait is unbounded (their latch scales with movements.size(), not a fixed count). The flag overrides all 8 either way. Also removes the deprecated SolrJ setter and the v2 REST field for the 3 unbounded commands.
github-actions
Bot
added
the
documentation
Contributor
Author
|
You're right that #3684 is the prior work, and I should have looked at it more closely before opening this. I have now. The CI failure you flagged was real: the client hardcoded I also went back to your comment on #3684 ( |
- spotlessApply on 5 files with javadoc/line-wrap violations. - Collections.emptyMap()/singletonMap() -> Map.of() in CollectionHandlingUtilsTest (forbiddenApisTest). - Drop an eager e.toString() in a parameterized log call (validateLogCalls flags it as suspicious).
They wait on a replica catching up on existing data, not a fixed-size empty one like CREATE/CREATESHARD/SPLITSHARD -- bounded replica count isn't bounded time. Matches unmodified main's current behavior; the WAIT_FOR_FINAL_STATE_DEFAULT_PROP escape hatch can still force true. Found via CloudHttp2SolrClientTest.testPerReplicaStateCollection timing out under the true default. Updated WaitForFinalStateEnvFallbackTest to match the new default/override split.
Contributor
|
CC @abumarjikar |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://issues.apache.org/jira/browse/SOLR-18367
Completes SOLR-17712's proposal, but via the global flag David asked for on #3684 ("I'd be more comfortable making this change now if there's a global flag... to mitigate the performance/scale risk"), not by hardcoding the new default.
Server: new
CollectionHandlingUtils.getBoolWithEnvFallback(message, param, envProp, defaultValue)resolveswaitForFinalStateasmessage->-Dsolr.cloud.waitForFinalState.enabled-> per-command default. Default differs by command, based onActiveReplicaWatcher's blast radius:CREATE,ADDREPLICA,CREATESHARD,MOVEREPLICA,SPLITSHARD->true(bounded wait,AddReplicaCmd/MoveReplicaCmdlatch on a fixed replica count).BALANCE_REPLICAS,MIGRATE_REPLICAS,REPLACENODE-> staysfalse(ReplicaMigrationUtils's latch scales withmovements.size()-- unbounded, cluster-wide, onetimeoutregardless of count).The system property overrides all 8 uniformly in either direction; only the unset default differs.
Client:
AsyncCollectionAdminRequest.setWaitForFinalStateremoved (deprecated since 9.10) -- no subclass sends the param anymore, so the server default fully applies. For the 3 unbounded commands, the v2 REST*RequestBodyPOJOs drop the field entirely (matches the client-side removal); this also removes the V1 admin API's per-request override forREPLACENODEspecifically (BALANCE_REPLICAS/MIGRATE_REPLICAShave no V1 path) -- the global flag is the only override left for those 3, across every calling surface.Known limitation, not fixed here:
ActiveReplicaWatcherreacts to the client-side ZK-watch cache, not ground truth -- a wait can time out even though the replica is already active, if the local watch notification hasn't caught up yet. Pre-existing, but now exercised on 5 of 8 commands by default instead of essentially none.AI-assisted (Claude Sonnet 5)