You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "HoustonPutman (via GitHub)" <gi...@apache.org> on 2026/01/21 20:15:13 UTC
[PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
HoustonPutman opened a new pull request, #4069:
URL: https://github.com/apache/solr/pull/4069
https://issues.apache.org/jira/browse/SOLR-18080
Whenever the shard terms change such that the leader is no longer the highest term, a leader election should take place, and all non-up-to-date replicas should go into recovery.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #4069:
URL: https://github.com/apache/solr/pull/4069#discussion_r2934190698
##########
solr/core/src/java/org/apache/solr/update/DefaultSolrCoreState.java:
##########
@@ -51,7 +51,7 @@ public final class DefaultSolrCoreState extends SolrCoreState
private final ReentrantLock recoveryLock = new ReentrantLock();
- private final ActionThrottle recoveryThrottle = new ActionThrottle("recovery", 10000);
+ private final ActionThrottle recoveryThrottle = new ActionThrottle("recovery", 1000);
Review Comment:
Talked with Mark about this, he isn't against lowering it to 1 second. 10 seconds make any tests that explicitly fail recoveries and then succeed recoveries hard to do.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman merged PR #4069:
URL: https://github.com/apache/solr/pull/4069
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #4069:
URL: https://github.com/apache/solr/pull/4069#discussion_r2742661617
##########
solr/core/src/test-files/log4j2.xml:
##########
@@ -22,7 +22,7 @@
<PatternLayout>
<Pattern>
%maxLen{%-4r %-5p (%t) [%notEmpty{n:%X{node_name}}%notEmpty{ c:%X{collection}}%notEmpty{ s:%X{shard}}%notEmpty{ r:%X{replica}}%notEmpty{ x:%X{core}}%notEmpty{ t:%X{trace_id}}] %c{1.} %m%notEmpty{
- =>%ex{short}}}{10240}%n
+ =>%ex}}{10240}%n
Review Comment:
Yeah, I add this when testing on all my pre then remove it before committing. This got through unfortunately. But thanks for taking the lead on it, appreciate that! I'll remove it.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #4069:
URL: https://github.com/apache/solr/pull/4069#discussion_r2976854650
##########
solr/core/src/java/org/apache/solr/cloud/ShardTerms.java:
##########
@@ -124,6 +124,28 @@ private boolean skipIncreaseTermOf(String key, Set<String> replicasNeedingRecove
return replicasNeedingRecovery.contains(key);
}
+ public ShardTerms setHighestTerms(Set<String> highestTermKeys) {
Review Comment:
I also updated the documentation to mention this new use of the class, which should not interfere with the other two uses.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #4069:
URL: https://github.com/apache/solr/pull/4069#discussion_r2714392014
##########
solr/core/src/java/org/apache/solr/cloud/RecoveringCoreTermWatcher.java:
##########
@@ -52,15 +53,55 @@ public boolean onTermChanged(ShardTerms terms) {
if (solrCore.getCoreDescriptor() == null
|| solrCore.getCoreDescriptor().getCloudDescriptor() == null) return true;
String coreNodeName = solrCore.getCoreDescriptor().getCloudDescriptor().getCoreNodeName();
- if (terms.haveHighestTermValue(coreNodeName)) return true;
- if (lastTermDoRecovery.get() < terms.getTerm(coreNodeName)) {
- log.info(
- "Start recovery on {} because core's term is less than leader's term", coreNodeName);
- lastTermDoRecovery.set(terms.getTerm(coreNodeName));
- solrCore
- .getUpdateHandler()
- .getSolrCoreState()
- .doRecovery(solrCore.getCoreContainer(), solrCore.getCoreDescriptor());
+
+ // If we have the highest term, there is nothing to do
+ if (terms.haveHighestTermValue(coreNodeName)) {
+ return true;
+ }
+
+ long lastRecoveryTerm;
+ long newTerm;
+ synchronized (lastTermDoRecovery) {
+ lastRecoveryTerm = lastTermDoRecovery.get();
+ newTerm = terms.getTerm(coreNodeName);
+ if (lastRecoveryTerm < newTerm) {
+ lastTermDoRecovery.set(newTerm);
Review Comment:
Yeah `lastTermDoRecovery` might be a bad name, but after leader election, recovery is guaranteed for these replicas at this term value. So while recovery is not explicitly being done here, we know that leader election will do the recovery. So `lastTermDoRecovery` is still technically correct, just assuming the leader election succeeds.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on code in PR #4069:
URL: https://github.com/apache/solr/pull/4069#discussion_r2714303962
##########
solr/core/src/java/org/apache/solr/cloud/RecoveringCoreTermWatcher.java:
##########
@@ -52,15 +53,55 @@ public boolean onTermChanged(ShardTerms terms) {
if (solrCore.getCoreDescriptor() == null
|| solrCore.getCoreDescriptor().getCloudDescriptor() == null) return true;
String coreNodeName = solrCore.getCoreDescriptor().getCloudDescriptor().getCoreNodeName();
- if (terms.haveHighestTermValue(coreNodeName)) return true;
- if (lastTermDoRecovery.get() < terms.getTerm(coreNodeName)) {
- log.info(
- "Start recovery on {} because core's term is less than leader's term", coreNodeName);
- lastTermDoRecovery.set(terms.getTerm(coreNodeName));
- solrCore
- .getUpdateHandler()
- .getSolrCoreState()
- .doRecovery(solrCore.getCoreContainer(), solrCore.getCoreDescriptor());
+
+ // If we have the highest term, there is nothing to do
+ if (terms.haveHighestTermValue(coreNodeName)) {
+ return true;
+ }
+
+ long lastRecoveryTerm;
+ long newTerm;
+ synchronized (lastTermDoRecovery) {
+ lastRecoveryTerm = lastTermDoRecovery.get();
+ newTerm = terms.getTerm(coreNodeName);
+ if (lastRecoveryTerm < newTerm) {
+ lastTermDoRecovery.set(newTerm);
Review Comment:
lastTermDoRecovery is set here but its possible recovery is deferred below because of leader election now. Is that right? The old logic set it, then actually does recovery regardless. Reading this, seems like there is a possibility that `lastTermDoRecovery` is set to the new term but can skip actually doing recovery further down. So the term this was set to is incorrect based on the name if recovery is skipped?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #4069:
URL: https://github.com/apache/solr/pull/4069#discussion_r2977019561
##########
solr/core/src/java/org/apache/solr/cloud/ShardTerms.java:
##########
@@ -124,6 +124,28 @@ private boolean skipIncreaseTermOf(String key, Set<String> replicasNeedingRecove
return replicasNeedingRecovery.contains(key);
}
+ public ShardTerms setHighestTerms(Set<String> highestTermKeys) {
Review Comment:
> Ok, I added a param to pass the DocCollection in to make sure that the collection is in a read-only state.
Sgtm, thanks @HoustonPutman
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #4069:
URL: https://github.com/apache/solr/pull/4069#discussion_r2742674834
##########
solr/core/src/test/org/apache/solr/cloud/ZkShardTermsRecoveryTest.java:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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 static org.hamcrest.Matchers.in;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.SolrQuery;
+import org.apache.solr.client.solrj.request.UpdateRequest;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.Replica;
+import org.apache.solr.common.cloud.Slice;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class ZkShardTermsRecoveryTest extends SolrCloudTestCase {
+ private static final String COLLECTION = "collection1";
+ private static final int NUM_SHARDS = 2;
+ private static final int NUM_REPLICAS = 5;
+ private static int NUM_DOCS = 0;
+
+ @BeforeClass
+ public static void setupCluster() throws Exception {
+ configureCluster(3).addConfig("conf", configset("cloud-minimal")).configure();
+ assertEquals(
+ 0,
+ CollectionAdminRequest.createCollection(COLLECTION, "conf", NUM_SHARDS, NUM_REPLICAS)
+ .process(cluster.getSolrClient())
+ .getStatus());
+ cluster.waitForActiveCollection(COLLECTION, 10, TimeUnit.SECONDS, 2, NUM_SHARDS * NUM_REPLICAS);
+ }
+
+ @Before
+ public void waitForActiveState() {
+ cluster.waitForActiveCollection(COLLECTION, 10, TimeUnit.SECONDS, 2, NUM_SHARDS * NUM_REPLICAS);
+ }
+
+ @Test
+ public void testShardTermsInducedReplication() throws Exception {
+ String shard = "shard2";
+ if (random().nextBoolean()) {
+ // Add uncommitted/committed documents, to test that part of the recovery
+ UpdateRequest up = new UpdateRequest();
+ for (int i = 0; i < 1000; i++) {
+ up.add("id", "id2-" + i);
+ }
+ up.process(cluster.getSolrClient(), COLLECTION);
+ NUM_DOCS += 1000;
+ if (random().nextBoolean()) {
+ cluster.getSolrClient().commit(COLLECTION);
+ }
+ }
+
+ DocCollection docCollection = cluster.getZkStateReader().getCollection(COLLECTION);
+ JettySolrRunner jetty = cluster.getRandomJetty(random());
+
+ Slice shard1 = docCollection.getSlice(shard);
+ Replica leader = shard1.getLeader();
+ Replica replica = shard1.getReplicas(r -> !r.isLeader()).getFirst();
+ List<Replica> recoveryReplicas =
+ shard1.getReplicas(
+ r -> r.getType().leaderEligible && !(r.equals(leader) || r.equals(replica)));
+
+ ZkShardTerms shardTerms =
+ jetty.getCoreContainer().getZkController().getShardTerms(COLLECTION, shard);
+ // Increase the leader and another replica's shardTerms
+ shardTerms.ensureHighestTerms(Set.of(leader.getName(), replica.getName()));
+
+ waitForState(
+ "Waiting for replicas to go into recovery",
+ COLLECTION,
+ 5,
+ TimeUnit.SECONDS,
+ state -> {
+ Slice shardState = state.getSlice(shard);
+ for (Replica r : recoveryReplicas) {
+ if (shardState.getReplica(r.name).getState() != Replica.State.RECOVERING) {
Review Comment:
I believe waitForState uses a ZK watcher, so it should be safe and generally get all of the ZK updates... I haven't had it fail yet and I've run it at least 100 times. But yeah there is a concern there, so might think about it later. Ultimately going into recovery is very necessary here, and didn't used to be what happened always.
Ultimately, we can probably just check that the shard terms all become equal in the end and that the docs are there. That would probably be good enough (And check that the replicas become active of course).
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #4069:
URL: https://github.com/apache/solr/pull/4069#discussion_r2933572934
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1047,8 +1047,7 @@ public static String getShardLeadersElectPath(String collection, String shardId)
+ (shardId != null ? ("/" + shardId + "/" + ELECTION_NODE) : "");
}
- public List<ZkCoreNodeProps> getReplicaProps(
- String collection, String shardId, String thisCoreNodeName) {
+ public List<Replica> getReplicaProps(String collection, String shardId, String thisCoreNodeName) {
Review Comment:
yeah, getReplicas is fine and the footprint isn't too big to include in this PR. Done.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #4069:
URL: https://github.com/apache/solr/pull/4069#discussion_r2744303532
##########
solr/core/src/test/org/apache/solr/cloud/ZkShardTermsRecoveryTest.java:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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 static org.hamcrest.Matchers.in;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.SolrQuery;
+import org.apache.solr.client.solrj.request.UpdateRequest;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.Replica;
+import org.apache.solr.common.cloud.Slice;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class ZkShardTermsRecoveryTest extends SolrCloudTestCase {
+ private static final String COLLECTION = "collection1";
+ private static final int NUM_SHARDS = 2;
+ private static final int NUM_REPLICAS = 5;
+ private static int NUM_DOCS = 0;
+
+ @BeforeClass
+ public static void setupCluster() throws Exception {
+ configureCluster(3).addConfig("conf", configset("cloud-minimal")).configure();
+ assertEquals(
+ 0,
+ CollectionAdminRequest.createCollection(COLLECTION, "conf", NUM_SHARDS, NUM_REPLICAS)
+ .process(cluster.getSolrClient())
+ .getStatus());
+ cluster.waitForActiveCollection(COLLECTION, 10, TimeUnit.SECONDS, 2, NUM_SHARDS * NUM_REPLICAS);
+ }
+
+ @Before
+ public void waitForActiveState() {
+ cluster.waitForActiveCollection(COLLECTION, 10, TimeUnit.SECONDS, 2, NUM_SHARDS * NUM_REPLICAS);
+ }
+
+ @Test
+ public void testShardTermsInducedReplication() throws Exception {
+ String shard = "shard2";
+ if (random().nextBoolean()) {
+ // Add uncommitted/committed documents, to test that part of the recovery
+ UpdateRequest up = new UpdateRequest();
+ for (int i = 0; i < 1000; i++) {
+ up.add("id", "id2-" + i);
+ }
+ up.process(cluster.getSolrClient(), COLLECTION);
+ NUM_DOCS += 1000;
+ if (random().nextBoolean()) {
+ cluster.getSolrClient().commit(COLLECTION);
+ }
+ }
+
+ DocCollection docCollection = cluster.getZkStateReader().getCollection(COLLECTION);
+ JettySolrRunner jetty = cluster.getRandomJetty(random());
+
+ Slice shard1 = docCollection.getSlice(shard);
+ Replica leader = shard1.getLeader();
+ Replica replica = shard1.getReplicas(r -> !r.isLeader()).getFirst();
+ List<Replica> recoveryReplicas =
+ shard1.getReplicas(
+ r -> r.getType().leaderEligible && !(r.equals(leader) || r.equals(replica)));
+
+ ZkShardTerms shardTerms =
+ jetty.getCoreContainer().getZkController().getShardTerms(COLLECTION, shard);
+ // Increase the leader and another replica's shardTerms
+ shardTerms.ensureHighestTerms(Set.of(leader.getName(), replica.getName()));
+
+ waitForState(
+ "Waiting for replicas to go into recovery",
+ COLLECTION,
+ 5,
+ TimeUnit.SECONDS,
+ state -> {
+ Slice shardState = state.getSlice(shard);
+ for (Replica r : recoveryReplicas) {
+ if (shardState.getReplica(r.name).getState() != Replica.State.RECOVERING) {
Review Comment:
Ok, so I changed it to make sure that all given replicas go into recovery at some point, not necessarily at the same point. This should make it a bit more resilient on maybe weird hardware.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #4069:
URL: https://github.com/apache/solr/pull/4069#discussion_r2933553583
##########
solr/core/src/java/org/apache/solr/cloud/ShardTerms.java:
##########
@@ -124,6 +124,28 @@ private boolean skipIncreaseTermOf(String key, Set<String> replicasNeedingRecove
return replicasNeedingRecovery.contains(key);
}
+ public ShardTerms setHighestTerms(Set<String> highestTermKeys) {
Review Comment:
Ok, I added a param to pass the DocCollection in to make sure that the collection is in a read-only state. This could possibly be improved as an API later, but the functionality is correct.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #4069:
URL: https://github.com/apache/solr/pull/4069#discussion_r2933578861
##########
solr/solrj/src/java/org/apache/solr/common/cloud/ZkCoreNodeProps.java:
##########
@@ -47,6 +47,10 @@ public String getCoreName() {
return nodeProps.getStr(ReplicaStateProps.CORE_NAME);
}
+ public String getCoreNodeName() {
Review Comment:
This is generally the same as replica name, but coreNodeName is used very widely across the codebase, so I'm not sure javadocs are really necessary here, since none of the other methods have any.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #4069:
URL: https://github.com/apache/solr/pull/4069#discussion_r2744329474
##########
solr/core/src/java/org/apache/solr/cloud/ShardTerms.java:
##########
@@ -124,6 +124,28 @@ private boolean skipIncreaseTermOf(String key, Set<String> replicasNeedingRecove
return replicasNeedingRecovery.contains(key);
}
+ public ShardTerms setHighestTerms(Set<String> highestTermKeys) {
Review Comment:
So this ultimately doesn't happen from the leader, and the leaders term is not guaranteed to be increased. If we see in our new test class, leader election can be triggered because of this new API.
I think the easiest thing to do here is insist that the collection is in a read-only state when this API is called. I'm not sure how I'll do that, but it will definitely guard against any issues with missing updates or anything like that.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #4069:
URL: https://github.com/apache/solr/pull/4069#discussion_r2741757775
##########
solr/core/src/java/org/apache/solr/cloud/ShardLeaderElectionContextBase.java:
##########
@@ -182,8 +182,7 @@ void runLeaderProcess(boolean weAreReplacement) throws KeeperException, Interrup
.getClusterState()
.getCollection(collection)
.getSlice(shardId)
- .getReplicas()
- .size()
+ .getNumLeaderReplicas()
Review Comment:
[+1] Good catch, clear logic error here.
##########
solr/core/src/test-files/log4j2.xml:
##########
@@ -22,7 +22,7 @@
<PatternLayout>
<Pattern>
%maxLen{%-4r %-5p (%t) [%notEmpty{n:%X{node_name}}%notEmpty{ c:%X{collection}}%notEmpty{ s:%X{shard}}%notEmpty{ r:%X{replica}}%notEmpty{ x:%X{core}}%notEmpty{ t:%X{trace_id}}] %c{1.} %m%notEmpty{
- =>%ex{short}}}{10240}%n
+ =>%ex}}{10240}%n
Review Comment:
[Q] If I understand this correctly, this is changing how test runs in 'core' log exceptions?
No problem with changing that but we have one of these files in each module and I imagine if we're updating one of them we should update them all?
I know your time is limited for the next few weeks and you've taken a lot of pains to pare this PR down. With your agreement let's drop this change from the PR and I'll open another PR to make the change across the board for our test files and shepherd it through.
##########
solr/core/src/java/org/apache/solr/cloud/ShardTerms.java:
##########
@@ -124,6 +124,28 @@ private boolean skipIncreaseTermOf(String key, Set<String> replicasNeedingRecove
return replicasNeedingRecovery.contains(key);
}
+ public ShardTerms setHighestTerms(Set<String> highestTermKeys) {
Review Comment:
The whole "term" algorithm makes some pretty strict assumptions about who can update term values, and on what conditions. From class javadocs on ZkShardTerms:
```
* <p>Terms can only updated in two strict ways:
*
* <ul>
* <li>A replica sets its term equals to leader's term
* <li>The leader increase its term and some other replicas by 1
* </ul>
```
This method seems to fit under the latter provision, which is good. But could we add Javadocs here to indicate that this method should only be called by current shard-leaders? Or if this is safe for non-leaders to call in certain situations, add javadocs to describe what those are and why.
Just trying to defend against the possibility of someone coming back through here in a month or two and thinking: "Hey this doesn't fit with the documented algorithm at all"
##########
solr/core/src/java/org/apache/solr/cloud/ZkShardTerms.java:
##########
@@ -120,6 +120,11 @@ public void ensureTermsIsHigher(String leader, Set<String> replicasNeedingRecove
mutate(terms -> terms.increaseTerms(leader, replicasNeedingRecovery));
}
+ public void ensureHighestTerms(Set<String> mostUpToDateCores) {
Review Comment:
Ditto, re: my previous comment on `ShardTerms.setHighestTerms`. We should add some Javadocs to make sure this only called by leaders, or if it's actually safe to call elsewhere describe where and why.
##########
solr/core/src/test/org/apache/solr/cloud/ZkShardTermsRecoveryTest.java:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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 static org.hamcrest.Matchers.in;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.SolrQuery;
+import org.apache.solr.client.solrj.request.UpdateRequest;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.Replica;
+import org.apache.solr.common.cloud.Slice;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class ZkShardTermsRecoveryTest extends SolrCloudTestCase {
+ private static final String COLLECTION = "collection1";
+ private static final int NUM_SHARDS = 2;
+ private static final int NUM_REPLICAS = 5;
+ private static int NUM_DOCS = 0;
+
+ @BeforeClass
+ public static void setupCluster() throws Exception {
+ configureCluster(3).addConfig("conf", configset("cloud-minimal")).configure();
+ assertEquals(
+ 0,
+ CollectionAdminRequest.createCollection(COLLECTION, "conf", NUM_SHARDS, NUM_REPLICAS)
+ .process(cluster.getSolrClient())
+ .getStatus());
+ cluster.waitForActiveCollection(COLLECTION, 10, TimeUnit.SECONDS, 2, NUM_SHARDS * NUM_REPLICAS);
+ }
+
+ @Before
+ public void waitForActiveState() {
+ cluster.waitForActiveCollection(COLLECTION, 10, TimeUnit.SECONDS, 2, NUM_SHARDS * NUM_REPLICAS);
+ }
+
+ @Test
+ public void testShardTermsInducedReplication() throws Exception {
+ String shard = "shard2";
+ if (random().nextBoolean()) {
+ // Add uncommitted/committed documents, to test that part of the recovery
+ UpdateRequest up = new UpdateRequest();
+ for (int i = 0; i < 1000; i++) {
+ up.add("id", "id2-" + i);
+ }
+ up.process(cluster.getSolrClient(), COLLECTION);
+ NUM_DOCS += 1000;
+ if (random().nextBoolean()) {
+ cluster.getSolrClient().commit(COLLECTION);
+ }
+ }
+
+ DocCollection docCollection = cluster.getZkStateReader().getCollection(COLLECTION);
+ JettySolrRunner jetty = cluster.getRandomJetty(random());
+
+ Slice shard1 = docCollection.getSlice(shard);
Review Comment:
[0] Should probably be "shard2", given the value of the string variable set on L64?
Idrc about the name, just mentioning it in case it's a bug in your test logic.
##########
solr/core/src/test/org/apache/solr/cloud/ZkShardTermsRecoveryTest.java:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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 static org.hamcrest.Matchers.in;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.SolrQuery;
+import org.apache.solr.client.solrj.request.UpdateRequest;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.Replica;
+import org.apache.solr.common.cloud.Slice;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class ZkShardTermsRecoveryTest extends SolrCloudTestCase {
+ private static final String COLLECTION = "collection1";
+ private static final int NUM_SHARDS = 2;
+ private static final int NUM_REPLICAS = 5;
+ private static int NUM_DOCS = 0;
+
+ @BeforeClass
+ public static void setupCluster() throws Exception {
+ configureCluster(3).addConfig("conf", configset("cloud-minimal")).configure();
+ assertEquals(
+ 0,
+ CollectionAdminRequest.createCollection(COLLECTION, "conf", NUM_SHARDS, NUM_REPLICAS)
+ .process(cluster.getSolrClient())
+ .getStatus());
+ cluster.waitForActiveCollection(COLLECTION, 10, TimeUnit.SECONDS, 2, NUM_SHARDS * NUM_REPLICAS);
+ }
+
+ @Before
+ public void waitForActiveState() {
+ cluster.waitForActiveCollection(COLLECTION, 10, TimeUnit.SECONDS, 2, NUM_SHARDS * NUM_REPLICAS);
+ }
+
+ @Test
+ public void testShardTermsInducedReplication() throws Exception {
+ String shard = "shard2";
+ if (random().nextBoolean()) {
+ // Add uncommitted/committed documents, to test that part of the recovery
+ UpdateRequest up = new UpdateRequest();
+ for (int i = 0; i < 1000; i++) {
+ up.add("id", "id2-" + i);
+ }
+ up.process(cluster.getSolrClient(), COLLECTION);
+ NUM_DOCS += 1000;
+ if (random().nextBoolean()) {
+ cluster.getSolrClient().commit(COLLECTION);
+ }
+ }
+
+ DocCollection docCollection = cluster.getZkStateReader().getCollection(COLLECTION);
+ JettySolrRunner jetty = cluster.getRandomJetty(random());
+
+ Slice shard1 = docCollection.getSlice(shard);
+ Replica leader = shard1.getLeader();
+ Replica replica = shard1.getReplicas(r -> !r.isLeader()).getFirst();
+ List<Replica> recoveryReplicas =
+ shard1.getReplicas(
+ r -> r.getType().leaderEligible && !(r.equals(leader) || r.equals(replica)));
+
+ ZkShardTerms shardTerms =
+ jetty.getCoreContainer().getZkController().getShardTerms(COLLECTION, shard);
+ // Increase the leader and another replica's shardTerms
+ shardTerms.ensureHighestTerms(Set.of(leader.getName(), replica.getName()));
+
+ waitForState(
+ "Waiting for replicas to go into recovery",
+ COLLECTION,
+ 5,
+ TimeUnit.SECONDS,
+ state -> {
+ Slice shardState = state.getSlice(shard);
+ for (Replica r : recoveryReplicas) {
+ if (shardState.getReplica(r.name).getState() != Replica.State.RECOVERING) {
Review Comment:
[Q] Since "recovering" is a transient state and the doc/index size here is very small, is it possible that the replica would go into recovery as expected and `waitForState` would just miss it based on when it polls?
##########
solr/core/src/java/org/apache/solr/cloud/ShardLeaderElectionContext.java:
##########
@@ -250,7 +222,7 @@ void runLeaderProcess(boolean weAreReplacement) throws KeeperException, Interrup
try {
if (log.isDebugEnabled()) {
log.debug(
- "{} synched {}",
+ "{} synced {}",
Review Comment:
lol
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #4069:
URL: https://github.com/apache/solr/pull/4069#discussion_r2934188305
##########
solr/core/src/test/org/apache/solr/cloud/LeaderVoteWaitTimeoutTest.java:
##########
@@ -284,6 +284,15 @@ public void testMostInSyncReplicasCanWinElection() throws Exception {
waitForState("Timeout waiting for 1x3 collection", collectionName, clusterShape(1, 3));
assertDocsExistInAllReplicas(Arrays.asList(leader, replica1), collectionName, 1, 3);
+
+ try (ZkShardTerms zkShardTerms =
Review Comment:
this would have actually failed before this PR. So we are fixing a case where zkShardTerms can fail to be set to the leader term after "recovery" (in this case peer-sync).
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #4069:
URL: https://github.com/apache/solr/pull/4069#issuecomment-4069393497
I think this should be good to go now...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #4069:
URL: https://github.com/apache/solr/pull/4069#discussion_r2742661617
##########
solr/core/src/test-files/log4j2.xml:
##########
@@ -22,7 +22,7 @@
<PatternLayout>
<Pattern>
%maxLen{%-4r %-5p (%t) [%notEmpty{n:%X{node_name}}%notEmpty{ c:%X{collection}}%notEmpty{ s:%X{shard}}%notEmpty{ r:%X{replica}}%notEmpty{ x:%X{core}}%notEmpty{ t:%X{trace_id}}] %c{1.} %m%notEmpty{
- =>%ex{short}}}{10240}%n
+ =>%ex}}{10240}%n
Review Comment:
Yeah, I add this when testing on all my PRs then remove it before committing. This got through unfortunately. But thanks for taking the lead on it, appreciate that! I'll remove it.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18080: Initiate Leader election for ShardTerms [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4069:
URL: https://github.com/apache/solr/pull/4069#discussion_r2726273126
##########
solr/solrj/src/java/org/apache/solr/common/cloud/ZkCoreNodeProps.java:
##########
@@ -47,6 +47,10 @@ public String getCoreName() {
return nodeProps.getStr(ReplicaStateProps.CORE_NAME);
}
+ public String getCoreNodeName() {
Review Comment:
even a little example of what this is/means would be helpful
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1047,8 +1047,7 @@ public static String getShardLeadersElectPath(String collection, String shardId)
+ (shardId != null ? ("/" + shardId + "/" + ELECTION_NODE) : "");
}
- public List<ZkCoreNodeProps> getReplicaProps(
- String collection, String shardId, String thisCoreNodeName) {
+ public List<Replica> getReplicaProps(String collection, String shardId, String thisCoreNodeName) {
Review Comment:
Does the name still make sense, vs `getReplica`?
##########
solr/core/src/test-files/log4j2.xml:
##########
@@ -22,7 +22,7 @@
<PatternLayout>
<Pattern>
%maxLen{%-4r %-5p (%t) [%notEmpty{n:%X{node_name}}%notEmpty{ c:%X{collection}}%notEmpty{ s:%X{shard}}%notEmpty{ r:%X{replica}}%notEmpty{ x:%X{core}}%notEmpty{ t:%X{trace_id}}] %c{1.} %m%notEmpty{
- =>%ex{short}}}{10240}%n
+ =>%ex}}{10240}%n
Review Comment:
Coincidentally, I have this same change in my working tree, and have yet to file a PR to do this everywhere.
##########
solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java:
##########
@@ -546,7 +546,7 @@ protected void doDistribDeleteByQuery(
Replica leaderReplica =
zkController.getZkStateReader().getLeaderRetry(collection, myShardId);
// DBQ forwarded to NRT and TLOG replicas
- List<ZkCoreNodeProps> replicaProps =
Review Comment:
it's a small thing but it's nice to see this little improvement
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org