SOLR-17066: Switch ConcurrentSolrClients away from core URLs #2254

gerlowskija · 2024-02-08T17:49:58Z

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

Description

SOLR-17066 introduced a withDefaultCollection builder method that can be used in lieu of providing a core URL as a "base". (Core URLs are not ideal as they prevent clients from making core-agnostic requests.)

However, Solr's own client usage still needs modified to use this new builder method.

Solution

This commit migrates Solr's "concurrent update" Solr client implementations away from core URLs and towards using withDefaultCollection. Following this PR, the only client that still remains to be migrated are the two "load balancing" clients.)

Tests

Existing tests continue to pass. No new tests needed, as this is "just" a refactor.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.

SOLR-17066 introduced a withDefaultCollection builder method that can be used in lieu of providing a core URL as a "base". (Core URLs are not ideal as they prevent clients from making core-agnostic requests.) However, Solr's own client usage still needs modified to use this new builder method. This commit migrates all ConcurrentUpdateSolrClient usage away from core URLs and towards using `withDefaultCollection` instead.

SOLR-17066 introduced a withDefaultCollection builder method that can be used in lieu of providing a core URL as a "base". (Core URLs are not ideal as they prevent clients from making core-agnostic requests.) However, Solr's own client usage still needs modified to use this new builder method. This commit migrates all LBHttpSolrClient usage away from core URLs and towards using `withDefaultCollection` instead.

SOLR-17066 introduced a withDefaultCollection builder method that can be used in lieu of providing a core URL as a "base". (Core URLs are not ideal as they prevent clients from making core-agnostic requests.) However, Solr's own client usage still needs modified to use this new builder method. This commit migrates all ConcurrentUpdateHttp2SolrClient usage away from core URLs and towards using `withDefaultCollection` instead.

SOLR-17066 introduced a withDefaultCollection builder method that can be used in lieu of providing a core URL as a "base". (Core URLs are not ideal as they prevent clients from making core-agnostic requests.) However, Solr's own client usage still needs modified to use this new builder method. This commit migrates all LBHttp2SolrClient usage away from core URLs and towards using `withDefaultCollection` instead. Has a test failure in TestLBHttp2SolrClient that still needs resolved.

epugh

LGTM!

epugh · 2024-02-08T17:51:46Z

solr/core/src/test/org/apache/solr/client/solrj/impl/ConnectionReuseTest.java

@@ -83,7 +83,8 @@ private SolrClient buildClient(CloseableHttpClient httpClient, URL url) {
    switch (random().nextInt(3)) {
      case 0:
        // currently, only testing with 1 thread
-        return new ConcurrentUpdateSolrClient.Builder(url.toString() + "/" + COLLECTION)
+        return new ConcurrentUpdateSolrClient.Builder(url.toString())


sigh... if only a builder new what a url was and we didn't .tostring everywhere. ;-) #beatdeadhorse

epugh · 2024-02-08T17:52:51Z

solr/solrj/src/test/org/apache/solr/client/solrj/TestLBHttp2SolrClient.java

@@ -156,7 +158,7 @@ public void testSimple() throws Exception {
      // Start the killed server once again
      solr[1].startJetty();
      // Wait for the alive check to complete
-      Thread.sleep(1200);
+      Thread.sleep(1200 * 5);


1.2 seconds times five?

I have no memory of this 🤔 - will remove.

epugh · 2024-02-08T17:53:06Z

solr/solrj/src/test/org/apache/solr/client/solrj/TestLBHttp2SolrClient.java

@@ -167,17 +169,15 @@ public void testSimple() throws Exception {
    }
  }

-  private LBHttp2SolrClient getLBHttp2SolrClient(Http2SolrClient httpClient, String... s) {


this is nice!

epugh · 2024-02-08T17:54:09Z

.../solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClientBuilderTest.java

@@ -31,6 +31,8 @@
 /** Unit tests for {@link Builder}. */
 public class ConcurrentUpdateSolrClientBuilderTest extends SolrTestCase {

+  private static final String ANY_BASE_URL = "http://localhost:8983/solr";


Zombie-resurrection was broken in LBHttp2SolrClient because the default collection wasn't being taken into account on these requests. The root of the problem is the way that LBHttp2 takes in an already-constructed SolrClient, making it difficult to set the default collection. (In contrast, LBHttpSolrClient creates a different client for each base URL using a common Builder - this allows it to property set the default collection.) This commit fixes this by created a 'DelegatingSolrClient' that LBHttp2 can use internally around its single Http2SolrClient. The default collection is set on this 'Delegating' wrapper, so that it gets picked up by the zombie-resurrection logic.

epugh · 2024-02-09T17:42:05Z

solr/solrj/src/test/org/apache/solr/client/solrj/TestLBHttp2SolrClient.java

@@ -158,7 +158,7 @@ public void testSimple() throws Exception {
      // Start the killed server once again
      solr[1].startJetty();
      // Wait for the alive check to complete
-      Thread.sleep(1200 * 5);
+      Thread.sleep(1200);


The most important code change ;-)

This reverts commit ceb81c9.

This reverts commit e70b2e2.

This reverts commit 410676b.

This reverts commit 06a2065.

This reverts commit 5905b71.

gerlowskija · 2024-02-14T16:25:45Z

Turns out doing this for our 'LB' clients is a little more involved than I initially thought, due to some of the additional public methods they have (e.g. request(LBSolrClient.Req)). I'm going to back the 'LB' changes out of this PR, and focus on only handling our "Concurrent" clients here.

I'll open a separate PR for tackling the LB clients.

Providing a core URL as a SolrClient's "base URL" prevents it from communicating with other cores or making core-agnostic API requests (e.g. node healthcheck, list cores, etc.) This commit migrates all usage of both "concurrent update" clients away from core URLs.

gerlowskija added 5 commits February 2, 2024 11:15

Merge branch 'main' into SOLR-17066-cusc-remove-core-urls-pt-1

4059125

github-actions bot added test-framework client:solrj labels Feb 8, 2024

epugh approved these changes Feb 8, 2024

View reviewed changes

gerlowskija mentioned this pull request Feb 8, 2024

SOLR-17066: Switch Http2SolrClient away from coreURLs #2255

Merged

5 tasks

gerlowskija added 3 commits February 8, 2024 14:48

Fix compilation error

410676b

Make check happy

ceb81c9

epugh reviewed Feb 9, 2024

View reviewed changes

gerlowskija added 6 commits February 14, 2024 11:12

Revert "Make check happy"

3b9f1fd

This reverts commit ceb81c9.

Revert "Fix LBHttp2SolrClient zombie testing"

8a14233

This reverts commit e70b2e2.

Revert "Fix compilation error"

36d0c47

This reverts commit 410676b.

Revert "SOLR-17066: Switch LBHttp2SolrClient away from core URLs"

0af8021

This reverts commit 06a2065.

Revert "SOLR-17066: Switch LBHttpSolrClient away from core URLs"

47f7c65

This reverts commit 5905b71.

Fix check, pt 1

2fd9154

gerlowskija added 2 commits February 14, 2024 11:31

Cleanup

2b91139

Cleanup, 2

0576dbf

gerlowskija changed the title ~~SOLR-17066: Switch most SolrClients away from core URLs~~ SOLR-17066: Switch ConcurrentSolrClients away from core URLs Feb 14, 2024

gerlowskija merged commit 6afbc20 into apache:main Feb 14, 2024
3 of 4 checks passed

gerlowskija deleted the SOLR-17066-cusc-remove-core-urls-pt-1 branch February 14, 2024 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-17066: Switch ConcurrentSolrClients away from core URLs #2254

SOLR-17066: Switch ConcurrentSolrClients away from core URLs #2254

gerlowskija commented Feb 8, 2024 •

edited

Loading

epugh left a comment

epugh Feb 8, 2024

epugh Feb 8, 2024

gerlowskija Feb 8, 2024

epugh Feb 8, 2024

epugh Feb 8, 2024

epugh Feb 9, 2024

gerlowskija commented Feb 14, 2024

SOLR-17066: Switch ConcurrentSolrClients away from core URLs #2254

SOLR-17066: Switch ConcurrentSolrClients away from core URLs #2254

Conversation

gerlowskija commented Feb 8, 2024 • edited Loading

Description

Solution

Tests

Checklist

epugh left a comment

Choose a reason for hiding this comment

epugh Feb 8, 2024

Choose a reason for hiding this comment

epugh Feb 8, 2024

Choose a reason for hiding this comment

gerlowskija Feb 8, 2024

Choose a reason for hiding this comment

epugh Feb 8, 2024

Choose a reason for hiding this comment

epugh Feb 8, 2024

Choose a reason for hiding this comment

epugh Feb 9, 2024

Choose a reason for hiding this comment

gerlowskija commented Feb 14, 2024

gerlowskija commented Feb 8, 2024 •

edited

Loading