gus-asf commented on code in PR #4120:
URL: https://github.com/apache/solr/pull/4120#discussion_r2901386982
##########
solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java:
##########
@@ -125,7 +118,7 @@ public void doFilter(HttpServletRequest request,
HttpServletResponse response, F
throws IOException, ServletException {
// internal version of doFilter that tracks if we are in a retry
- dispatch(chain, closeShield(request), closeShield(response), false);
+ dispatch(chain, request, response, false);
Review Comment:
I guess I don't understand this concern. Do you envision possible filters
between RequiredSolrRequestFilter and this one that would want to call close on
these? What would be an example of that? Conceptually close shielding is a
setup/safety thing entirely unrelated to dispatch. If we put it here than any
code that takes a different path also has to duplicate this logic... I'm hoping
that by breaking out these filters they could be wrapped around multiple
endpoints without code duplication eventually, and if there is one filter that
is most likely to be omitted for other cases it's dispatch filter... so if the
other paths also want close shielding we have to duplicate this wrapping logic
everywhere.
Not going to die on this hill, but having these here seems like a code smell
(of unrelated stuff bundled together) to me.
I do think it needs to happen before SolrRequestInfo gets set for sure.
(which it does either way since I punted any form of setting up SolrRequestInfo
in a filter and its still in HttpSolrCall).
--
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]
