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<String> 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.
+ *
+ * <p>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<String> 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.
+ *
+ * <p>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<String> 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.
+ *
+ * <p>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 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
