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: 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