dsmiley commented on code in PR #3684:
URL: https://github.com/apache/solr/pull/3684#discussion_r2409211840
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java:
##########
@@ -1864,7 +1864,8 @@ public RequestStatusState waitFor(SolrClient client, long
timeoutSeconds)
long finishTime = System.nanoTime() +
TimeUnit.SECONDS.toNanos(timeoutSeconds);
RequestStatusState state = RequestStatusState.NOT_FOUND;
while (System.nanoTime() < finishTime) {
- state = this.process(client).getRequestStatus();
+ RequestStatusResponse response = this.process(client);
Review Comment:
I just wanted this on its own line so that in a debugger we can easily see
what the response payload has
##########
solr/core/src/java/org/apache/solr/cloud/ActiveReplicaWatcher.java:
##########
@@ -150,11 +153,13 @@ public synchronized boolean onStateChanged(Set<String>
liveNodes, DocCollection
log.debug("-- already done, exiting...");
return true;
}
- if (collectionState.getZNodeVersion() == lastZkVersion) {
+ if (collectionState.getZNodeVersion() == lastZkVersion
+ && Objects.equals(lastPrs, collectionState.getPerReplicaStates())) {
Review Comment:
PRS wasn't working with waitForFinalState until this
##########
solr/solrj/src/java/org/apache/solr/common/params/CollectionParams.java:
##########
@@ -111,8 +111,8 @@ enum CollectionAction {
DISTRIBUTEDAPIPROCESSING(false, LockLevel.NONE),
LIST(false, LockLevel.NONE),
CLUSTERSTATUS(false, LockLevel.NONE),
- ADDREPLICAPROP(true, LockLevel.REPLICA),
- DELETEREPLICAPROP(true, LockLevel.REPLICA),
+ ADDREPLICAPROP(true, LockLevel.NONE), // nocommit discuss
+ DELETEREPLICAPROP(true, LockLevel.NONE), // nocommit discuss
Review Comment:
@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.
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/AddReplicaCmd.java:
##########
Review Comment:
quite a few SolrCloud commands directly call AddReplicaCmd...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
