You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@solr.apache.org by ho...@apache.org on 2023/01/07 00:54:15 UTC
[solr] 06/12: Beef up the assertions we can make by refactoring RecordingResults to track requests and commands (ot just core names)
This is an automated email from the ASF dual-hosted git repository.
hossman pushed a commit to branch jira/SOLR-6312
in repository https://gitbox.apache.org/repos/asf/solr.git
commit 1ad2a161b785cee15db60be1052f6c7f9d78e199
Author: Chris Hostetter <ho...@apache.org>
AuthorDate: Tue Dec 20 11:25:06 2022 -0700
Beef up the assertions we can make by refactoring RecordingResults to track requests and commands (ot just core names)
---
.../impl/SendUpdatesToLeadersOverrideTest.java | 147 ++++++++++++++++-----
1 file changed, 114 insertions(+), 33 deletions(-)
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SendUpdatesToLeadersOverrideTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SendUpdatesToLeadersOverrideTest.java
index af7d86a649b..1e72d6f16f1 100644
--- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SendUpdatesToLeadersOverrideTest.java
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SendUpdatesToLeadersOverrideTest.java
@@ -23,9 +23,11 @@ import static org.hamcrest.Matchers.isIn;
import static org.hamcrest.Matchers.not;
import java.lang.invoke.MethodHandles;
+import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
+import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
@@ -34,6 +36,8 @@ import org.apache.solr.client.solrj.request.CollectionAdminRequest;
import org.apache.solr.client.solrj.request.UpdateRequest;
import org.apache.solr.cloud.SolrCloudTestCase;
import org.apache.solr.common.cloud.Replica;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.update.UpdateCommand;
import org.apache.solr.update.processor.TrackingUpdateProcessorFactory;
import org.junit.AfterClass;
import org.junit.BeforeClass;
@@ -122,14 +126,41 @@ public class SendUpdatesToLeadersOverrideTest extends SolrCloudTestCase {
.collect(Collectors.toUnmodifiableList());
}
+ /** Convinience class for making assertions about the updates that were processed */
private static class RecordingResults {
- public final List<String> preDistribCoreNames;
- public final List<String> postDistribCoreNames;
+ public final List<UpdateCommand> preDistribCommands;
+ public final List<UpdateCommand> postDistribCommands;
+
+ public final Map<SolrQueryRequest, List<UpdateCommand>> preDistribRequests;
+ public final Map<SolrQueryRequest, List<UpdateCommand>> postDistribRequests;
+
+ public final Map<String, List<SolrQueryRequest>> preDistribCores;
+ public final Map<String, List<SolrQueryRequest>> postDistribCores;
+
+ private static Map<SolrQueryRequest, List<UpdateCommand>> mapReqsToCommands(
+ final List<UpdateCommand> commands) {
+ return commands.stream().collect(Collectors.groupingBy(UpdateCommand::getReq));
+ }
+
+ private static Map<String, List<SolrQueryRequest>> mapCoresToReqs(
+ final Collection<SolrQueryRequest> reqs) {
+ return reqs.stream()
+ .collect(
+ Collectors.groupingBy(
+ r -> r.getContext().get(TrackingUpdateProcessorFactory.REQUEST_NODE).toString()));
+ }
public RecordingResults(
- final List<String> preDistribCoreNames, final List<String> postDistribCoreNames) {
- this.preDistribCoreNames = preDistribCoreNames;
- this.postDistribCoreNames = postDistribCoreNames;
+ final List<UpdateCommand> preDistribCommands,
+ final List<UpdateCommand> postDistribCommands) {
+ this.preDistribCommands = preDistribCommands;
+ this.postDistribCommands = postDistribCommands;
+
+ this.preDistribRequests = mapReqsToCommands(preDistribCommands);
+ this.postDistribRequests = mapReqsToCommands(postDistribCommands);
+
+ this.preDistribCores = mapCoresToReqs(preDistribRequests.keySet());
+ this.postDistribCores = mapCoresToReqs(postDistribRequests.keySet());
}
}
@@ -148,13 +179,15 @@ public class SendUpdatesToLeadersOverrideTest extends SolrCloudTestCase {
assertEquals(0, req.process(client, COLLECTION_NAME).getStatus());
final RecordingResults results =
- new RecordingResults(stopRecording("pre-distrib"), stopRecording("post-distrib"));
+ new RecordingResults(
+ TrackingUpdateProcessorFactory.stopRecording("pre-distrib"),
+ TrackingUpdateProcessorFactory.stopRecording("post-distrib"));
// post-distrib should never match any PULL replicas, regardless of request, if this fails
// something is seriously wrong with our cluster
assertThat(
"post-distrib should never be PULL replica",
- results.postDistribCoreNames,
+ results.postDistribCores.keySet(),
everyItem(not(isIn(PULL_REPLICA_CORE_NAMES))));
return results;
@@ -182,7 +215,6 @@ public class SendUpdatesToLeadersOverrideTest extends SolrCloudTestCase {
}
}
- /** nocommit: This test will fail until bug is fixed */
public void testUpdatesWithShardsPrefPull() throws Exception {
try (CloudSolrClient client =
new CloudLegacySolrClient.Builder(
@@ -209,44 +241,71 @@ public class SendUpdatesToLeadersOverrideTest extends SolrCloudTestCase {
assertUpdateWithRecording(new UpdateRequest().add(sdoc("id", "hoss")), client);
// single NRT leader is only core that should be involved at all
- assertThat("add pre-distrib size", add.preDistribCoreNames, hasSize(1));
+ assertThat("add pre-distrib size", add.preDistribCores.keySet(), hasSize(1));
+ assertThat("add pre-distrib size", add.preDistribRequests.keySet(), hasSize(1));
+ assertThat("add pre-distrib size", add.preDistribCommands, hasSize(1));
assertThat(
"add pre-distrib must be leader",
- add.preDistribCoreNames,
+ add.preDistribCores.keySet(),
everyItem(isIn(LEADER_CORE_NAMES)));
assertEquals(
- "add pre and post should match", add.preDistribCoreNames, add.postDistribCoreNames);
+ "add pre and post should match",
+ add.preDistribCores.keySet(),
+ add.postDistribCores.keySet());
+ assertEquals(
+ "add pre and post should be exact same reqs",
+ add.preDistribRequests.keySet(),
+ add.postDistribRequests.keySet());
+ // NOTE: we can't assert the pre/post commands are the same, because they add versioning
// whatever leader our add was routed to, a DBI for the same id should go to the same leader
final RecordingResults del =
assertUpdateWithRecording(new UpdateRequest().deleteById("hoss"), client);
assertEquals(
- "del pre and post should match", add.preDistribCoreNames, add.postDistribCoreNames);
+ "del pre and post should match",
+ del.preDistribCores.keySet(),
+ del.postDistribCores.keySet());
assertEquals(
"add and del should have been routed the same",
- add.preDistribCoreNames,
- del.preDistribCoreNames);
+ add.preDistribCores.keySet(),
+ del.preDistribCores.keySet());
+ assertThat("del pre-distrib size", del.preDistribRequests.keySet(), hasSize(1));
+ assertThat("del pre-distrib size", del.preDistribCommands, hasSize(1));
}
{ // DBQ should start on some leader, and then distrib to both leaders
final RecordingResults record =
assertUpdateWithRecording(new UpdateRequest().deleteByQuery("*:*"), client);
- assertThat("dbq pre-distrib size", record.preDistribCoreNames, hasSize(1));
+ assertThat("dbq pre-distrib size", record.preDistribCores.keySet(), hasSize(1));
assertThat(
"dbq pre-distrib must be leader",
- record.preDistribCoreNames,
+ record.preDistribCores.keySet(),
everyItem(isIn(LEADER_CORE_NAMES)));
+ assertThat("dbq pre-distrib size", record.preDistribRequests.keySet(), hasSize(1));
+ assertThat("dbq pre-distrib size", record.preDistribCommands, hasSize(1));
assertEquals(
"dbq post-distrib must be all leaders",
LEADER_CORE_NAMES,
- new HashSet<>(record.postDistribCoreNames));
+ record.postDistribCores.keySet());
+ assertThat(
+ "dbq post-distrib size",
+ record.postDistribRequests.keySet(),
+ hasSize(LEADER_CORE_NAMES.size()));
+ assertThat(
+ "dbq post-distrib size", record.postDistribCommands, hasSize(LEADER_CORE_NAMES.size()));
}
- // nocommit: check an UpdateRequest with a mix of many doc adds DBIs, ...
- // nocommit: confirm exactly 2 requests, one to each leader
+ { // When we have multiple direct updates for different shards, client will
+ // split them and merge the responses.
+ //
+ // But we should still only see at most one request per shard leader (pre and post)
+ // nocommit: check an UpdateRequest with a mix of many doc adds DBIs, ...
+ // nocommit: confirm no more then 2 pre-requests, one to each leader
+ // nocommit: confirm same pre and post requests
+ }
}
/**
@@ -264,53 +323,75 @@ public class SendUpdatesToLeadersOverrideTest extends SolrCloudTestCase {
assertUpdateWithRecording(prefPull(new UpdateRequest().add(sdoc("id", "hoss"))), client);
// ...should start on (some) PULL replica, since we asked nicely
- assertThat("add pre-distrib size", add.preDistribCoreNames, hasSize(1));
+ assertThat("add pre-distrib size", add.preDistribCores.keySet(), hasSize(1));
assertThat(
"add pre-distrib must be PULL",
- add.preDistribCoreNames,
+ add.preDistribCores.keySet(),
everyItem(isIn(PULL_REPLICA_CORE_NAMES)));
+ assertThat("add pre-distrib size", add.preDistribRequests.keySet(), hasSize(1));
+ assertThat("add pre-distrib size", add.preDistribCommands, hasSize(1));
// ...then be routed to single leader for this id
- assertThat("add post-distrib size", add.postDistribCoreNames, hasSize(1));
+ assertThat("add post-distrib size", add.postDistribCores.keySet(), hasSize(1));
assertThat(
"add post-distrib must be leader",
- add.postDistribCoreNames,
+ add.postDistribCores.keySet(),
everyItem(isIn(LEADER_CORE_NAMES)));
+ assertThat("add post-distrib size", add.postDistribRequests.keySet(), hasSize(1));
+ assertThat("add post-distrib size", add.postDistribCommands, hasSize(1));
// A DBI should also start on (some) PULL replica, since we asked nicely.
//
- // then it should be distributed to whaever leader our add doc (for the same id) was sent to
+ // then it should be distributed to whatever leader our add doc (for the same id) was sent to
final RecordingResults del =
assertUpdateWithRecording(prefPull(new UpdateRequest().deleteById("hoss")), client);
- assertThat("del pre-distrib size", del.preDistribCoreNames, hasSize(1));
+ assertThat("del pre-distrib size", del.preDistribCores.keySet(), hasSize(1));
assertThat(
"del pre-distrib must be PULL",
- del.preDistribCoreNames,
+ del.preDistribCores.keySet(),
everyItem(isIn(PULL_REPLICA_CORE_NAMES)));
+ assertThat("del pre-distrib size", del.preDistribRequests.keySet(), hasSize(1));
+ assertThat("del pre-distrib size", del.preDistribCommands, hasSize(1));
+
assertEquals(
"add and del should have same post-distrib leader",
- add.postDistribCoreNames,
- del.postDistribCoreNames);
+ add.postDistribCores.keySet(),
+ del.postDistribCores.keySet());
+ assertThat("del post-distrib size", del.postDistribRequests.keySet(), hasSize(1));
+ assertThat("del post-distrib size", del.postDistribCommands, hasSize(1));
}
{ // DBQ start on (some) PULL replica, since we asked nicely, then be routed to all leaders
final RecordingResults record =
assertUpdateWithRecording(prefPull(new UpdateRequest().deleteByQuery("*:*")), client);
- assertThat("dbq pre-distrib size", record.preDistribCoreNames, hasSize(1));
+ assertThat("dbq pre-distrib size", record.preDistribCores.keySet(), hasSize(1));
assertThat(
"dbq pre-distrib must be PULL",
- record.preDistribCoreNames,
+ record.preDistribCores.keySet(),
everyItem(isIn(PULL_REPLICA_CORE_NAMES)));
+ assertThat("dbq pre-distrib size", record.preDistribRequests.keySet(), hasSize(1));
+ assertThat("dbq pre-distrib size", record.preDistribCommands, hasSize(1));
assertEquals(
"dbq post-distrib must be all leaders",
LEADER_CORE_NAMES,
- new HashSet<>(record.postDistribCoreNames));
+ record.postDistribCores.keySet());
+ assertThat(
+ "dbq post-distrib size",
+ record.postDistribRequests.keySet(),
+ hasSize(LEADER_CORE_NAMES.size()));
+ assertThat(
+ "dbq post-distrib size", record.postDistribCommands, hasSize(LEADER_CORE_NAMES.size()));
}
- // nocommit: check an UpdateRequest with a mix of many doc adds DBIs, ...
- // nocommit: confirm we get exactly one request, no request splitting kicking in
+ { // When we have multiple direct updates for different shards, client will
+ // split them and merge the responses.
+ //
+ // But we should still only see at most one request per shard leader (pre and post)
+ // nocommit: check an UpdateRequest with a mix of many doc adds DBIs, ...
+ // nocommit: confirm we get exactly one pre request, no request splitting kicking in
+ }
}
}