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<SolrPingResponse> {
/** 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<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:
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
