You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@solr.apache.org by ds...@apache.org on 2023/07/19 17:07:30 UTC
[solr] branch main updated: SOLR-16889: Rate Limiter should stop processing on HTTP 429 (#1780)
This is an automated email from the ASF dual-hosted git repository.
dsmiley pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/main by this push:
new ee9e37252d7 SOLR-16889: Rate Limiter should stop processing on HTTP 429 (#1780)
ee9e37252d7 is described below
commit ee9e37252d71082802a3211fc990701e388aad44
Author: Alex <st...@apache.org>
AuthorDate: Wed Jul 19 10:07:25 2023 -0700
SOLR-16889: Rate Limiter should stop processing on HTTP 429 (#1780)
And restructured some nearby code for readability / organization
---
solr/CHANGES.txt | 2 +
.../org/apache/solr/servlet/RateLimitManager.java | 2 +
.../java/org/apache/solr/servlet/ServletUtils.java | 104 ++++++++-------------
.../apache/solr/servlet/SolrDispatchFilter.java | 33 ++++---
.../solr/servlet/TestRequestRateLimiter.java | 13 ++-
5 files changed, 70 insertions(+), 84 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 79163fdd61e..0e95dd35fe0 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -73,6 +73,8 @@ Bug Fixes
* SOLR-16886: Don't commit multi-part uploads that have been aborted (Tomás Fernández Löbbe, Houston Putman)
+* SOLR-16889: Rate Limiter should stop processing on 429 (Alex Deparvu, David Smiley)
+
Dependency Upgrades
---------------------
(No changes)
diff --git a/solr/core/src/java/org/apache/solr/servlet/RateLimitManager.java b/solr/core/src/java/org/apache/solr/servlet/RateLimitManager.java
index 8531ec3318d..baef6e8501a 100644
--- a/solr/core/src/java/org/apache/solr/servlet/RateLimitManager.java
+++ b/solr/core/src/java/org/apache/solr/servlet/RateLimitManager.java
@@ -47,6 +47,8 @@ import org.slf4j.LoggerFactory;
public class RateLimitManager implements ClusterPropertiesListener {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ public static final String ERROR_MESSAGE =
+ "Too many requests for this request type. Please try after some time or increase the quota for this request type";
public static final int DEFAULT_CONCURRENT_REQUESTS =
(Runtime.getRuntime().availableProcessors()) * 3;
public static final long DEFAULT_SLOT_ACQUISITION_TIMEOUT_MS = -1;
diff --git a/solr/core/src/java/org/apache/solr/servlet/ServletUtils.java b/solr/core/src/java/org/apache/solr/servlet/ServletUtils.java
index d10d97451ec..a286078dafb 100644
--- a/solr/core/src/java/org/apache/solr/servlet/ServletUtils.java
+++ b/solr/core/src/java/org/apache/solr/servlet/ServletUtils.java
@@ -45,7 +45,6 @@ import org.apache.http.HttpHeaders;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.SolrException.ErrorCode;
import org.apache.solr.logging.MDCLoggingContext;
-import org.apache.solr.request.SolrRequestInfo;
import org.apache.solr.util.tracing.HttpServletCarrier;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -197,34 +196,26 @@ public abstract class ServletUtils {
* @param request The request to limit
* @param response The associated response
* @param limitedExecution code that will be traced
- * @param trace a boolean that turns tracing on or off
*/
static void rateLimitRequest(
+ RateLimitManager rateLimitManager,
HttpServletRequest request,
HttpServletResponse response,
- Runnable limitedExecution,
- boolean trace)
+ Runnable limitedExecution)
throws ServletException, IOException {
boolean accepted = false;
- RateLimitManager rateLimitManager = getRateLimitManager(request);
try {
- try {
- accepted = rateLimitManager.handleRequest(request);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- throw new SolrException(ErrorCode.SERVER_ERROR, e.getMessage());
- }
-
+ accepted = rateLimitManager.handleRequest(request);
if (!accepted) {
- String errorMessage =
- "Too many requests for this request type."
- + "Please try after some time or increase the quota for this request type";
-
- response.sendError(429, errorMessage);
+ response.sendError(ErrorCode.TOO_MANY_REQUESTS.code, RateLimitManager.ERROR_MESSAGE);
+ return;
}
// todo: this shouldn't be required, tracing and rate limiting should be independently
// composable
- traceHttpRequestExecution2(request, response, limitedExecution, trace);
+ traceHttpRequestExecution2(request, response, limitedExecution);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new SolrException(ErrorCode.SERVER_ERROR, e.getMessage());
} finally {
if (accepted) {
rateLimitManager.decrementActiveRequests(request);
@@ -236,59 +227,42 @@ public abstract class ServletUtils {
* Sets up tracing for an HTTP request. Perhaps should be converted to a servlet filter at some
* point.
*
+ * @param request The request to limit
+ * @param response The associated response
* @param tracedExecution the executed code
*/
private static void traceHttpRequestExecution2(
- HttpServletRequest request,
- HttpServletResponse response,
- Runnable tracedExecution,
- boolean required)
+ HttpServletRequest request, HttpServletResponse response, Runnable tracedExecution)
throws ServletException, IOException {
Tracer tracer = getTracer(request);
- if (tracer != null) {
- Span span = buildSpan(tracer, request);
-
- request.setAttribute(Span.class.getName(), span);
- try (var scope = tracer.scopeManager().activate(span)) {
-
- assert scope != null; // prevent javac warning about scope being unused
- MDCLoggingContext.setTracerId(span.context().toTraceId()); // handles empty string
- try {
- tracedExecution.run();
- } catch (ExceptionWhileTracing e) {
- if (e.e instanceof SolrAuthenticationException) {
- throw (SolrAuthenticationException) e.e;
- }
- if (e.e instanceof ServletException) {
- throw (ServletException) e.e;
- }
- if (e.e instanceof IOException) {
- throw (IOException) e.e;
- }
- if (e.e instanceof RuntimeException) {
- throw (RuntimeException) e.e;
- } else {
- throw new RuntimeException(e.e);
- }
- }
- } catch (SolrAuthenticationException e) {
- // done, the response and status code have already been sent
- } finally {
- consumeInputFully(request, response);
- SolrRequestInfo.reset();
- SolrRequestParsers.cleanupMultipartFiles(request);
+ Span span = buildSpan(tracer, request);
+
+ request.setAttribute(SolrDispatchFilter.ATTR_TRACING_SPAN, span);
+ try (var scope = tracer.scopeManager().activate(span)) {
- span.setTag(Tags.HTTP_STATUS, response.getStatus());
- span.finish();
+ assert scope != null; // prevent javac warning about scope being unused
+ MDCLoggingContext.setTracerId(span.context().toTraceId()); // handles empty string
+
+ tracedExecution.run();
+ } catch (ExceptionWhileTracing e) {
+ if (e.e instanceof SolrAuthenticationException) {
+ // done, the response and status code have already been sent
+ return;
}
- } else {
- if (required) {
- throw new IllegalStateException(
- "Tracing required, but could not find Tracer in request attribute:"
- + SolrDispatchFilter.ATTR_TRACING_TRACER);
+ if (e.e instanceof ServletException) {
+ throw (ServletException) e.e;
+ }
+ if (e.e instanceof IOException) {
+ throw (IOException) e.e;
+ }
+ if (e.e instanceof RuntimeException) {
+ throw (RuntimeException) e.e;
} else {
- tracedExecution.run();
+ throw new RuntimeException(e.e);
}
+ } finally {
+ span.setTag(Tags.HTTP_STATUS, response.getStatus());
+ span.finish();
}
}
@@ -296,10 +270,6 @@ public abstract class ServletUtils {
return (Tracer) req.getAttribute(SolrDispatchFilter.ATTR_TRACING_TRACER);
}
- private static RateLimitManager getRateLimitManager(HttpServletRequest req) {
- return (RateLimitManager) req.getAttribute(SolrDispatchFilter.ATTR_RATELIMIT_MANAGER);
- }
-
protected static Span buildSpan(Tracer tracer, HttpServletRequest request) {
if (tracer instanceof NoopTracer) {
return NoopSpan.INSTANCE;
@@ -321,7 +291,7 @@ public abstract class ServletUtils {
// we make sure we read the full client request so that the client does
// not hit a connection reset and we can reuse the
// connection - see SOLR-8453 and SOLR-8683
- private static void consumeInputFully(HttpServletRequest req, HttpServletResponse response) {
+ static void consumeInputFully(HttpServletRequest req, HttpServletResponse response) {
try {
ServletInputStream is = req.getInputStream();
//noinspection StatementWithEmptyBody
diff --git a/solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java b/solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
index 44c02cce109..cfd9836c4ed 100644
--- a/solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
+++ b/solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
@@ -52,6 +52,7 @@ import org.apache.solr.core.NodeRoles;
import org.apache.solr.handler.api.V2ApiUtils;
import org.apache.solr.logging.MDCLoggingContext;
import org.apache.solr.logging.MDCSnapshot;
+import org.apache.solr.request.SolrRequestInfo;
import org.apache.solr.security.AuditEvent;
import org.apache.solr.security.AuthenticationPlugin;
import org.apache.solr.security.PKIAuthenticationPlugin;
@@ -74,7 +75,6 @@ public class SolrDispatchFilter extends BaseSolrFilter implements PathExcluder {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static final String ATTR_TRACING_SPAN = Span.class.getName();
public static final String ATTR_TRACING_TRACER = Tracer.class.getName();
- public static final String ATTR_RATELIMIT_MANAGER = RateLimitManager.class.getName();
// TODO: see if we can get rid of the holder here (Servlet spec actually guarantees
// ContextListeners run before filter init, but JettySolrRunner that we use for tests is
@@ -207,20 +207,25 @@ public class SolrDispatchFilter extends BaseSolrFilter implements PathExcluder {
return;
}
Tracer t = getCores() == null ? GlobalTracer.get() : getCores().getTracer();
- request.setAttribute(Tracer.class.getName(), t);
+ request.setAttribute(ATTR_TRACING_TRACER, t);
RateLimitManager rateLimitManager = coreService.getService().getRateLimitManager();
- request.setAttribute(RateLimitManager.class.getName(), rateLimitManager);
- ServletUtils.rateLimitRequest(
- request,
- response,
- () -> {
- try {
- dispatch(chain, request, response, retry);
- } catch (IOException | ServletException | SolrAuthenticationException e) {
- throw new ExceptionWhileTracing(e);
- }
- },
- true);
+ try {
+ ServletUtils.rateLimitRequest(
+ rateLimitManager,
+ request,
+ response,
+ () -> {
+ try {
+ dispatch(chain, request, response, retry);
+ } catch (IOException | ServletException | SolrAuthenticationException e) {
+ throw new ExceptionWhileTracing(e);
+ }
+ });
+ } finally {
+ ServletUtils.consumeInputFully(request, response);
+ SolrRequestInfo.reset();
+ SolrRequestParsers.cleanupMultipartFiles(request);
+ }
}
private static Span getSpan(HttpServletRequest req) {
diff --git a/solr/core/src/test/org/apache/solr/servlet/TestRequestRateLimiter.java b/solr/core/src/test/org/apache/solr/servlet/TestRequestRateLimiter.java
index c9b62b2b3c5..a88c9f3f296 100644
--- a/solr/core/src/test/org/apache/solr/servlet/TestRequestRateLimiter.java
+++ b/solr/core/src/test/org/apache/solr/servlet/TestRequestRateLimiter.java
@@ -19,20 +19,24 @@ package org.apache.solr.servlet;
import static org.apache.solr.servlet.RateLimitManager.DEFAULT_SLOT_ACQUISITION_TIMEOUT_MS;
import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.instanceOf;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
import java.util.concurrent.atomic.AtomicInteger;
import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.SolrQuery;
import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.impl.BaseHttpSolrClient.RemoteSolrException;
import org.apache.solr.client.solrj.impl.CloudSolrClient;
import org.apache.solr.client.solrj.request.CollectionAdminRequest;
import org.apache.solr.client.solrj.response.QueryResponse;
import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.SolrException;
import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.common.cloud.SolrZkClient;
import org.apache.solr.common.util.ExecutorUtil;
@@ -176,7 +180,7 @@ public class TestRequestRateLimiter extends SolrCloudTestCase {
assertEquals(numDocuments, response.getResults().getNumFound());
} catch (Exception e) {
- throw new RuntimeException(e.getMessage());
+ throw new RuntimeException(e.getMessage(), e);
}
return true;
@@ -188,9 +192,12 @@ public class TestRequestRateLimiter extends SolrCloudTestCase {
for (Future<?> future : futures) {
try {
assertNotNull(future.get());
- } catch (Exception e) {
+ } catch (ExecutionException e) {
+ MatcherAssert.assertThat(e.getCause().getCause(), instanceOf(RemoteSolrException.class));
+ RemoteSolrException rse = (RemoteSolrException) e.getCause().getCause();
+ assertEquals(SolrException.ErrorCode.TOO_MANY_REQUESTS.code, rse.code());
MatcherAssert.assertThat(
- e.getMessage(), containsString("non ok status: 429, message:Too Many Requests"));
+ rse.getMessage(), containsString("non ok status: 429, message:Too Many Requests"));
}
}
} finally {