Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
gerlowskija commented on PR #3238: URL: https://github.com/apache/solr/pull/3238#issuecomment-2827244538 Thanks for catching this @HoustonPutman; I've got a fix I'll commit shortly. -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
jkmuriithi commented on PR #3238: URL: https://github.com/apache/solr/pull/3238#issuecomment-2819080052 > I think this broke the nightly test `TestTlogReplica.testRebalanceLeaders()`: > > ``` > Error 404 Can not find: /solr/tlog_replica_test_rebalance_leaders_shard1_replica_t3/admin/collections > ``` > > This is at `TestTlogReplica:816` Yeah as of this PR those requests doesn't work, since QueryRequest is `requiresCollection=true`. Let me run the nightly tests locally, there could be a few more of these. -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
HoustonPutman commented on PR #3238: URL: https://github.com/apache/solr/pull/3238#issuecomment-2819025088 I think this broke the nightly test `TestTlogReplica.testRebalanceLeaders()`: ``` Error 404 Can not find: /solr/tlog_replica_test_rebalance_leaders_shard1_replica_t3/admin/collections ``` -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
gerlowskija merged PR #3238: URL: https://github.com/apache/solr/pull/3238 -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
gerlowskija commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2048844469
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##
@@ -1045,12 +1048,11 @@ protected NamedList sendRequest(SolrRequest
request, List inp
requestEndpoints.add(new LBSolrClient.Endpoint(chosenNodeUrl));
}
-} else if (ADMIN_PATHS.contains(request.getPath())) {
+} else if (request.getRequestType() == SolrRequestType.ADMIN &&
!request.requiresCollection()) {
Review Comment:
Yep; done.
Now that we have `requiresCollection` I think we can get rid of the "is this
a v2 API" conditional as well. But that's work for a different PR.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
gerlowskija commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2047753765
##
solr/prometheus-exporter/src/java/org/apache/solr/prometheus/scraper/SolrScraper.java:
##
@@ -130,29 +134,35 @@ protected MetricSamples request(SolrClient client,
MetricsQuery query) throws IO
zkHostLabelValue = ((CloudSolrClient)
client).getClusterStateProvider().getQuorumHosts();
}
-QueryRequest queryRequest = new QueryRequest(query.getParameters());
-queryRequest.setPath(query.getPath());
-
-NamedList queryResponse;
+SolrRequestType requestType = SolrRequestType.QUERY;
Review Comment:
Done
##
solr/core/src/test/org/apache/solr/cloud/TestRebalanceLeaders.java:
##
@@ -445,10 +449,12 @@ private void rebalancePropUsingStandardRequest(String
prop)
if (prop.toLowerCase(Locale.ROOT).contains("preferredleader") == false) {
params.set("shardUnique", true);
}
-QueryRequest request = new QueryRequest(params);
-request.setPath("/admin/collections");
-QueryResponse resp = request.process(cluster.getSolrClient());
-assertEquals("Call to rebalanceLeaders failed ", 0, resp.getStatus());
+var request =
+new GenericSolrRequest(METHOD.GET, "/admin/collections",
SolrRequestType.ADMIN, params);
+request.setRequiresCollection(false);
+SimpleSolrResponse resp = request.process(cluster.getSolrClient());
Review Comment:
Done
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
gerlowskija commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2047713433
##
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java:
##
@@ -666,11 +669,13 @@ public void testNonRetryableRequests() throws Exception {
}
ModifiableSolrParams params = new ModifiableSolrParams();
- params.set("qt", adminPath);
params.set("action", "foobar"); // this should cause an error
- QueryRequest req = new QueryRequest(params);
+
+ var request =
+ new GenericSolrRequest(METHOD.GET, adminPath,
SolrRequestType.ADMIN, params);
+ request.setRequiresCollection(false);
Review Comment:
Done
##
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientRetryTest.java:
##
@@ -55,14 +56,17 @@ public void testRetry() throws Exception {
solrClient.add(collectionName, new SolrInputDocument("id", "1"));
ModifiableSolrParams params = new ModifiableSolrParams();
-params.set(CommonParams.QT, "/admin/metrics");
String updateRequestCountKey =
"solr.core.testRetry.shard1.replica_n1:UPDATE./update.requestTimes:count";
params.set("key", updateRequestCountKey);
params.set("indent", "true");
+params.set(CommonParams.WT, "xml");
-QueryResponse response = solrClient.query(collectionName, params,
SolrRequest.METHOD.GET);
-NamedList namedList = response.getResponse();
+var metricsRequest =
+new GenericSolrRequest(METHOD.GET, "/admin/metrics",
SolrRequestType.ADMIN, params);
+metricsRequest.setRequiresCollection(false);
Review Comment:
Done
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
gerlowskija commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2047689209
##
solr/core/src/test/org/apache/solr/cloud/TestRebalanceLeaders.java:
##
@@ -479,8 +485,9 @@ void setPropWithStandardRequest(Slice slice, Replica rep,
String prop)
params.set("shardUnique", "true");
}
-QueryRequest request = new QueryRequest(params);
-request.setPath("/admin/collections");
+var request =
+new GenericSolrRequest(METHOD.GET, "/admin/collections",
SolrRequestType.ADMIN, params);
+request.setRequiresCollection(false);
Review Comment:
Done
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
gerlowskija commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2047754065
##
solr/core/src/test/org/apache/solr/cloud/TestRebalanceLeaders.java:
##
@@ -445,10 +449,12 @@ private void rebalancePropUsingStandardRequest(String
prop)
if (prop.toLowerCase(Locale.ROOT).contains("preferredleader") == false) {
params.set("shardUnique", true);
}
-QueryRequest request = new QueryRequest(params);
-request.setPath("/admin/collections");
-QueryResponse resp = request.process(cluster.getSolrClient());
-assertEquals("Call to rebalanceLeaders failed ", 0, resp.getStatus());
+var request =
+new GenericSolrRequest(METHOD.GET, "/admin/collections",
SolrRequestType.ADMIN, params);
+request.setRequiresCollection(false);
Review Comment:
Done
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
gerlowskija commented on PR #3238: URL: https://github.com/apache/solr/pull/3238#issuecomment-2810718828 Hoping to move this along a bit, I've pushed some fixes to address some of @dsmiley 's smaller style comments. I still have a few more to go, and then will need to re-test, etc. But this LGTM, and I'm going to target merging once I'm able to attend to the last few things. Thanks @jkmuriithi for your patience on this, and @dsmiley for the repeated reviews! -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
gerlowskija commented on code in PR #3238: URL: https://github.com/apache/solr/pull/3238#discussion_r2047697653 ## solr/prometheus-exporter/src/java/org/apache/solr/prometheus/scraper/SolrScraper.java: ## @@ -130,29 +134,35 @@ protected MetricSamples request(SolrClient client, MetricsQuery query) throws IO zkHostLabelValue = ((CloudSolrClient) client).getClusterStateProvider().getQuorumHosts(); } -QueryRequest queryRequest = new QueryRequest(query.getParameters()); -queryRequest.setPath(query.getPath()); - -NamedList queryResponse; +SolrRequestType requestType = SolrRequestType.QUERY; Review Comment: Done -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
gerlowskija commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2047712486
##
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##
@@ -730,11 +733,14 @@ public void testNonRetryableRequests() throws Exception {
}
ModifiableSolrParams params = new ModifiableSolrParams();
- params.set("qt", adminPath);
params.set("action", "foobar"); // this should cause an error
- QueryRequest req = new QueryRequest(params);
+ params.set("qt", adminPath);
+
+ var request =
+ new GenericSolrRequest(METHOD.GET, adminPath,
SolrRequestType.ADMIN, params);
+ request.setRequiresCollection(false);
Review Comment:
Done
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
gerlowskija commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2047711676
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##
@@ -138,8 +136,9 @@ public Set getUrlParamNames() {
public CompletableFuture requestAsync(Req req) {
CompletableFuture apiFuture = new CompletableFuture<>();
Rsp rsp = new Rsp();
-boolean isNonRetryable =
-req.request instanceof IsUpdateRequest ||
ADMIN_PATHS.contains(req.request.getPath());
+boolean isAdmin =
Review Comment:
Yeah, IMO it makes sense to give `SolrRequest` a `isRetryable()` method or
something similar that clients can key off of. But very much out of scope
here...
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
gerlowskija commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2047698897
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##
@@ -1001,25 +999,30 @@ protected NamedList sendRequest(SolrRequest
request, List inp
boolean sendToLeaders = false;
-if (request instanceof IsUpdateRequest) {
- sendToLeaders = ((IsUpdateRequest) request).isSendToLeaders() &&
this.isUpdatesToLeaders();
+if (request.getRequestType() == SolrRequestType.UPDATE) {
+ sendToLeaders = this.isUpdatesToLeaders();
- // Check if we can do a "directUpdate" ...
if (sendToLeaders && request instanceof UpdateRequest) {
-if (inputCollections.size() > 1) {
- throw new SolrException(
- SolrException.ErrorCode.BAD_REQUEST,
- "Update request must be sent to a single collection "
- + "or an alias: "
- + inputCollections);
-}
-String collection =
-inputCollections.isEmpty()
-? null
-: inputCollections.get(0); // getting first mimics HttpSolrCall
-NamedList response = directUpdate((AbstractUpdateRequest)
request, collection);
-if (response != null) {
- return response;
+var updateRequest = (UpdateRequest) request;
Review Comment:
Done
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2040234407
##
solr/core/src/test/org/apache/solr/cloud/TestRebalanceLeaders.java:
##
@@ -445,10 +449,12 @@ private void rebalancePropUsingStandardRequest(String
prop)
if (prop.toLowerCase(Locale.ROOT).contains("preferredleader") == false) {
params.set("shardUnique", true);
}
-QueryRequest request = new QueryRequest(params);
-request.setPath("/admin/collections");
-QueryResponse resp = request.process(cluster.getSolrClient());
-assertEquals("Call to rebalanceLeaders failed ", 0, resp.getStatus());
+var request =
+new GenericSolrRequest(METHOD.GET, "/admin/collections",
SolrRequestType.ADMIN, params);
+request.setRequiresCollection(false);
Review Comment:
this line isn't needed; that's the default
##
solr/core/src/test/org/apache/solr/cloud/TestRebalanceLeaders.java:
##
@@ -479,8 +485,9 @@ void setPropWithStandardRequest(Slice slice, Replica rep,
String prop)
params.set("shardUnique", "true");
}
-QueryRequest request = new QueryRequest(params);
-request.setPath("/admin/collections");
+var request =
+new GenericSolrRequest(METHOD.GET, "/admin/collections",
SolrRequestType.ADMIN, params);
+request.setRequiresCollection(false);
Review Comment:
this line isn't needed; that's the default
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##
@@ -1045,12 +1048,11 @@ protected NamedList sendRequest(SolrRequest
request, List inp
requestEndpoints.add(new LBSolrClient.Endpoint(chosenNodeUrl));
}
-} else if (ADMIN_PATHS.contains(request.getPath())) {
+} else if (request.getRequestType() == SolrRequestType.ADMIN &&
!request.requiresCollection()) {
Review Comment:
Perhaps this line can only be that the request isn't targeting a collection?
##
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java:
##
@@ -666,11 +669,13 @@ public void testNonRetryableRequests() throws Exception {
}
ModifiableSolrParams params = new ModifiableSolrParams();
- params.set("qt", adminPath);
params.set("action", "foobar"); // this should cause an error
- QueryRequest req = new QueryRequest(params);
+
+ var request =
+ new GenericSolrRequest(METHOD.GET, adminPath,
SolrRequestType.ADMIN, params);
+ request.setRequiresCollection(false);
Review Comment:
that's the default; can remove this line
##
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientRetryTest.java:
##
@@ -55,14 +56,17 @@ public void testRetry() throws Exception {
solrClient.add(collectionName, new SolrInputDocument("id", "1"));
ModifiableSolrParams params = new ModifiableSolrParams();
-params.set(CommonParams.QT, "/admin/metrics");
String updateRequestCountKey =
"solr.core.testRetry.shard1.replica_n1:UPDATE./update.requestTimes:count";
params.set("key", updateRequestCountKey);
params.set("indent", "true");
+params.set(CommonParams.WT, "xml");
-QueryResponse response = solrClient.query(collectionName, params,
SolrRequest.METHOD.GET);
-NamedList namedList = response.getResponse();
+var metricsRequest =
+new GenericSolrRequest(METHOD.GET, "/admin/metrics",
SolrRequestType.ADMIN, params);
+metricsRequest.setRequiresCollection(false);
Review Comment:
that's the default; can remove this line
##
solr/core/src/test/org/apache/solr/cloud/TestRebalanceLeaders.java:
##
@@ -445,10 +449,12 @@ private void rebalancePropUsingStandardRequest(String
prop)
if (prop.toLowerCase(Locale.ROOT).contains("preferredleader") == false) {
params.set("shardUnique", true);
}
-QueryRequest request = new QueryRequest(params);
-request.setPath("/admin/collections");
-QueryResponse resp = request.process(cluster.getSolrClient());
-assertEquals("Call to rebalanceLeaders failed ", 0, resp.getStatus());
+var request =
+new GenericSolrRequest(METHOD.GET, "/admin/collections",
SolrRequestType.ADMIN, params);
+request.setRequiresCollection(false);
+SimpleSolrResponse resp = request.process(cluster.getSolrClient());
Review Comment:
minor but declaring as SimpleSolrResponse is overly specific. I'd just use
`var`
##
solr/prometheus-exporter/src/java/org/apache/solr/prometheus/scraper/SolrScraper.java:
##
@@ -130,29 +134,35 @@ protected MetricSamples request(SolrClient client,
MetricsQuery query) throws IO
zkHostLabelValue = ((CloudSolrClient)
client).getClusterStateProvider().getQuorumHosts();
}
-QueryRequest queryRequest = new QueryRequest(query.getParameters());
-queryRequest.setPath(query.getPath());
-
-NamedList queryResponse;
+SolrRequestType re
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
jkmuriithi commented on PR #3238: URL: https://github.com/apache/solr/pull/3238#issuecomment-2795076015 Think the changes should be good now. I went through and changed problematic tests, and I fixed the routing logic issues with ADMIN requests by adding a check for `requiresCollection`. -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2032402387
##
solr/core/src/test/org/apache/solr/security/BasicAuthIntegrationTest.java:
##
@@ -142,13 +142,14 @@ public void testBasicAuth() throws Exception {
.build();
} else {
GenericSolrRequest genericSolrRequest =
-new GenericSolrRequest(SolrRequest.METHOD.POST, authcPrefix);
+new GenericSolrRequest(
+SolrRequest.METHOD.POST, authcPrefix,
SolrRequest.SolrRequestType.ADMIN);
genericSolrRequest.setContentWriter(
new StringPayloadContentWriter(command, CommonParams.JSON_MIME));
genericReq = genericSolrRequest;
}
- // avoid bad connection races due to shutdown
+ // avoid bad connection races due to shutdownn
Review Comment:
typo
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2006610968
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +193,21 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * The type of this Solr request.
+ *
+ * Pattern matches {@link SolrRequest#getPath} to identify ADMIN requests
and other special
+ * cases. Overriding this method may affect request routing within various
clients (i.e. {@link
+ * CloudSolrClient}).
+ */
+ public SolrRequestType getRequestType() {
+String path = getPath();
+if (path != null && CommonParams.ADMIN_PATHS.contains(path)) {
Review Comment:
Speaking of "qt": I filed
[SOLR-17715](https://issues.apache.org/jira/browse/SOLR-17715) to remove it for
SolrJ 10.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on PR #3238: URL: https://github.com/apache/solr/pull/3238#issuecomment-2760234752 Please let us know when it's believed this PR is ready for reviewers again (feedback has been addressed). I'm looking forward to this nice change! -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
gerlowskija commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2001372713
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -210,6 +236,16 @@ public boolean requiresCollection() {
return false;
}
+ /**
+ * Indicates if clients should make attempts to route this request to a
shard leader, overriding
+ * typical client routing preferences for requests. Defaults to true.
+ *
+ * @see CloudSolrClient#isUpdatesToLeaders
+ */
+ public boolean shouldSendToLeaders() {
Review Comment:
> Any way, I don't think it makes sense for this to be on SolrRequest
generally.
@dsmiley is this more "sharing your thoughts", or do you want Jude to
address this in some way? Just want to double-check so there's no ambiguity.
I agree with you that UpdateRequest should probably re-use
`shards.preference` instead of offering additional methods to impact routing.
(Or perhaps - maybe UpdateRequest offers a few syntax-sugar methods to set
common preferences, but those operate by modifying `shards.preference` rather
than maintaining a separate boolean?)...but all of that feels out of scope
in a PR that was proposed initially as a refactor?
If you feel strongly about the method being out of place on SolrRequest,
maybe the best approach for now is to file a follow-up ticket, and to hold off
touching the UpdateRequest stuff at all in this PR?
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
gerlowskija commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2001444583
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/GenericSolrRequest.java:
##
@@ -50,7 +50,7 @@ public GenericSolrRequest(METHOD m, String path) {
* @param params query parameter names and values for making this request.
*/
public GenericSolrRequest(METHOD m, String path, SolrParams params) {
-super(m, path);
+super(m, path, SolrRequestType.UNSPECIFIED);
Review Comment:
> I could see defaulting to ADMIN only if the path starts with "/admin" and
otherwise throwing an IllegalArgumentException
+1 to the additional ctor so that callers can specify their own request-type
as desired, but I dislike the suggestion around conditionally defaulting to
"ADMIN", for two reasons.
1. Some built-in paths containing "admin" are emphatically NOT "ADMIN" APIs;
`/admin/security` being a particularly thorny example. And the assumption gets
even rockier when you start considering users who might want GenericSolrRequest
to hit their custom endpoints. The whole purpose of the class is to be generic
- making assumptions about the type of request (beyond "UNSPECIFIED") seems
shaky IMO.
2. One of the main purposes of this JIRA/PR is to get rid of path-inspection
and string comparison, which tends to be brittle. Let's not just shift it to a
different place!
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2001636812
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -210,6 +236,16 @@ public boolean requiresCollection() {
return false;
}
+ /**
+ * Indicates if clients should make attempts to route this request to a
shard leader, overriding
+ * typical client routing preferences for requests. Defaults to true.
+ *
+ * @see CloudSolrClient#isUpdatesToLeaders
+ */
+ public boolean shouldSendToLeaders() {
Review Comment:
> If you feel strongly about the method being out of place on SolrRequest
Very much so.
[-1] to `ShardRequest.shouldSendToLeaders` since it's usage is actually only
for updates; isn't generally controlling all requests (nor should it). We
could just leave Solr as it is with regards to this boolean in `UpdateRequest`
& `IsUpdateRequest`. Maybe there are other possible solutions. Maybe it
doesn't need to exist if users will configure CloudSolrClient with the setting
and not change it; not sure.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
gerlowskija commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2001404933
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +193,21 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * The type of this Solr request.
+ *
+ * Pattern matches {@link SolrRequest#getPath} to identify ADMIN requests
and other special
+ * cases. Overriding this method may affect request routing within various
clients (i.e. {@link
+ * CloudSolrClient}).
+ */
+ public SolrRequestType getRequestType() {
+String path = getPath();
+if (path != null && CommonParams.ADMIN_PATHS.contains(path)) {
Review Comment:
> [re: 'qt' param] I almost removed it completely as one of my first big
actions as a committer
It's been on my list for awhile, as it's caused problems with the v2 API as
well. We really should nuke it across the board...
> The reason this is necessary at the moment is because of this pattern,
which is present in several unit tests
I think this pattern is mostly a result of gaps in SolrJ. We didn't offer a
"reload core" SolrRequest implementation at some point, so a test-author hacked
around it by forcing the request into a QueryRequest. But I wonder how many of
those gaps have closed up over time.
We have a reload-core SolrRequest now, on the v2 side at least:
`org.apache.solr.client.solrj.request.CoresApi.ReloadCore`. That should be
safe to use in tests. And even where there's still a SolrJ gap we should still
be able to replace could these QueryRequest mis-uses with GenericSolrRequest I
think?
Is there's not that many instances of the pattern, maybe that's a path
forward as well? (Though if it's too much scope creep, I'd also understand
that...)
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
jkmuriithi commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2001458019
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +193,21 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * The type of this Solr request.
+ *
+ * Pattern matches {@link SolrRequest#getPath} to identify ADMIN requests
and other special
+ * cases. Overriding this method may affect request routing within various
clients (i.e. {@link
+ * CloudSolrClient}).
+ */
+ public SolrRequestType getRequestType() {
+String path = getPath();
+if (path != null && CommonParams.ADMIN_PATHS.contains(path)) {
Review Comment:
I can go ahead and make the changes to the tests, I agree that it seems like
the best path forwards.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2001166122
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/schema/AbstractSchemaRequest.java:
##
@@ -30,17 +30,12 @@ public AbstractSchemaRequest(METHOD m, String path) {
}
public AbstractSchemaRequest(METHOD m, String path, SolrParams params) {
-super(m, path);
+super(m, path, SolrRequestType.UNSPECIFIED);
Review Comment:
again; lets not change that in this PR
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/FieldAnalysisRequest.java:
##
@@ -39,7 +39,7 @@ public class FieldAnalysisRequest extends
CollectionRequiringSolrRequest queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * The type of this Solr request.
+ *
+ * Pattern matches {@link SolrRequest#getPath} to identify ADMIN requests
and other special
+ * cases. Overriding this method may affect request routing within various
clients (i.e. {@link
+ * CloudSolrClient}).
+ */
+ public SolrRequestType getRequestType() {
+String path = getPath();
+if (path != null && CommonParams.ADMIN_PATHS.contains(path)) {
Review Comment:
I could sympathize if my suggestion increases the scope of the PR but I
still think we shouldn't play games in this method.
What could help is having request constructors that don't take the request
type but *insist* the path is obviously admin, so we default to admin or
otherwise throw IllegalArgumentException. That would cut down on many changes.
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/LukeRequest.java:
##
@@ -35,11 +35,12 @@ public class LukeRequest extends
CollectionRequiringSolrRequest {
private Boolean includeIndexFieldFlags = null;
public LukeRequest() {
-super(METHOD.GET, "/admin/luke");
+// this request is not processed as an ADMIN request
+super(METHOD.GET, "/admin/luke", SolrRequestType.UNSPECIFIED);
Review Comment:
I thought we agreed to change the classification separately (another PR)
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/GenericSolrRequest.java:
##
@@ -50,7 +50,7 @@ public GenericSolrRequest(METHOD m, String path) {
* @param params query parameter names and values for making this request.
*/
public GenericSolrRequest(METHOD m, String path, SolrParams params) {
-super(m, path);
+super(m, path, SolrRequestType.UNSPECIFIED);
Review Comment:
Should overload the constructor and encourage specifying this param. II
could see defaulting to ADMIN *only* if the path starts with "/admin" and
otherwise throwing an IllegalArgumentException, thus compelling users to be
explicit while also benefiting from the implied ADMIN from the path when that's
commonly the case.
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/V2Request.java:
##
@@ -50,7 +50,7 @@ public class V2Request extends SolrRequest
implements MapWriter {
private ResponseParser parser;
private V2Request(METHOD m, String resource, boolean useBinary) {
-super(m, resource);
+super(m, resource, SolrRequestType.UNSPECIFIED);
Review Comment:
keep ADMIN
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##
@@ -988,8 +987,8 @@ protected NamedList sendRequest(SolrRequest
request, List inp
boolean sendToLeaders = false;
-if (request instanceof IsUpdateRequest) {
- sendToLeaders = ((IsUpdateRequest) request).isSendToLeaders() &&
this.isUpdatesToLeaders();
+if (request.getRequestType() == SolrRequestType.UPDATE) {
+ sendToLeaders = request.shouldSendToLeaders() &&
this.isUpdatesToLeaders();
Review Comment:
I know casting is never elegant but shouldSendToLeaders() doesn't belong on
SolrRequest base class
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
jkmuriithi commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1999402882
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +193,21 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * The type of this Solr request.
+ *
+ * Pattern matches {@link SolrRequest#getPath} to identify ADMIN requests
and other special
+ * cases. Overriding this method may affect request routing within various
clients (i.e. {@link
+ * CloudSolrClient}).
+ */
+ public SolrRequestType getRequestType() {
+String path = getPath();
+if (path != null && CommonParams.ADMIN_PATHS.contains(path)) {
Review Comment:
Some pattern matching is necessary here to make the APIs work as intended.
Moving that logic to `SolrRequest` seemed like a good idea when I first made
this change, but I am kind of second-guessing that now.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
jkmuriithi commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1999394354
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +193,21 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * The type of this Solr request.
+ *
+ * Pattern matches {@link SolrRequest#getPath} to identify ADMIN requests
and other special
+ * cases. Overriding this method may affect request routing within various
clients (i.e. {@link
+ * CloudSolrClient}).
+ */
+ public SolrRequestType getRequestType() {
+String path = getPath();
+if (path != null && CommonParams.ADMIN_PATHS.contains(path)) {
Review Comment:
The primary consequence of misclassifying requests is that it changes how
the requests are routed in `CloudSolrClient` due to this [code
block](https://github.com/apache/solr/blob/af97ef7037aec0c1fb028e173b6d8931cd12588d/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java#L1027-L1134).
If an `ADMIN` request is routed like a non-ADMIN request, or vice-versa, the
result is a 404 when using this client. My apologies for not being more
specific about this.
The root cause of the problem seems to be that `ADMIN` endpoint URLs are
constructed this way:
```java
final var nodeBaseUrl = Utils.getBaseUrlForNodeName(liveNode, urlScheme);
requestEndpoints.add(new LBSolrClient.Endpoint(nodeBaseUrl));
```
while typical endpoint URLs are constructed this way:
```java
String joinedInputCollections = StrUtils.join(inputCollections, ',');
final var endpoints =
preferredNodes.stream()
.map(nodeName -> Utils.getBaseUrlForNodeName(nodeName, urlScheme))
.map(nodeUrl -> new LBSolrClient.Endpoint(nodeUrl,
joinedInputCollections))
.collect(Collectors.toList());
```
So request type is being used to determine whether or not to include the
name of the collection in the final endpoint URL.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1999351627
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +193,21 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * The type of this Solr request.
+ *
+ * Pattern matches {@link SolrRequest#getPath} to identify ADMIN requests
and other special
+ * cases. Overriding this method may affect request routing within various
clients (i.e. {@link
+ * CloudSolrClient}).
+ */
+ public SolrRequestType getRequestType() {
+String path = getPath();
+if (path != null && CommonParams.ADMIN_PATHS.contains(path)) {
Review Comment:
Are there any "real" risks of mis-classification of the request type in
SolrJ?
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1999350676
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +193,21 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * The type of this Solr request.
+ *
+ * Pattern matches {@link SolrRequest#getPath} to identify ADMIN requests
and other special
+ * cases. Overriding this method may affect request routing within various
clients (i.e. {@link
+ * CloudSolrClient}).
+ */
+ public SolrRequestType getRequestType() {
+String path = getPath();
+if (path != null && CommonParams.ADMIN_PATHS.contains(path)) {
Review Comment:
I see "qt" makes this harder. I hate it; I _almost_ removed it completely
as one of my first big actions as a committer, forever ago, but I went half-way
by removing its processing server-side. It's _still_ present in SolrJ as you
see here; _sigh_. I'd love to see this gone. I know it's a convenience for
tests that want a SolrParams to be sort of all-encompassing about the request.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1999342437
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +193,21 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * The type of this Solr request.
+ *
+ * Pattern matches {@link SolrRequest#getPath} to identify ADMIN requests
and other special
+ * cases. Overriding this method may affect request routing within various
clients (i.e. {@link
+ * CloudSolrClient}).
+ */
+ public SolrRequestType getRequestType() {
+String path = getPath();
+if (path != null && CommonParams.ADMIN_PATHS.contains(path)) {
Review Comment:
Those unit tests are then "wrong". QueryRequest is only for a query, not
requests generally. It's parameterized response type is QueryResponse holding
all the interesting information from `/select`. Note that, unfortunately, some
classes in Solr include the word "Query" when it shouldn't but QueryRequest
isn't one of them.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
jkmuriithi commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1999136119
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +193,21 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * The type of this Solr request.
+ *
+ * Pattern matches {@link SolrRequest#getPath} to identify ADMIN requests
and other special
+ * cases. Overriding this method may affect request routing within various
clients (i.e. {@link
+ * CloudSolrClient}).
+ */
+ public SolrRequestType getRequestType() {
+String path = getPath();
+if (path != null && CommonParams.ADMIN_PATHS.contains(path)) {
Review Comment:
The reason this is necessary at the moment is because of this pattern, which
is present in several unit tests across the codebase (example from
[TestReplicationHandler](https://github.com/apache/solr/blob/af97ef7037aec0c1fb028e173b6d8931cd12588d/solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java#L221-L225)):
```java
ModifiableSolrParams params = new ModifiableSolrParams();
params.set("action", "reload");
params.set("core", core);
params.set("qt", "/admin/cores");
QueryRequest req = new QueryRequest(params);
```
Doing pattern matching on `getPath` allows this type of path manipulation to
"just work" with `CloudSolrClient`, at the cost of introducing more complexity
into `SolrRequest`. I considered going through and changing each instance of
this pattern across all unit tests, but I felt like it might expand the scope
of the PR a bit too much.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1997661934
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -210,6 +236,16 @@ public boolean requiresCollection() {
return false;
}
+ /**
+ * Indicates if clients should make attempts to route this request to a
shard leader, overriding
+ * typical client routing preferences for requests. Defaults to true.
+ *
+ * @see CloudSolrClient#isUpdatesToLeaders
+ */
+ public boolean shouldSendToLeaders() {
Review Comment:
I looked at this closer just now.`shards.preference` routes the entire
request whereas `IsUpdateRequest.isSendToLeaders` is about each document in the
request, individually routing it. So not the same. Any way, I don't think it
makes sense for this to be on SolrRequest generally.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1997243850
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +193,21 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * The type of this Solr request.
+ *
+ * Pattern matches {@link SolrRequest#getPath} to identify ADMIN requests
and other special
+ * cases. Overriding this method may affect request routing within various
clients (i.e. {@link
+ * CloudSolrClient}).
+ */
+ public SolrRequestType getRequestType() {
+String path = getPath();
+if (path != null && CommonParams.ADMIN_PATHS.contains(path)) {
Review Comment:
this part is troubling IMO; I think we should just return the field of the
requestType. Therefore each subclass needs to provide it.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1997244468
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/SolrPing.java:
##
@@ -38,7 +38,8 @@ public class SolrPing extends
CollectionRequiringSolrRequest {
/** Create a new SolrPing object. */
public SolrPing() {
-super(METHOD.GET, CommonParams.PING_HANDLER);
+// this request is not processed as an admin request
+super(METHOD.GET, CommonParams.PING_HANDLER, SolrRequestType.UNSPECIFIED);
Review Comment:
ADMIN
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/schema/AbstractSchemaRequest.java:
##
@@ -38,9 +38,4 @@ public AbstractSchemaRequest(METHOD m, String path,
SolrParams params) {
public SolrParams getParams() {
return params;
}
-
- @Override
- public String getRequestType() {
-return SolrRequestType.ADMIN.toString();
Review Comment:
forgot to supply in constructor
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +193,21 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * The type of this Solr request.
+ *
+ * Pattern matches {@link SolrRequest#getPath} to identify ADMIN requests
and other special
+ * cases. Overriding this method may affect request routing within various
clients (i.e. {@link
+ * CloudSolrClient}).
+ */
+ public SolrRequestType getRequestType() {
+String path = getPath();
+if (path != null && CommonParams.ADMIN_PATHS.contains(path)) {
Review Comment:
this part is troubling IMO; I think we should just return the field of the
requestType
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -90,6 +92,7 @@ public enum SolrClientContext {
private METHOD method = METHOD.GET;
private String path = null;
+ private SolrRequestType defaultType = SolrRequestType.UNSPECIFIED;
Review Comment:
can we just call this field requestType? The word "default" isn't needed.
##
solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java:
##
@@ -1032,11 +1031,6 @@ public Boolean call() throws Exception {
protected SolrResponse createResponse(SolrClient client) {
return null;
}
-
-@Override
-public String getRequestType() {
Review Comment:
If you remove this here then you should supply ADMIN in the constructor.
This underscores a point I make in my review here that it should be explicit;
not an optional parameter.
##
solr/test-framework/src/java/org/apache/solr/cloud/ConfigRequest.java:
##
@@ -56,9 +56,4 @@ public RequestWriter.ContentWriter getContentWriter(String
expectedType) {
public SolrResponse createResponse(SolrClient client) {
return new SolrResponseBase();
}
-
- @Override
- public String getRequestType() {
-return SolrRequest.SolrRequestType.ADMIN.toString();
Review Comment:
forgot to then pass in constructor
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/V2Request.java:
##
@@ -134,11 +134,6 @@ public ResponseParser getResponseParser() {
return super.getResponseParser();
}
- @Override
- public String getRequestType() {
-return SolrRequestType.ADMIN.toString();
Review Comment:
forgot to supply in constructor
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -210,6 +236,16 @@ public boolean requiresCollection() {
return false;
}
+ /**
+ * Indicates if clients should make attempts to route this request to a
shard leader, overriding
+ * typical client routing preferences for requests. Defaults to true.
+ *
+ * @see CloudSolrClient#isUpdatesToLeaders
+ */
+ public boolean shouldSendToLeaders() {
Review Comment:
Indeed, the author has made this method more prominent by elevating to
SolrRequest, so my feedback is relevant & timely. I don't like this method
here at all (or anywhere).
> Are you suggesting we remove it altogether in favor of UpdateRequest
setting a shards.preference value, or something similar?
Exactly.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1997236700
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -130,9 +130,14 @@ public String getBasicAuthPassword() {
// -
// -
- public SolrRequest(METHOD m, String path) {
+ public SolrRequest(METHOD m, String path, SolrRequestType defaultType) {
this.method = m;
this.path = path;
+this.defaultType = defaultType;
+ }
+
+ public SolrRequest(METHOD m, String path) {
Review Comment:
The request type here would then be subtle/hidden at the caller site. I
think the caller of SolrRequest's constructor should be explicit about this.
##
solr/solrj/src/resources/java-template/api.mustache:
##
@@ -218,12 +219,6 @@ public class {{classname}} {
}
{{/bodyParam}}
-// TODO Hardcode this for now, but in reality we'll want to parse
this out of the Operation data somehow
Review Comment:
you removed this comment but isn't it still relevant?
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionRequiringSolrRequest.java:
##
@@ -22,6 +22,10 @@
/** Parent {@link SolrRequest} class that requires a target collection or
core. */
public abstract class CollectionRequiringSolrRequest
extends SolrRequest {
+ public CollectionRequiringSolrRequest(METHOD m, String path, SolrRequestType
defaultType) {
Review Comment:
why use the word "default" in "defaultType? why not just "requestType"?
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1997236700
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -130,9 +130,14 @@ public String getBasicAuthPassword() {
// -
// -
- public SolrRequest(METHOD m, String path) {
+ public SolrRequest(METHOD m, String path, SolrRequestType defaultType) {
this.method = m;
this.path = path;
+this.defaultType = defaultType;
+ }
+
+ public SolrRequest(METHOD m, String path) {
Review Comment:
The request type here would then be subtle/hidden at the caller site. I
think the caller of SolrRequest's constructor should be explicit about this,
just as they are explicit about the method.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
jkmuriithi commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1996350108
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +188,31 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * Defines the intended type of this Solr request.
+ *
+ * Subclasses should typically override this method instead of {@link
+ * SolrRequest#getRequestType}. Note that changing request type can
break/impact request routing
+ * within various clients (i.e. {@link CloudSolrClient}).
+ */
+ protected SolrRequestType getBaseRequestType() {
+return SolrRequestType.UNSPECIFIED;
+ }
+
+ /**
+ * Pattern matches on the underlying {@link SolrRequest} to identify ADMIN
requests and other
+ * special cases. If no special case is identified, {@link
SolrRequest#getBaseRequestType()} is
+ * returned.
+ */
+ public SolrRequestType getRequestType() {
+if (CommonParams.ADMIN_PATHS.contains(getPath())) {
+ return SolrRequestType.ADMIN;
+} else if (this instanceof IsUpdateRequest) {
Review Comment:
Removed this
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
jkmuriithi commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1996349605
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +188,31 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * Defines the intended type of this Solr request.
+ *
+ * Subclasses should typically override this method instead of {@link
+ * SolrRequest#getRequestType}. Note that changing request type can
break/impact request routing
+ * within various clients (i.e. {@link CloudSolrClient}).
+ */
+ protected SolrRequestType getBaseRequestType() {
Review Comment:
Resolving the type in the constructor breaks `QueryRequest`, since it can
only handle calls to `SolrRequest.getPath()` after construction. I removed
`getBaseRequestType`, added a field `defaultType`, and put the request type
pattern matching logic in `getRequestType`. Does this compromise work?
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1994350312
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/DirectXmlRequest.java:
##
@@ -55,8 +55,13 @@ public SolrParams getParams() {
}
@Override
- public String getRequestType() {
-return SolrRequestType.UPDATE.toString();
+ protected SolrRequestType getBaseRequestType() {
Review Comment:
I already did that:
https://github.com/apache/solr/blob/cbc2321598f30066dab4ba396e6c593577f9d8f4/solr/solrj/src/java/org/apache/solr/client/solrj/request/DirectXmlRequest.java#L33
I should merge that PR so we can more easily communicate to even each other
what things are going away so we shouldn't spend attention on
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1994346969
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/DelegationTokenRequest.java:
##
@@ -84,8 +84,8 @@ public DelegationTokenResponse.Get createResponse(SolrClient
client) {
}
@Override
-public String getRequestType() {
- return SolrRequestType.ADMIN.toString();
+protected SolrRequestType getBaseRequestType() {
Review Comment:
@risdenk I also at-mentioned you on that point on the related Hadoop removal
PR since you are the SME on this (not Pugh).
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1994348390
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/DirectXmlRequest.java:
##
@@ -55,8 +55,13 @@ public SolrParams getParams() {
}
@Override
- public String getRequestType() {
-return SolrRequestType.UPDATE.toString();
+ protected SolrRequestType getBaseRequestType() {
Review Comment:
IMO we should deprecate `DirectXmlRequest`. It's unused, untested, and not
needed. I'll do so in my deprecation branch/PR
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1994305927
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -210,6 +236,16 @@ public boolean requiresCollection() {
return false;
}
+ /**
+ * Indicates if clients should make attempts to route this request to a
shard leader, overriding
+ * typical client routing preferences for requests. Defaults to true.
+ *
+ * @see CloudSolrClient#isUpdatesToLeaders
+ */
+ public boolean shouldSendToLeaders() {
Review Comment:
I should have clarified that my comment wasn't a critique of this PR. The
presence of this boolean seemed to somewhat get in the way of this PR (?) so I
chose this moment to share my thoughts. Feel free to ignore!
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
gerlowskija commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1991496264
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##
@@ -139,7 +137,8 @@ public CompletableFuture requestAsync(Req req) {
CompletableFuture apiFuture = new CompletableFuture<>();
Rsp rsp = new Rsp();
boolean isNonRetryable =
-req.request instanceof IsUpdateRequest ||
ADMIN_PATHS.contains(req.request.getPath());
+req.request.getRequestType() == SolrRequestType.UPDATE
Review Comment:
🎉
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/DelegationTokenRequest.java:
##
@@ -84,8 +84,8 @@ public DelegationTokenResponse.Get createResponse(SolrClient
client) {
}
@Override
-public String getRequestType() {
- return SolrRequestType.ADMIN.toString();
+protected SolrRequestType getBaseRequestType() {
Review Comment:
[Q] Unrelated to this PR, but does DelegationTokenRequest serve a purpose
anymore? SOLR-9200 added it originally in order to support 'hadoop-auth', and
now that the "hadoop-auth" module is gone this is only referenced in tests
afaict.
Perhaps it could've been removed when @epugh removed hadoop-auth earlier
this year?
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +188,31 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * Defines the intended type of this Solr request.
+ *
+ * Subclasses should typically override this method instead of {@link
+ * SolrRequest#getRequestType}. Note that changing request type can
break/impact request routing
+ * within various clients (i.e. {@link CloudSolrClient}).
+ */
+ protected SolrRequestType getBaseRequestType() {
+return SolrRequestType.UNSPECIFIED;
+ }
+
+ /**
+ * Pattern matches on the underlying {@link SolrRequest} to identify ADMIN
requests and other
+ * special cases. If no special case is identified, {@link
SolrRequest#getBaseRequestType()} is
+ * returned.
+ */
+ public SolrRequestType getRequestType() {
+if (CommonParams.ADMIN_PATHS.contains(getPath())) {
+ return SolrRequestType.ADMIN;
+} else if (this instanceof IsUpdateRequest) {
Review Comment:
+1 to Luke's comment. We could probably avoid the instanceof check (and
relying on `IsUpdateRequest` entirely) if this method was non-final and we gave
`UpdateRequest` its own implementation:
```
public class UpdateRequest {
public SolrRequestType getRequestType() {
return SolrRequestType.UPDATE;
}
}
```
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java:
##
@@ -752,7 +752,7 @@ private Header[] buildRequestSpecificHeaders(final
SolrRequest request) {
new BasicHeader(CommonParams.SOLR_REQUEST_CONTEXT_PARAM,
getContext().toString());
contextHeaders[1] =
-new BasicHeader(CommonParams.SOLR_REQUEST_TYPE_PARAM,
request.getRequestType());
+new BasicHeader(CommonParams.SOLR_REQUEST_TYPE_PARAM,
request.getRequestType().toString());
Review Comment:
Oh wow, I never noticed we set the "request type" as a header - neat!
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/SolrPing.java:
##
@@ -53,8 +53,9 @@ public ModifiableSolrParams getParams() {
}
@Override
- public String getRequestType() {
-return SolrRequestType.ADMIN.toString();
+ /* This request is not processed as an ADMIN request. */
+ protected SolrRequestType getBaseRequestType() {
+return SolrRequestType.UNSPECIFIED;
Review Comment:
+1 to avoid any "reclassifying" in this PR.
LukeRequest is in a similar boat IMO (in terms of deserving
reclassification), and there's probably a few others so best to make it a
separate PR/effort.
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +188,31 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * Defines the intended type of this Solr request.
+ *
+ * Subclasses should typically override this method instead of {@link
+ * SolrRequest#getRequestType}. Note that changing request type can
break/impact request routing
+ * within various clients (i.e. {@link CloudSolrClient}).
+ */
+ protected SolrRequestType getBaseRequestType() {
Review Comment:
Yeah, it does seem to complicate the interface a bit.
I'd be fine with the "field" approach that Luke suggested. Or alternately,
could this be solved without even a "field" if `getRequestType` below was
non-final and subclasses could implement it as n
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
jkmuriithi commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1987459862
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +188,31 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * Defines the intended type of this Solr request.
+ *
+ * Subclasses should typically override this method instead of {@link
+ * SolrRequest#getRequestType}. Note that changing request type can
break/impact request routing
+ * within various clients (i.e. {@link CloudSolrClient}).
+ */
+ protected SolrRequestType getBaseRequestType() {
+return SolrRequestType.UNSPECIFIED;
+ }
+
+ /**
+ * Pattern matches on the underlying {@link SolrRequest} to identify ADMIN
requests and other
+ * special cases. If no special case is identified, {@link
SolrRequest#getBaseRequestType()} is
+ * returned.
+ */
+ public SolrRequestType getRequestType() {
Review Comment:
Good suggestion!
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
jkmuriithi commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1987458954
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +188,31 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * Defines the intended type of this Solr request.
+ *
+ * Subclasses should typically override this method instead of {@link
+ * SolrRequest#getRequestType}. Note that changing request type can
break/impact request routing
+ * within various clients (i.e. {@link CloudSolrClient}).
+ */
+ protected SolrRequestType getBaseRequestType() {
+return SolrRequestType.UNSPECIFIED;
+ }
+
+ /**
+ * Pattern matches on the underlying {@link SolrRequest} to identify ADMIN
requests and other
+ * special cases. If no special case is identified, {@link
SolrRequest#getBaseRequestType()} is
+ * returned.
+ */
+ public SolrRequestType getRequestType() {
+if (CommonParams.ADMIN_PATHS.contains(getPath())) {
+ return SolrRequestType.ADMIN;
+} else if (this instanceof IsUpdateRequest) {
Review Comment:
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.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
kotman12 commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1985801827
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/SolrPing.java:
##
@@ -53,8 +53,9 @@ public ModifiableSolrParams getParams() {
}
@Override
- public String getRequestType() {
-return SolrRequestType.ADMIN.toString();
+ /* This request is not processed as an ADMIN request. */
+ protected SolrRequestType getBaseRequestType() {
+return SolrRequestType.UNSPECIFIED;
Review Comment:
Sure, this comment was a gut reaction to explicitly 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
specifying as UNSPECIFIED at such a low level (I think) because the
default/two-arg super constructor would set it. Idk
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
kotman12 commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1985801827
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/SolrPing.java:
##
@@ -53,8 +53,9 @@ public ModifiableSolrParams getParams() {
}
@Override
- public String getRequestType() {
-return SolrRequestType.ADMIN.toString();
+ /* This request is not processed as an ADMIN request. */
+ protected SolrRequestType getBaseRequestType() {
+return SolrRequestType.UNSPECIFIED;
Review Comment:
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
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
kotman12 commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1985801827
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/SolrPing.java:
##
@@ -53,8 +53,9 @@ public ModifiableSolrParams getParams() {
}
@Override
- public String getRequestType() {
-return SolrRequestType.ADMIN.toString();
+ /* This request is not processed as an ADMIN request. */
+ protected SolrRequestType getBaseRequestType() {
+return SolrRequestType.UNSPECIFIED;
Review Comment:
Sure, this comment was a gut reaction to explicitly overridingspecifying 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
specifying as UNSPECIFIED at such a low level (I think) because the
default/two-arg super constructor would set it. Idk
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
dsmiley commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1985660911
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -210,6 +236,16 @@ public boolean requiresCollection() {
return false;
}
+ /**
+ * Indicates if clients should make attempts to route this request to a
shard leader, overriding
+ * typical client routing preferences for requests. Defaults to true.
+ *
+ * @see CloudSolrClient#isUpdatesToLeaders
+ */
+ public boolean shouldSendToLeaders() {
Review Comment:
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).
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##
@@ -787,7 +786,7 @@ protected NamedList requestWithRetryOnStaleState(
if (request instanceof V2Request) {
isCollectionRequestOfV2 = ((V2Request) request).isPerCollectionRequest();
}
-boolean isAdmin = ADMIN_PATHS.contains(request.getPath());
+boolean isAdmin = request.getRequestType() == SolrRequestType.ADMIN;
Review Comment:
awesome
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +188,31 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * Defines the intended type of this Solr request.
+ *
+ * Subclasses should typically override this method instead of {@link
+ * SolrRequest#getRequestType}. Note that changing request type can
break/impact request routing
+ * within various clients (i.e. {@link CloudSolrClient}).
+ */
+ protected SolrRequestType getBaseRequestType() {
Review Comment:
`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.
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/SolrPing.java:
##
@@ -53,8 +53,9 @@ public ModifiableSolrParams getParams() {
}
@Override
- public String getRequestType() {
-return SolrRequestType.ADMIN.toString();
+ /* This request is not processed as an ADMIN request. */
+ protected SolrRequestType getBaseRequestType() {
+return SolrRequestType.UNSPECIFIED;
Review Comment:
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?
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17043: Remove SolrClient path pattern matching [solr]
kotman12 commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1982512886
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +188,31 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * Defines the intended type of this Solr request.
+ *
+ * Subclasses should typically override this method instead of {@link
+ * SolrRequest#getRequestType}. Note that changing request type can
break/impact request routing
+ * within various clients (i.e. {@link CloudSolrClient}).
+ */
+ protected SolrRequestType getBaseRequestType() {
+return SolrRequestType.UNSPECIFIED;
+ }
+
+ /**
+ * Pattern matches on the underlying {@link SolrRequest} to identify ADMIN
requests and other
+ * special cases. If no special case is identified, {@link
SolrRequest#getBaseRequestType()} is
+ * returned.
+ */
+ public SolrRequestType getRequestType() {
+if (CommonParams.ADMIN_PATHS.contains(getPath())) {
+ return SolrRequestType.ADMIN;
+} else if (this instanceof IsUpdateRequest) {
Review Comment:
If we are marking as deprecated then maybe it makes sense to stop adding
code that relies on this?
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +188,31 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * Defines the intended type of this Solr request.
+ *
+ * Subclasses should typically override this method instead of {@link
+ * SolrRequest#getRequestType}. Note that changing request type can
break/impact request routing
+ * within various clients (i.e. {@link CloudSolrClient}).
+ */
+ protected SolrRequestType getBaseRequestType() {
+return SolrRequestType.UNSPECIFIED;
+ }
+
+ /**
+ * Pattern matches on the underlying {@link SolrRequest} to identify ADMIN
requests and other
+ * special cases. If no special case is identified, {@link
SolrRequest#getBaseRequestType()} is
+ * returned.
+ */
+ public SolrRequestType getRequestType() {
Review Comment:
Should this be `final` if you only expect `getBaseRequestType` to be
overridden?
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +188,31 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * Defines the intended type of this Solr request.
+ *
+ * Subclasses should typically override this method instead of {@link
+ * SolrRequest#getRequestType}. Note that changing request type can
break/impact request routing
+ * within various clients (i.e. {@link CloudSolrClient}).
+ */
+ protected SolrRequestType getBaseRequestType() {
Review Comment:
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.
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/SolrPing.java:
##
@@ -53,8 +53,9 @@ public ModifiableSolrParams getParams() {
}
@Override
- public String getRequestType() {
-return SolrRequestType.ADMIN.toString();
+ /* This request is not processed as an ADMIN request. */
+ protected SolrRequestType getBaseRequestType() {
+return SolrRequestType.UNSPECIFIED;
Review Comment:
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`?
--
This is an
