dsmiley commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1148385132
##########
solr/core/src/test/org/apache/solr/cloud/ForceLeaderTest.java:
##########
@@ -44,6 +46,7 @@
@Nightly // this test is currently too slow for non-nightly
public class ForceLeaderTest extends HttpPartitionTest {
+ public static final String DEFAULT_COLLECTION =
"forceleader_lower_terms_collection";
Review Comment:
Why call this constant that? The "DEFAULT"-ness is unclear while reading
the code... I thought oh this is "collection1" when really it's something test
specific. I suggest simply calling this COLLECTION (sufficient) or
COLLECTION_NAME (longer, I don't prefer but okay).
##########
solr/core/src/test/org/apache/solr/cloud/ForceLeaderTest.java:
##########
@@ -182,17 +184,32 @@ public void testReplicasInLowerTerms() throws Exception {
assertEquals(
"Expected only 2 documents in the index",
2,
- cloudClient.query(params).getResults().getNumFound());
+ cloudClient.query(DEFAULT_COLLECTION,
params).getResults().getNumFound());
}
- bringBackOldLeaderAndSendDoc(testCollectionName, leader, notLeaders, 5);
+ bringBackOldLeaderAndSendDoc(DEFAULT_COLLECTION, leader, notLeaders, 5);
} finally {
log.info("Cleaning up after the test.");
// try to clean up
- attemptCollectionDelete(cloudClient, testCollectionName);
+ attemptCollectionDelete(cloudClient, DEFAULT_COLLECTION);
}
}
+ /**
+ * For this test, we need a cloudClient that is not randomized since we need
to NEVER send the
+ * updates only to the leader. The way the RandomizingCloudSolrClientBuilder
works, you can't
+ * avoid its internal decision-making process to sometimes send updates only
to leaders.
+ */
+ @Override
+ protected CloudSolrClient createCloudClient(String defaultCollection) {
+ CloudLegacySolrClient.Builder builder =
+ new CloudLegacySolrClient.Builder(
+ Collections.singletonList(zkServer.getZkAddress()),
Optional.empty());
+ defaultCollection = DEFAULT_COLLECTION;
Review Comment:
This is highly suspicious; you are ignoring the param. Then don't pass a
param. Or assert it's equivalency to this constant if you must.
##########
solr/core/src/test/org/apache/solr/cloud/ForceLeaderTest.java:
##########
@@ -182,17 +184,32 @@ public void testReplicasInLowerTerms() throws Exception {
assertEquals(
"Expected only 2 documents in the index",
2,
- cloudClient.query(params).getResults().getNumFound());
+ cloudClient.query(DEFAULT_COLLECTION,
params).getResults().getNumFound());
}
- bringBackOldLeaderAndSendDoc(testCollectionName, leader, notLeaders, 5);
+ bringBackOldLeaderAndSendDoc(DEFAULT_COLLECTION, leader, notLeaders, 5);
} finally {
log.info("Cleaning up after the test.");
// try to clean up
- attemptCollectionDelete(cloudClient, testCollectionName);
+ attemptCollectionDelete(cloudClient, DEFAULT_COLLECTION);
}
}
+ /**
+ * For this test, we need a cloudClient that is not randomized since we need
to NEVER send the
+ * updates only to the leader. The way the RandomizingCloudSolrClientBuilder
works, you can't
+ * avoid its internal decision-making process to sometimes send updates only
to leaders.
+ */
+ @Override
+ protected CloudSolrClient createCloudClient(String defaultCollection) {
+ CloudLegacySolrClient.Builder builder =
Review Comment:
"var" please; long name.
Or better yet, don't even store in a var; just do one long line ending in
.build() that you return directly!
##########
solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java:
##########
@@ -92,73 +89,86 @@ public static String createAndSetNewDefaultCollection()
throws Exception {
DEFAULT_TIMEOUT,
TimeUnit.SECONDS,
(n, c) -> DocCollection.isFullyActive(n, c, 2, 2));
- cloudClient.setDefaultCollection(name);
return name;
}
@Test
public void testBasicUpdates() throws Exception {
Review Comment:
You had stated that my `withCollection` idea wasn't going to be useful for
you but I think here it would have allowed you to keep this method as it was,
albeit after you use withCollection in the beginning. Specifying
collectionName every friggin time is tiresome (I suspect you agree).
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractBasicDistributedZkTestBase.java:
##########
@@ -1589,22 +1589,20 @@ private void testMultipleCollections() throws Exception
{
indexDoc("collection2", getDoc(id, "10000000"));
indexDoc("collection2", getDoc(id, "10000001"));
indexDoc("collection2", getDoc(id, "10000003"));
- getCommonCloudSolrClient().setDefaultCollection("collection2");
- getCommonCloudSolrClient().add(getDoc(id, "10000004"));
- getCommonCloudSolrClient().setDefaultCollection(null);
+
+ getSolrClient("collection2").add(getDoc(id, "10000004"));
Review Comment:
BTW just want to thank you again for cleaning up crap like this
##########
solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java:
##########
@@ -947,9 +947,14 @@ private void searchSeveralWays(
responseConsumer.accept(cluster.getSolrClient().query(collectionList,
solrQuery));
} else {
// new CloudSolrClient (random shardLeadersOnly)
- try (CloudSolrClient solrClient = getCloudSolrClient(cluster)) {
- if (random().nextBoolean()) {
- solrClient.setDefaultCollection(collectionList);
+
+ RandomizingCloudSolrClientBuilder builder = new
RandomizingCloudSolrClientBuilder(cluster);
Review Comment:
"var" please; wow that type is long!
##########
solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java:
##########
@@ -258,25 +278,38 @@ public void testDeleteByIdImplicitRouter() throws
Exception {
}
}
+ private String specifyCollectionParameterForCloudSolrClient(
+ SolrClient solrClient, String collectionName) {
+ // For CloudSolrClient we must specify the collection name as a parameter,
+ // however for shard1 and shard2, it's already part of the URL so we can
specify a null
+ // parameter. The
+ // instanceof check makes that decision.
+ String collectionNameRequiredParameter = null;
+ if (solrClient instanceof CloudSolrClient) {
+ collectionNameRequiredParameter = collectionName;
+ }
+ return collectionNameRequiredParameter;
Review Comment:
could be ternary operator one-liner and be just as clear. One liners are
nice, not just for their brevity, but it also often means not naming yet
another variable (and wow this one is a doozy)
##########
solr/core/src/test/org/apache/solr/cloud/ForceLeaderTest.java:
##########
@@ -284,29 +301,38 @@ private void bringBackOldLeaderAndSendDoc(
int numActiveReplicas = getNumberOfActiveReplicas(clusterState,
collection, SHARD1);
assertEquals(1 + notLeaders.size(), numActiveReplicas);
log.info("Sending doc {}...", docid);
- sendDoc(docid);
+ sendDoc(collection, docid);
log.info("Committing...");
- cloudClient.commit();
+ cloudClient.commit(collection);
log.info("Doc {} sent and commit issued", docid);
assertDocsExistInAllReplicas(notLeaders, collection, docid, docid);
assertDocsExistInAllReplicas(Collections.singletonList(leader),
collection, docid, docid);
}
@Override
- protected int sendDoc(int docId) throws Exception {
+ protected int sendDoc(String collectionName, int docId) throws Exception {
SolrInputDocument doc = new SolrInputDocument();
doc.addField(id, String.valueOf(docId));
doc.addField("a_t", "hello" + docId);
- return sendDocsWithRetry(Collections.singletonList(doc), 1, 5, 1);
+ return sendDocsWithRetry(collectionName, Collections.singletonList(doc),
1, 5, 1);
}
private void doForceLeader(String collectionName, String shard)
throws IOException, SolrServerException {
CollectionAdminRequest.ForceLeader forceLeader =
CollectionAdminRequest.forceLeaderElection(collectionName, shard);
+ boolean shardLeadersOnly = random().nextBoolean();
+ var builder =
+ new RandomizingCloudSolrClientBuilder(
+ Collections.singletonList(zkServer.getZkAddress()),
Optional.empty());
+ if (shardLeadersOnly) {
+ builder.sendUpdatesOnlyToShardLeaders();
+ } else {
+ builder.sendUpdatesToAllReplicasInShard();
+ }
Review Comment:
I recommend removing this random choice of who you talk to. I understand
what this test is testing; randomizing this particular aspect is pointless.
##########
solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java:
##########
@@ -307,16 +340,24 @@ public void
testDeleteByIdCompositeRouterWithRouterField() throws Exception {
// including cloudClient helps us test view from other nodes
that aren't the
// leaders...
for (SolrClient c : Arrays.asList(cloudClient, shard1, shard2)) {
+ String queryWithCollectionName =
Review Comment:
Is the "queryWith" part really helpful?
##########
solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java:
##########
@@ -258,25 +278,38 @@ public void testDeleteByIdImplicitRouter() throws
Exception {
}
}
+ private String specifyCollectionParameterForCloudSolrClient(
+ SolrClient solrClient, String collectionName) {
+ // For CloudSolrClient we must specify the collection name as a parameter,
+ // however for shard1 and shard2, it's already part of the URL so we can
specify a null
Review Comment:
I hope we can get to the point that we never ever more bake in the
collection/core name into the URL of the constructor of a Http client. Not
today but someday.
##########
solr/core/src/test/org/apache/solr/cloud/HttpPartitionTest.java:
##########
@@ -563,25 +541,29 @@ protected void assertDocsExistInAllReplicas(
}
}
- protected SolrClient getHttpSolrClient(Replica replica, String coll) {
+ protected SolrClient getHttpSolrClient(Replica replica, String collection) {
Review Comment:
Not a fan of brevity, I see ;-). 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: [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]
