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<FieldAn
/** Constructs a new FieldAnalysisRequest with a default uri of
"/fieldanalysis". */
public FieldAnalysisRequest() {
- super(METHOD.GET, "/analysis/field");
+ super(METHOD.GET, "/analysis/field", SolrRequestType.UNSPECIFIED);
Review Comment:
keep as QUERY
##########
solr/test-framework/src/java/org/apache/solr/cloud/ConfigRequest.java:
##########
@@ -36,7 +36,7 @@ public class ConfigRequest extends
CollectionRequiringSolrRequest {
protected final String message;
public ConfigRequest(String message) {
- super(SolrRequest.METHOD.POST, "/config");
+ super(SolrRequest.METHOD.POST, "/config", SolrRequestType.UNSPECIFIED);
Review Comment:
keep ADMIN
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -185,8 +193,21 @@ public void setQueryParams(Set<String> queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * The type of this Solr request.
+ *
+ * <p>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<LukeResponse> {
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<V2Response>
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<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 = 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
