dsmiley commented on code in PR #4451:
URL: https://github.com/apache/solr/pull/4451#discussion_r3367935138
##########
solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/HttpJettySolrClientCompatibilityTest.java:
##########
Review Comment:
Changes here were to ensure that the "old nodes" run with Http1 -- as I
assume that is what's meant by "old nodes". Not sure why this test was
failing, causing me to do this.
Ideally this would have been in the misc-test-fixes PR.
##########
solr/core/src/test/org/apache/solr/security/BasicAuthIntegrationTest.java:
##########
@@ -136,9 +135,9 @@ public void testBasicAuth() throws Exception {
}
// avoid bad connection races due to shutdown
- final var apacheHttpClient = ((CloudLegacySolrClient)
cluster.getSolrClient()).getHttpClient();
- apacheHttpClient.getConnectionManager().closeExpiredConnections();
- apacheHttpClient.getConnectionManager().closeIdleConnections(1,
TimeUnit.MILLISECONDS);
+ // var apacheHttpClient = ((CloudLegacySolrClient)
cluster.getSolrClient()).getHttpClient();
+ // apacheHttpClient.getConnectionManager().closeExpiredConnections();
+ // apacheHttpClient.getConnectionManager().closeIdleConnections(1,
TimeUnit.MILLISECONDS);
Review Comment:
leaving commented code here since if this becomes flaky, I want an
investigator to see how this used to be.
##########
solr/core/src/test/org/apache/solr/cloud/BasicDistributedZk2Test.java:
##########
@@ -158,11 +159,25 @@ public void test() throws Exception {
.getZkController()
.getZkClient()
.getCuratorFramework()
- .blockUntilConnected(50, TimeUnit.MILLISECONDS);
+ .blockUntilConnected(30, TimeUnit.SECONDS);
indexr("id", docId + 1, t1, "slip this doc in");
- waitForRecoveriesToFinish(false);
+ // Wait for the session-expired node to fully re-register its replica as
ACTIVE.
Review Comment:
one of the slightly more interesting changes in this massive PR. Probably
should have been in the misc-test-fixes PR.
##########
solr/test-framework/src/java/org/apache/solr/util/SSLTestConfig.java:
##########
Review Comment:
Substantial change... some is simply removing test support specific to
Apache HttpClient test infra. But the more interesting part is that this infra
was using utilities in Apache HttpClient's libraries (see those import
statements) that are now gone so we must do manually what it was doing. So
some increase in code for that.
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -161,7 +163,8 @@ public void beforeTest() {
protected volatile CloudSolrClient controlClientCloud; // cloud version of
the control client
protected volatile CloudSolrClient cloudClient;
- protected final List<SolrClient> coreClients =
Collections.synchronizedList(new ArrayList<>());
+ protected final List<HttpSolrClient> coreClients =
+ Collections.synchronizedList(new ArrayList<>());
Review Comment:
This has wide ripple effects on other tests... but positve because finally
we can see this is an HttpSolrClient and thus stop casting, e.g. just to get
the base URL
##########
solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/graph/GraphExpressionTest.java:
##########
Review Comment:
ideally this was have been done in the "misc-test-fixes" pr, since merged.
ah well.
--
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]
