murblanc commented on a change in pull request #2318: URL: https://github.com/apache/lucene-solr/pull/2318#discussion_r574498232
########## File path: solr/core/src/java/org/apache/solr/cloud/RefreshCollectionMessage.java ########## @@ -0,0 +1,58 @@ +/* + * 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; + +import org.apache.solr.common.cloud.ClusterState; +import org.apache.solr.common.cloud.DocCollection; +import org.apache.solr.common.cloud.ZkStateReader; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.data.Stat; + +/**Refresh the Cluster State for a given collection + * + */ +public class RefreshCollectionMessage implements Overseer.Message { + public final Operation operation; + public final String collection; + + public RefreshCollectionMessage(String collection) { + this.operation = Operation.REFRESH_COLL; + this.collection = collection; + } + + ClusterState run(ClusterState clusterState, Overseer overseer) throws InterruptedException, KeeperException { Review comment: Can you please another name than `run()` that is usually related to a `Runnable` which is not the case here. ########## File path: solr/core/src/java/org/apache/solr/cloud/Overseer.java ########## @@ -1064,4 +1073,19 @@ public void offerStateUpdate(byte[] data) throws KeeperException, InterruptedExc getStateUpdateQueue().offer(data); } + /**Submit an intra-process message + * This will be picked up and executed when clusterstate updater thread runs + */ + public void submit(Message message) { + unprocessedMessages.add(message); + } + + public interface Message { Review comment: No attributes of `Message` are used in the code. The queue could well be defined as `CopyOnWriteArrayList<Object>` and not a single line of code would have to change. I suggest we define the processing method in Message (`processMessage`? the one currently called `run`) and when getting a new message from the queue above in `Message m = unprocessedMessages.remove(0);` simply call `m.processMessage(clusterState, Overseer.this);` ########## File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java ########## @@ -213,7 +219,16 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin ZkStateReader.NODE_NAME_PROP, nodeName, ZkStateReader.REPLICA_TYPE, replicaPosition.type.name(), CommonAdminParams.WAIT_FOR_FINAL_STATE, Boolean.toString(waitForFinalState)); - ocmh.overseer.offerStateUpdate(Utils.toJSON(props)); + if(isPrs) { + ZkWriteCommand command = new SliceMutator(ocmh.cloudManager).addReplica(clusterState, props); Review comment: Note that each call to `addReplica` here will read back all existing znodes under `state.json`. This is therefore running in `n^2` (`n*(n+1)/2`) with `n` the number of replicas. (`fetch` in `PerReplicaStates` called from `SliceMutator.addReplica` will always see a different `cversion` value since the previous replica was added on the previous iteration and will therefore `getChildren`) ########## File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java ########## @@ -256,6 +280,23 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin shardRequestTracker.processResponses(results, shardHandler, false, null, Collections.emptySet()); @SuppressWarnings({"rawtypes"}) boolean failure = results.get("failure") != null && ((SimpleOrderedMap)results.get("failure")).size() > 0; + if(isPrs) { + TimeOut timeout = new TimeOut(Integer.getInteger("solr.waitToSeeReplicasInStateTimeoutSeconds", 120), TimeUnit.SECONDS, timeSource); // could be a big cluster + PerReplicaStates prs = PerReplicaStates.fetch(collectionPath, ocmh.zkStateReader.getZkClient(), null); + while (!timeout.hasTimedOut()) { + if(prs.allActive()) break; + Thread.sleep(100); + prs = PerReplicaStates.fetch(collectionPath, ocmh.zkStateReader.getZkClient(), null); + } + if (prs.allActive()) { + // we have successfully found all replicas to be ACTIVE + // Now ask Overseer to fetch the latest state of collection + // from ZK + ocmh.overseer.submit(new RefreshCollectionMessage(collectionName)); + } else { + failure = true; + } + } if (failure) { // Let's cleanup as we hit an exception // We shouldn't be passing 'results' here for the cleanup as the response would then contain 'success' Review comment: In case of PRS collection (and failure, so we're executing here), the collection delete called from two lines below (cannot attach comment to the actual line, thanks GitHub) will fail because the `ClusterStateUpdater` will not know about the collection, it wasn't refreshed. The failure will happen while waiting for the state after the call to `ocmh.overseer.offerStateUpdate(Utils.toJSON(m));` in `DeleteCollectionCmd`. ########## File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java ########## @@ -246,7 +261,16 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin } // wait for all replica entries to be created Review comment: move comment into the `else` bloc. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
