Skip to content

SOLR-15399: IndexFetcher should not issue a local commit for PULL replicas #133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 20, 2021
Merged

SOLR-15399: IndexFetcher should not issue a local commit for PULL replicas #133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
657ea50
SOLR-15399: Pull replica shouldn't commit locally in IndexFetcher
thelabdude May 18, 2021
94da20e
Fix if logic to only change skipCommitOnLeaderVersionZero for PULL re…
thelabdude May 19, 2021
af64b96
Address review comments and restart a PULL replica before adding docs…
thelabdude May 19, 2021
3402484
Improve assert message
thelabdude May 20, 2021
f6c542d
Merge remote-tracking branch 'asf/main' into SOLR-15399
thelabdude May 20, 2021
a3bbc17
Remove stray comment after refactor
thelabdude May 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions solr/CHANGES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,8 @@ Bug Fixes

* SOLR-15424: Solr replication UI wraps ETA time on top of next line (janhoy)

* SOLR-15399: IndexFetcher should not issue a local commit for PULL replicas when leader's version is zero (Timothy Potter)

Other Changes
---------------------
* SOLR-15118: Deprecate CollectionAdminRequest.getV2Request(). (Jason Gerlowski)
Expand Down
18 changes: 17 additions & 1 deletion solr/core/src/java/org/apache/solr/cloud/ReplicateFromLeader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import org.apache.lucene.index.IndexCommit;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.cloud.Replica;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.util.NamedList;
import org.apache.solr.core.CoreContainer;
Expand Down Expand Up @@ -87,7 +88,22 @@ public void startReplication(boolean switchTransactionLog) {

NamedList<Object> followerConfig = new NamedList<>();
followerConfig.add("fetchFromLeader", Boolean.TRUE);
followerConfig.add(ReplicationHandler.SKIP_COMMIT_ON_LEADER_VERSION_ZERO, switchTransactionLog);

// don't commit on leader version zero for PULL replicas as PULL should only get its index state from leader
boolean skipCommitOnLeaderVersionZero = switchTransactionLog;
if (!skipCommitOnLeaderVersionZero) {
CloudDescriptor cloudDescriptor = core.getCoreDescriptor().getCloudDescriptor();
if (cloudDescriptor != null) {
Replica replica =
cc.getZkController().getZkStateReader().getCollection(cloudDescriptor.getCollectionName())
.getSlice(cloudDescriptor.getShardId()).getReplica(cloudDescriptor.getCoreNodeName());
if (replica != null && replica.getType() == Replica.Type.PULL) {
skipCommitOnLeaderVersionZero = true; // only set this to true if we're a PULL replica, otherwise use value of switchTransactionLog
}
}
}
followerConfig.add(ReplicationHandler.SKIP_COMMIT_ON_LEADER_VERSION_ZERO, skipCommitOnLeaderVersionZero);

followerConfig.add("pollInterval", pollIntervalStr);
NamedList<Object> replicationConfig = new NamedList<>();
replicationConfig.add("follower", followerConfig);
Expand Down
47 changes: 39 additions & 8 deletions solr/core/src/test/org/apache/solr/cloud/TestPullReplica.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
import org.slf4j.LoggerFactory;

@Slow
@LogLevel("org.apache.solr.handler.ReplicationHandler=DEBUG,org.apache.solr.handler.IndexFetcher=DEBUG")
@LogLevel("org.apache.solr.handler.ReplicationHandler=DEBUG;org.apache.solr.handler.IndexFetcher=DEBUG")
public class TestPullReplica extends SolrCloudTestCase {

private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Expand All @@ -80,7 +80,6 @@ private String suggestedCollectionName() {

@BeforeClass
public static void createTestCluster() throws Exception {
// cloudSolrClientMaxStaleRetries
System.setProperty("cloudSolrClientMaxStaleRetries", "1");
System.setProperty("zkReaderGetLeaderRetryTimeoutMs", "1000");

Expand Down Expand Up @@ -122,7 +121,6 @@ public void tearDown() throws Exception {
}

@Repeat(iterations=2) // 2 times to make sure cleanup is complete and we can create the same collection
// commented out on: 17-Feb-2019 @BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // 21-May-2018
public void testCreateDelete() throws Exception {
try {
switch (random().nextInt(3)) {
Expand Down Expand Up @@ -209,15 +207,18 @@ static void assertUlogPresence(DocCollection collection) {
}

@SuppressWarnings("unchecked")
@BadApple(bugUrl = "https://issues.apache.org/jira/browse/SOLR-15399")
public void testAddDocs() throws Exception {
int numPullReplicas = 1 + random().nextInt(3);
CollectionAdminRequest.createCollection(collectionName, "conf", 1, 1, 0, numPullReplicas)
.process(cluster.getSolrClient());
waitForState("Expected collection to be created with 1 shard and " + (numPullReplicas + 1) + " replicas", collectionName, clusterShape(1, numPullReplicas + 1));
waitForState("Expected collection to be created with 1 shard and " + (numPullReplicas + 1) + " replicas",
collectionName, clusterShape(1, numPullReplicas + 1));
DocCollection docCollection = assertNumberOfReplicas(1, 0, numPullReplicas, false, true);
assertEquals(1, docCollection.getSlices().size());

// ugly but needed to ensure a full PULL replication cycle (every sec) has occurred on the replicas before adding docs
Thread.sleep(1500);

boolean reloaded = false;
int numDocs = 0;
while (true) {
Expand All @@ -232,7 +233,8 @@ public void testAddDocs() throws Exception {
}
log.info("Found {} docs in leader, verifying updates make it to {} pull replicas", numDocs, numPullReplicas);

List<Replica> pullReplicas = s.getReplicas(EnumSet.of(Replica.Type.PULL));
List<Replica> pullReplicas =
(numDocs == 1) ? restartPullReplica(docCollection, numPullReplicas) : s.getReplicas(EnumSet.of(Replica.Type.PULL));
waitForNumDocsInAllReplicas(numDocs, pullReplicas);

for (Replica r : pullReplicas) {
Expand All @@ -258,6 +260,37 @@ public void testAddDocs() throws Exception {
assertUlogPresence(docCollection);
}

private List<Replica> restartPullReplica(DocCollection docCollection, int numPullReplicas) throws Exception {
Slice s = docCollection.getSlices().iterator().next();
List<Replica> pullReplicas = s.getReplicas(EnumSet.of(Replica.Type.PULL));

// find a node with a PULL replica that's not hosting the leader
JettySolrRunner leaderJetty = cluster.getReplicaJetty(s.getLeader());
JettySolrRunner replicaJetty = null;
for (Replica r : pullReplicas) {
JettySolrRunner jetty = cluster.getReplicaJetty(r);
if (!jetty.getNodeName().equals(leaderJetty.getNodeName())) {
replicaJetty = jetty;
break;
}
}

// stop / start the node with a pull replica
if (replicaJetty != null) {
replicaJetty.stop();
cluster.waitForJettyToStop(replicaJetty);
waitForState("Expected to see a downed PULL replica", collectionName, clusterStateReflectsActiveAndDownReplicas());
replicaJetty.start();
waitForState("Expected collection to have recovered with 1 shard and " + (numPullReplicas + 1) + " replicas after restarting " + replicaJetty.getNodeName(),
collectionName, clusterShape(1, numPullReplicas + 1));
docCollection = assertNumberOfReplicas(1, 0, numPullReplicas, false, true);
s = docCollection.getSlices().iterator().next();
pullReplicas = s.getReplicas(EnumSet.of(Replica.Type.PULL));
} // else it's ok if all replicas ended up on the same node, we're not testing replica placement here, but skip this part of the test

return pullReplicas;
}

public void testAddRemovePullReplica() throws Exception {
CollectionAdminRequest.createCollection(collectionName, "conf", 2, 1, 0, 0)
.process(cluster.getSolrClient());
Expand All @@ -282,13 +315,11 @@ public void testAddRemovePullReplica() throws Exception {
}

@Test
@BadApple(bugUrl = "https://issues.apache.org/jira/browse/SOLR-15399")
public void testRemoveAllWriterReplicas() throws Exception {
doTestNoLeader(true);
}

@Test
@BadApple(bugUrl = "https://issues.apache.org/jira/browse/SOLR-15399")
public void testKillLeader() throws Exception {
doTestNoLeader(false);
}
Expand Down
3 changes: 2 additions & 1 deletion solr/core/src/test/org/apache/solr/cloud/TestPullReplicaWithAuth.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.apache.solr.security.BasicAuthPlugin;
import org.apache.solr.security.RuleBasedAuthorizationPlugin;
import org.junit.BeforeClass;
import org.junit.Test;

import static java.util.Collections.singletonList;
import static java.util.Collections.singletonMap;
Expand Down Expand Up @@ -84,7 +85,7 @@ private QueryResponse queryWithBasicAuth(HttpSolrClient client, SolrQuery q) thr
return withBasicAuth(new QueryRequest(q)).process(client);
}

@BadApple(bugUrl = "https://issues.apache.org/jira/browse/SOLR-15399")
@Test
public void testPKIAuthWorksForPullReplication() throws Exception {
int numPullReplicas = 2;
withBasicAuth(CollectionAdminRequest.createCollection(collectionName, "conf", 1, 1, 0, numPullReplicas))
Expand Down