SOLR-17712 : Deprecate CommonAdminParams.waitForFinalState and make it true default #3684

abumarjikar · 2025-09-23T05:56:09Z

https://issues.apache.org/jira/browse/SOLR-17712

Description

This PR targets to Deprecate CommonAdminParams.waitForFinalState and make it true default

Solution

Please provide a short description of the approach taken to implement your solution.

Tests

Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
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)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

solr/solr-ref-guide/modules/deployment-guide/pages/collection-management.adoc

solr/api/src/java/org/apache/solr/client/api/model/BalanceReplicasRequestBody.java

solr/CHANGES.txt

…efault_true

abumarjikar · 2025-09-25T12:05:00Z

@dsmiley Changes as per the review.

dsmiley

Thanks; excellent! I'll merge tomorrow night at the earliest.

dsmiley · 2025-09-28T18:31:12Z

Looks like there are test failures. I suggest finding the one that looks the simplest and look into it further. If you need help, LMK.

abumarjikar · 2025-09-29T12:05:05Z

@dsmiley Mostly all the watchers are failing to complete or to fulfil the replica requirement. I might need help to which watcher to utilise in this case.

if (waitForFinalState) {
SolrCloseableLatch latch =
new SolrCloseableLatch(totalReplicas, ccc.getCloseableToLatchOn());
ActiveReplicaWatcher watcher =
new ActiveReplicaWatcher(
collectionName,
null,
createReplicas.stream()
.map(createReplica -> createReplica.coreName)
.collect(Collectors.toList()),
latch);
try {
zkStateReader.registerCollectionStateWatcher(collectionName, watcher);
runnable.run();
if (!latch.await(timeout, TimeUnit.SECONDS)) {
throw new SolrException(
SolrException.ErrorCode.SERVER_ERROR,
"Timeout waiting " + timeout + " seconds for replica to become active.");
}
} finally {
zkStateReader.removeCollectionStateWatcher(collectionName, watcher);
}
}

This is from addreplicacmd.

dsmiley · 2025-10-05T01:03:35Z

Should be deprecated: org.apache.solr.client.solrj.request.CollectionAdminRequest.AsyncCollectionAdminRequest#setWaitForFinalState

…nalState_default_true # Conflicts: # solr/CHANGES.txt

dsmiley · 2025-10-06T13:05:12Z

I debugged one test -- TestPullReplica. The test intentionally created a replica that would have issues becoming fully active. That aspect was easy -- switch from process to processAsync so the test doesn't hang at this step. I made that trivial change. But the very next thing it did was try to set a replica property on it. That blocks on the lock for the replica's creation. As an experiment, I made ADDREPLICAPROP and DELETEREPLICAPROP use LockLevel.NONE in CommonParams. The test passed. I wonder if admin command internals should have an optional separate phase for waiting for the "final state". Also BTW it was helpful in debugging to disable the Overseer.

(add replica, move replica, migrate, replace, balance)

dsmiley

I spent half the day investigating / fixing what I think is the last of the difficult tests; a collection restore test. What remains are a handful of tests that just check messages and need to be updated simply (I assume merely by looking at test method names). @abumarjikar could you handle those please?

The lock matter needs to be discussed more... I'll at-mention Houston for that.

dsmiley · 2025-10-07T03:07:05Z

solr/core/src/java/org/apache/solr/cloud/api/collections/AddReplicaCmd.java

quite a few SolrCloud commands directly call AddReplicaCmd...

dsmiley · 2025-10-07T03:07:57Z

solr/core/src/java/org/apache/solr/cloud/ActiveReplicaWatcher.java

+    if (collectionState.getZNodeVersion() == lastZkVersion
+        && Objects.equals(lastPrs, collectionState.getPerReplicaStates())) {


PRS wasn't working with waitForFinalState until this

dsmiley · 2025-10-07T03:08:36Z

solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java

      RequestStatusState state = RequestStatusState.NOT_FOUND;
      while (System.nanoTime() < finishTime) {
-        state = this.process(client).getRequestStatus();
+        RequestStatusResponse response = this.process(client);


I just wanted this on its own line so that in a debugger we can easily see what the response payload has

dsmiley · 2025-10-07T03:16:54Z

solr/solrj/src/java/org/apache/solr/common/params/CollectionParams.java

+    ADDREPLICAPROP(true, LockLevel.NONE), // nocommit discuss
+    DELETEREPLICAPROP(true, LockLevel.NONE), // nocommit discuss


@HoustonPutman I wonder what your opinion is here. We've got one test org.apache.solr.cloud.TestPullReplica#testSkipLeaderRecoveryProperty that purposefully adds a replica that isn't happy and then sets a property (SKIP_LEADER_RECOVERY_PROP) that leads to it being happy. I removed the lock level, which made the test pass so that it doesn't block on addReplica's happiness (final state). That's not my reasoning to remove the lock level... I'm wondering it it's harmless to let replica properties be set on any replica at any time?

Maybe ideally the waitForFinalState should happen outside the LockLevel but that would be tricky as it amounts to a design change to command processing, giving commands a final stage outside the lock.

Discussed in dev list. I'm inclined to have these be NONE. And add an upgrade ref guide note about BALANCESHARDSUNIQUE to advise not to change the property during running that command (duh).
For that matter... no reason not to do likewise for manipulating a collection property.

Please enter the commit message for your changes. Lines starting

…ate_default_true' into SOLR_17712_remove_waitForFinalState_default_true

…tForFinalState_default_true # Conflicts: # solr/CHANGES.txt

abumarjikar · 2025-10-09T19:18:40Z

Hey @dsmiley I have fixed the remaining test cases

…efault_true

solr/core/src/test/org/apache/solr/cloud/TestLockTree.java

dsmiley · 2025-10-14T00:39:24Z

solr/core/src/test/org/apache/solr/cloud/TestLockTree.java

      for (Pair<CollectionAction, List<String>> operation : operations) {
        final Lock lock = session.lock(operation.first(), operation.second());
-        if (lock != null) {
+        if (lock != null && !lock.equals(LockTree.FREELOCK)) {


curious what's going on here? (and with orderOfExecutionChanges)

dsmiley · 2025-10-20T13:24:17Z

I think a sync from main is in order

…true

…efault_true

github-actions · 2026-01-27T00:14:49Z

This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!

Initial Commit

05eb458

github-actions bot added documentation Improvements or additions to documentation client:solrj tests cat:cloud cat:api labels Sep 23, 2025

dsmiley reviewed Sep 25, 2025

View reviewed changes

solr/solr-ref-guide/modules/deployment-guide/pages/collection-management.adoc Show resolved Hide resolved

solr/api/src/java/org/apache/solr/client/api/model/BalanceReplicasRequestBody.java Show resolved Hide resolved

solr/CHANGES.txt Outdated Show resolved Hide resolved

abumarjikar and others added 2 commits September 25, 2025 17:33

Changes made as per the review

f0f0d31

Merge branch 'apache:main' into SOLR_17712_remove_waitForFinalState_d…

b76ce86

…efault_true

abumarjikar requested a review from dsmiley September 25, 2025 12:20

dsmiley approved these changes Sep 26, 2025

View reviewed changes

abumarjikar requested a review from dsmiley September 30, 2025 05:41

dsmiley added 3 commits October 6, 2025 09:00

TellPullReplica: processAsync

d8cbb8e

replica props don't need locks?

3e33422

Merge branch 'main' into fork/abumarjikar/SOLR_17712_remove_waitForFi…

cdb67b0

…nalState_default_true # Conflicts: # solr/CHANGES.txt

dsmiley added 2 commits October 6, 2025 16:40

Fix ActiveReplicaWatcher when PRS

8c09b33

(add replica, move replica, migrate, replace, balance)

AddReplicaCmd: don't wait if there are failures

76f7a32

dsmiley reviewed Oct 7, 2025

View reviewed changes

abumarjikar added 4 commits October 7, 2025 14:19

fixed some tests

5ef92a9

Please enter the commit message for your changes. Lines starting

Merge remote-tracking branch 'origin/SOLR_17712_remove_waitForFinalSt…

92543fd

…ate_default_true' into SOLR_17712_remove_waitForFinalState_default_true

Merge remote-tracking branch 'origin/main' into SOLR_17712_remove_wai…

80f6772

…tForFinalState_default_true # Conflicts: # solr/CHANGES.txt

changes done in the tests

77892bd

abumarjikar requested a review from dsmiley October 9, 2025 08:28

changes done to deprecate

2d403f5

abumarjikar added 2 commits October 10, 2025 00:49

Merge branch 'apache:main' into SOLR_17712_remove_waitForFinalState_d…

e19f7cd

…efault_true

Merge branch 'apache:main' into SOLR_17712_remove_waitForFinalState_d…

f629e4f

…efault_true

dsmiley mentioned this pull request Oct 11, 2025

SOLR-17712: Deprecating waitForFinalState parameter #3756

Merged

dsmiley reviewed Oct 14, 2025

View reviewed changes

dsmiley mentioned this pull request Oct 17, 2025

SOLR-17963: Don't lock replica/collection property commands #3786

Merged

Merge branch 'main' into SOLR_17712_remove_waitForFinalState_default_…

f8f82af

…true

github-actions bot removed the client:solrj label Oct 22, 2025

Merge branch 'apache:main' into SOLR_17712_remove_waitForFinalState_d…

518a82b

…efault_true

abumarjikar requested a review from dsmiley October 23, 2025 06:24

abumarjikar and others added 2 commits November 11, 2025 18:46

Changes made as per the review

b9daabf

Merge branch 'apache:main' into SOLR_17712_remove_waitForFinalState_d…

7500fcf

…efault_true

github-actions bot added the stale PR not updated in 60 days label Jan 27, 2026

		if (collectionState.getZNodeVersion() == lastZkVersion
		&& Objects.equals(lastPrs, collectionState.getPerReplicaStates())) {

		ADDREPLICAPROP(true, LockLevel.NONE), // nocommit discuss
		DELETEREPLICAPROP(true, LockLevel.NONE), // nocommit discuss

SOLR-17712 : Deprecate CommonAdminParams.waitForFinalState and make it true default #3684

Are you sure you want to change the base?

SOLR-17712 : Deprecate CommonAdminParams.waitForFinalState and make it true default #3684

Uh oh!

Conversation

abumarjikar commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Solution

Tests

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abumarjikar commented Sep 25, 2025

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

dsmiley commented Sep 28, 2025

Uh oh!

abumarjikar commented Sep 29, 2025

Uh oh!

dsmiley commented Oct 5, 2025

Uh oh!

dsmiley commented Oct 6, 2025

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

dsmiley Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

dsmiley Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

dsmiley Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

dsmiley Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

dsmiley Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

abumarjikar commented Oct 9, 2025

Uh oh!

Uh oh!

dsmiley Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

dsmiley commented Oct 20, 2025

Uh oh!

github-actions bot commented Jan 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

abumarjikar commented Sep 23, 2025 •

edited

Loading