dsmiley commented on code in PR #1780:
URL: https://github.com/apache/solr/pull/1780#discussion_r1265907722
##########
solr/core/src/java/org/apache/solr/servlet/ServletUtils.java:
##########
@@ -216,16 +217,19 @@ static void rateLimitRequest(
}
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;
}
+ needsCleanup = false;
// todo: this shouldn't be required, tracing and rate limiting should be
independently
// composable
traceHttpRequestExecution2(request, response, limitedExecution, trace);
} finally {
+ if (needsCleanup) {
+ consumeInputFully(request, response);
+ SolrRequestInfo.reset();
+ SolrRequestParsers.cleanupMultipartFiles(request);
Review Comment:
This looks wrong.... we won't reset SolrRequestInfo sometimes? Why not?
Not doing so is dangerous; messing up access to SolrRequestInfo for future
requests on the same thread.
And structurally this doesn't look right. We shouldn't be adding additional
callers to some of these methods. Control flow should be well balanced logical
and avoid calling methods from multiple spots under different conditions. It's
not that it's wrong necessarily but is hard to understand and reason about.
##########
solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java:
##########
@@ -209,8 +208,8 @@ public void doFilter(
Tracer t = getCores() == null ? GlobalTracer.get() :
getCores().getTracer();
request.setAttribute(Tracer.class.getName(), t);
RateLimitManager rateLimitManager =
coreService.getService().getRateLimitManager();
- request.setAttribute(RateLimitManager.class.getName(), rateLimitManager);
Review Comment:
It's not clear to me if the RateLimitManager was on the request merely out
of convenience for the existing logic or if it was intentionally used for
miscellaneous unforeseen use-cases (and you are now removing it). Shrug.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
