Skip to content
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.

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

SOLR-13880: Add test that creates/reloads/deletes collections #1105

Merged
merged 2 commits into from Apr 30, 2023

Conversation

epugh
Copy link
Contributor

@epugh epugh commented Oct 21, 2022

https://issues.apache.org/jira/browse/SOLR-13880

Description

Migrate test from apache/lucene-solr#986 to current repo.

Solution

Additional testing.

@epugh epugh requested review from risdenk and tflobbe October 21, 2022 19:11
tlogReplicas,
pullReplicas);
HttpGet createCollectionGet = new HttpGet(url);
((CloudLegacySolrClient) cluster.getSolrClient())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this to be CloudLegacySolrClient?

import org.apache.solr.SolrTestCaseJ4.SuppressSSL;
import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.impl.CloudLegacySolrClient;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use non legacy client?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

turns out we use this to get a HttpClient in order to do a direct GET..

private void createCollection(
int numShards, int nrtReplicas, int tlogReplicas, int pullReplicas, String collectionName)
throws SolrServerException, IOException {
switch (random().nextInt(3)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems weird these create strings - don't we have admin API wrappers around these? We shouldn't need to build straight httpget/httppost commands...

@epugh
Copy link
Contributor Author

epugh commented Oct 21, 2022

@risdenk there are a MILLION places where we create raw HttpGet in the tests, I saw that when I tried to migrate things to SolrClient or SolrCloudClient, that is what drives most of the legacy clients in the tests, the need for a getHttpClient() method... It would be great if we had a general pattern for I want to just do a HTTP thing and look at the results. How can I make a http call from the tests to a url?

As far at not using an API, well, there are two examples, one using the SolrJ, the other with the long string....

@risdenk
Copy link
Contributor

risdenk commented Oct 21, 2022

there are a MILLION places where we create raw HttpGet in the tests

yea more of a we can skip it now but probably has to be fixed at some point :)

@epugh
Copy link
Contributor Author

epugh commented Oct 24, 2022

@risdenk what are your thoughts on the CloudLegacySolrClient? Did my explanation of why make sense? Likewise the api call methods, does that make sense? Or need reworking to have this ready to go...

Copy link
Contributor

@risdenk risdenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epugh no reason to block this from merging due to the cloud client. The only thing I would do is beast this new test a few times (~10) to make sure it doesn't fail out of the gate.

Copy link
Member

@tflobbe tflobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I didn't merge this back when I created the PR is that it didn't find any bugs (ran successfully all the time on my machine), so I was debating myself if the coverage added was worth the potential noise of unrelated failures (that we see all the time in Jenkins).
I'm fine with the PR if you guys think it's worth it.

@@ -86,4 +96,107 @@ public void testReloadedLeaderStateAfterZkSessionLoss() throws Exception {

log.info("testReloadedLeaderStateAfterZkSessionLoss succeeded ... shutting down now!");
}

@Repeat(iterations = 5)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it worth having this set to 5? how long does this take? should we have maybe 2 here, and set it higher in nightly? not sure if that's possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two works for me... Makign the change.

@epugh
Copy link
Contributor Author

epugh commented Mar 29, 2023

@risdenk @tflobbe I was looking at the rather long list of PR's I seem to have open on Solr, and want to get them either committed or closed.. I'm up for merging this one unless either of you has a reason to not?

@epugh epugh merged commit c8edffc into apache:main Apr 30, 2023
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants