You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "janhoy (via GitHub)" <gi...@apache.org> on 2023/09/15 22:51:07 UTC
[GitHub] [solr] janhoy opened a new pull request, #1930: SOLR-16982: Trip a Circuit Breaker only for external requests
janhoy opened a new pull request, #1930:
URL: https://github.com/apache/solr/pull/1930
https://issues.apache.org/jira/browse/SOLR-16982
There may be more smart/robust ways of telling whether a request is "internal" or sharded, but this works :)
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1930: SOLR-16982: Trip a Circuit Breaker only for external requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on code in PR #1930:
URL: https://github.com/apache/solr/pull/1930#discussion_r1329781429
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -343,4 +343,16 @@ public Collection<Api> getApis() {
return Collections.singleton(
new ApiBag.ReqHandlerToApi(this, ApiBag.constructSpec(pluginInfo)));
}
+
+ /**
+ * Checks whether the given request is an internal request to a shard. We rely on the fact that an
+ * internal search request to a shard contains the param "isShard", and an internal update request
+ * to a shard contains the param "distrib.from".
+ *
+ * @return true if request is internal
+ */
+ public static boolean isInternalShardRequest(SolrQueryRequest req) {
Review Comment:
> Perhaps this could even be protected and non-static
We reference this method from unit tests on the SolrQueryRequest, without an instance of a request hander, so this is a bit hard. It would of course be possible to create another instance method that delegates to this static one, but that can also be done later when need arises.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy merged pull request #1930: SOLR-16982: Trip a Circuit Breaker only for external requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy merged PR #1930:
URL: https://github.com/apache/solr/pull/1930
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1930: SOLR-16982: Trip a Circuit Breaker only for external requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #1930:
URL: https://github.com/apache/solr/pull/1930#issuecomment-1728535462
Plan to merge this on Friday..
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a diff in pull request #1930: SOLR-16982: Trip a Circuit Breaker only for external requests
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #1930:
URL: https://github.com/apache/solr/pull/1930#discussion_r1329049406
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -343,4 +343,16 @@ public Collection<Api> getApis() {
return Collections.singleton(
new ApiBag.ReqHandlerToApi(this, ApiBag.constructSpec(pluginInfo)));
}
+
+ /**
+ * Checks whether the given request is an internal request to a shard. We rely on the fact that an
+ * internal search request to a shard contains the param "isShard", and an internal update request
+ * to a shard contains the param "distrib.from".
+ *
+ * @return true if request is internal
+ */
+ public static boolean isInternalShardRequest(SolrQueryRequest req) {
+ return req.getParams().get("distrib.from") != null
+ || "true".equals(req.getParams().get("isShard"));
Review Comment:
```suggestion
|| "true".equals(req.getParams().get(ShardParams.IS_SHARD));
```
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -343,4 +343,16 @@ public Collection<Api> getApis() {
return Collections.singleton(
new ApiBag.ReqHandlerToApi(this, ApiBag.constructSpec(pluginInfo)));
}
+
+ /**
+ * Checks whether the given request is an internal request to a shard. We rely on the fact that an
+ * internal search request to a shard contains the param "isShard", and an internal update request
+ * to a shard contains the param "distrib.from".
+ *
+ * @return true if request is internal
+ */
+ public static boolean isInternalShardRequest(SolrQueryRequest req) {
+ return req.getParams().get("distrib.from") != null
Review Comment:
```suggestion
return req.getParams().get(DistributedUpdateProcessor.DISTRIB_FROM) != null
```
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -343,4 +343,16 @@ public Collection<Api> getApis() {
return Collections.singleton(
new ApiBag.ReqHandlerToApi(this, ApiBag.constructSpec(pluginInfo)));
}
+
+ /**
+ * Checks whether the given request is an internal request to a shard. We rely on the fact that an
+ * internal search request to a shard contains the param "isShard", and an internal update request
+ * to a shard contains the param "distrib.from".
+ *
+ * @return true if request is internal
+ */
+ public static boolean isInternalShardRequest(SolrQueryRequest req) {
Review Comment:
Perhaps this could even be protected and non-static i.e. by overriding handlers could also treat other requests as-if they are internal and so exempt them from circuit breaker logic? Hmm, though then `isInternalShardRequest` could be confusing as a name.
```suggestion
protected boolean isInternalShardRequest(SolrQueryRequest req) {
```
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1930: SOLR-16982: Trip a Circuit Breaker only for external requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on code in PR #1930:
URL: https://github.com/apache/solr/pull/1930#discussion_r1333975664
##########
solr/CHANGES.txt:
##########
@@ -109,6 +109,8 @@ Improvements
* SOLR-15474: Make Circuit breakers individually pluggable (Atri Sharma, Christine Poerschke, janhoy)
+* SOLR-16982: Trip Circuit Breakers only for external requests (janhoy)
Review Comment:
```suggestion
* SOLR-16982: Trip Circuit Breakers only for external requests (janhoy, Christine Poerschke)
```
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1930: SOLR-16982: Trip a Circuit Breaker only for external requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on code in PR #1930:
URL: https://github.com/apache/solr/pull/1930#discussion_r1329355144
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -343,4 +343,16 @@ public Collection<Api> getApis() {
return Collections.singleton(
new ApiBag.ReqHandlerToApi(this, ApiBag.constructSpec(pluginInfo)));
}
+
+ /**
+ * Checks whether the given request is an internal request to a shard. We rely on the fact that an
+ * internal search request to a shard contains the param "isShard", and an internal update request
+ * to a shard contains the param "distrib.from".
+ *
+ * @return true if request is internal
+ */
+ public static boolean isInternalShardRequest(SolrQueryRequest req) {
Review Comment:
We have something called Rate Limiters in Solr too, and they rely on [some other mechanism](https://github.com/apache/solr/blob/main/solr/core/src/java/org/apache/solr/servlet/RateLimitManager.java#L82-L86) for determining internal vs external request, but I found no obvious way to re-use that here. So this param checking is a very naiive approach, but perhaps robust enough?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org