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<Object> sendRequest(SolrRequest<?>
request, List<String> 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<Object> 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<Object> queryResponse;
+ SolrRequestType requestType = SolrRequestType.QUERY;
Review Comment:
I think these ~9 lines would read clearer as an if-else, with initializing a
request two different ways. Rather than declaring a bunch of vars that are
components to a request.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -1001,25 +999,30 @@ protected NamedList<Object> sendRequest(SolrRequest<?>
request, List<String> inp
boolean sendToLeaders = false;
- if (request instanceof IsUpdateRequest) {
- sendToLeaders = ((IsUpdateRequest) request).isSendToLeaders() &&
this.isUpdatesToLeaders();
+ if (request.getRequestType() == SolrRequestType.UPDATE) {
Review Comment:
this check seems pointless since we're about to see if request is an
instanceof UpdateRequest
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -1001,25 +999,30 @@ protected NamedList<Object> sendRequest(SolrRequest<?>
request, List<String> 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<Object> response = directUpdate((AbstractUpdateRequest)
request, collection);
- if (response != null) {
- return response;
+ var updateRequest = (UpdateRequest) request;
Review Comment:
note in JDK 17 we can move updateRequest to the line above at the end
##########
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:
that's the default; can remove this line
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##########
@@ -138,8 +136,9 @@ public Set<String> getUrlParamNames() {
public CompletableFuture<Rsp> requestAsync(Req req) {
CompletableFuture<Rsp> apiFuture = new CompletableFuture<>();
Rsp rsp = new Rsp();
- boolean isNonRetryable =
- req.request instanceof IsUpdateRequest ||
ADMIN_PATHS.contains(req.request.getPath());
+ boolean isAdmin =
Review Comment:
out of scope but ADMIN-ness doesn't actually relate to retry-ability.
Ideally the request would indicate this, and would have a default
implementation that looks at HTTP GET vs other verbs.
--
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
