SOLR-17043: Remove SolrClient path pattern matching #3238

jkmuriithi · 2025-03-04T19:28:16Z

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

Description

Currently, some SolrClient implementations (especially our "load-balancing" and "cloud" clients) do pattern-matching on the path to guess the "type" (admin, update, etc. ) of each request. This seems unnecessary though, as SolrRequest already has a "getRequestType" method exposing this. We should use this method where possible instead of the ad-hoc pattern matching we currently do, which is brittle and doesn't map well to the varied paths used by our v2 APIs.

Solution

Use SolrRequest.getRequestType to identify request types in SolrJ, and remove other forms of request type identification from the SolrJ codebase. This includes deprecating the IsUpdateRequest interface.
Change getRequestType to return the actual SolrRequestType enum instead of a String.
Some unit tests use SolrRequest.setPath() to send a QueryRequest to a /admin path. This PR adds basic pattern matching in getRequestType and adds method SolrRequest.getBaseRequestType to promote an inheritance pattern which works with mutable request paths.
CloudSolrClient relies heavily on request type to route requests. After modifying the routing in CloudSolrClient to use getRequestType, several requests started failing because they returned request type ADMIN when their handler path was not in CommonParams.ADMIN_PATHS. This PR changes the type of those misclassified requests to UNSPECIFIED. Some of these changes may seem counterintuitive (i.e. /admin/ping and /admin/luke are not ADMIN requests), but they preserve the existing data flow.

Tests

Solr unit tests pass with ./gradlew test.

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, not available for branches on forks living under an organisation)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

kotman12 · 2025-03-05T23:41:40Z

solr/solrj/src/java/org/apache/solr/client/solrj/request/SolrPing.java

-    return SolrRequestType.ADMIN.toString();
+  /* This request is not processed as an ADMIN request. */
+  protected SolrRequestType getBaseRequestType() {
+    return SolrRequestType.UNSPECIFIED;


We know that this is a ping request so this feels misleading .. can the SolrRequestType enum be extended to reflect this, i.e. SolrRequestType.PING or SolrRequestType.NO_SIDE_EFFECT_ADMIN?

Can we make changes like this in a follow-up PR so that this PR is more focused on the refactoring instead of the choice of classifications?

Sure, this comment was a gut reaction to explicitly overriding or "specifying" as UNSPECIFIED, which felt a little off. If we rely on path to determine ADMINness and go the property route (with overloaded constructors), we can at least avoid overiding with UNSPECIFIED at such a low level (I think) because the default/two-arg super constructor would set it. Idk

kotman12 · 2025-03-06T02:11:30Z

solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java

+  public SolrRequestType getRequestType() {
+    if (CommonParams.ADMIN_PATHS.contains(getPath())) {
+      return SolrRequestType.ADMIN;
+    } else if (this instanceof IsUpdateRequest) {


If we are marking as deprecated then maybe it makes sense to stop adding code that relies on this?

Honestly I wasn't a huge fan of this when I wrote it. My goal was just to avoid disrupting existing code that relied on IsUpdateRequest, in order to make this less of a breaking change.

solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java

kotman12 · 2025-03-06T02:26:42Z

solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java

+   * SolrRequest#getRequestType}. Note that changing request type can break/impact request routing
+   * within various clients (i.e. {@link CloudSolrClient}).
+   */
+  protected SolrRequestType getBaseRequestType() {


Just thinking out loud: I wonder if it is possible to make this a member of SolrRequest itself, i.e. private SolrRequestType type

The advantage of this is you don't have to expand the interface with this arguably low-level detail. You could imagine a set of overloaded constructors of SolrRequest, i.e.:

private SolrRequestType type; public SolrRequest(METHOD m, String path) { this(m, path, UNSPECIFIED); } public SolrRequest(METHOD m, String path, SolrRequestType defaultType) { this.method = m; this.path = path; this.type = resolveType(path, defaultType); } public void setPath(String path) { this.path = path; this.type = resolveType(path, type); } public SolrRequestType getRequestType() { return type; }

The catch is that calling setPath has to update this additional property as well. But because getRequestType already depends on path, both solutions are equally mutable. It would be nice if SolrRequest had a builder which could unburden SolrRequest from this mutability but that would be a much bigger change I suppose.

getBaseRequestType seems sad to me; and I sympathize with Luke's musings. I'd like to see this method go away. I like's Luke's suggestion on a field. But going towards immutable is a bit much IMO.

dsmiley

Good stuff here!

dsmiley · 2025-03-07T20:15:07Z

solr/solrj/src/java/org/apache/solr/client/solrj/request/SolrPing.java

-    return SolrRequestType.ADMIN.toString();
+  /* This request is not processed as an ADMIN request. */
+  protected SolrRequestType getBaseRequestType() {
+    return SolrRequestType.UNSPECIFIED;


Can we make changes like this in a follow-up PR so that this PR is more focused on the refactoring instead of the choice of classifications?

dsmiley · 2025-03-07T20:24:31Z

solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java

+   * SolrRequest#getRequestType}. Note that changing request type can break/impact request routing
+   * within various clients (i.e. {@link CloudSolrClient}).
+   */
+  protected SolrRequestType getBaseRequestType() {


getBaseRequestType seems sad to me; and I sympathize with Luke's musings. I'd like to see this method go away. I like's Luke's suggestion on a field. But going towards immutable is a bit much IMO.

dsmiley · 2025-03-07T20:27:40Z

solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java

+   *
+   * @see CloudSolrClient#isUpdatesToLeaders
+   */
+  public boolean shouldSendToLeaders() {


I question that we want this. Maybe this pre-dated org.apache.solr.common.params.ShardParams#SHARDS_PREFERENCE where you can accomplish the same thing. Ideally this method goes away (and goes away from AbstractUpdateRequest).

dsmiley · 2025-03-07T20:27:52Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java

@@ -787,7 +786,7 @@ protected NamedList<Object> requestWithRetryOnStaleState(
    if (request instanceof V2Request) {
      isCollectionRequestOfV2 = ((V2Request) request).isPerCollectionRequest();
    }
-    boolean isAdmin = ADMIN_PATHS.contains(request.getPath());
+    boolean isAdmin = request.getRequestType() == SolrRequestType.ADMIN;


jmuriithi3 added 10 commits February 28, 2025 12:24

Change getRequestType and update CloudSolrClient

7c8efed

Deprecate IsUpdateRequest

2789ad2

Remove references to ADMIN_PATHS

ccd0779

Add getBaseRequestType for more complex use cases

24bfe7d

spotlessApply

8c17b8b

Use getBaseRequestType for everything

5f73e3c

getBaseRequestType should be protected, not public

ffa8600

Fix request routing for misclassified requests

52c3567

doc wording

5b58b7f

gradlew check

b890b07

github-actions bot added test-framework client:solrj tests cat:cloud cat:schema labels Mar 4, 2025

Fix Javadoc warning

64a00bf

kotman12 reviewed Mar 6, 2025

View reviewed changes

dsmiley reviewed Mar 7, 2025

View reviewed changes

dsmiley requested a review from gerlowskija March 7, 2025 20:29

Make getRequestType final

8e6a1c7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-17043: Remove SolrClient path pattern matching #3238

SOLR-17043: Remove SolrClient path pattern matching #3238

jkmuriithi commented Mar 4, 2025

kotman12 Mar 5, 2025

dsmiley Mar 7, 2025

kotman12 Mar 7, 2025 •

edited

Loading

kotman12 Mar 6, 2025

jkmuriithi Mar 10, 2025

kotman12 Mar 6, 2025

dsmiley Mar 7, 2025

dsmiley left a comment

dsmiley Mar 7, 2025

dsmiley Mar 7, 2025

dsmiley Mar 7, 2025

dsmiley Mar 7, 2025

SOLR-17043: Remove SolrClient path pattern matching #3238

Are you sure you want to change the base?

SOLR-17043: Remove SolrClient path pattern matching #3238

Conversation

jkmuriithi commented Mar 4, 2025

Description

Solution

Tests

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kotman12 Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsmiley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kotman12 Mar 7, 2025 •

edited

Loading