SOLR-18046 RateLimitFilter, rename EssentialSolrRequestFilter #4065
Conversation
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
Thanks for working on this!
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java
Show resolved
Hide resolved
| public abstract class CoreContainerAwareHttpFilter extends HttpFilter { | ||
| private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); | ||
|
|
||
| private CoreContainerProvider containerProvider; |
There was a problem hiding this comment.
accessed in org.apache.solr.embedded.JettySolrRunner#getCoreContainer and some tests, where I am not confident we can count on a same-thread or synchronized memory model.
There was a problem hiding this comment.
I'm fairly confident we can depend on a "happens-before" relationship with respect to init() occurring before requests are being processed. Are there any particular tests / patterns that concern you?
I don't like making something volatile because there's a slight shred of doubt... and then not documenting that shred of doubt. New eyes will see the code and draw a bad lesson -- OMG we can't trust that init() finishes first?! The sky is falling!
There was a problem hiding this comment.
- I'm not at all worried about init() in a full running jetty webapp.
- I'm also not worried about order of operations in the tests.
- I am worried that our test infrastructure may not have perfectly ensured "happens before" and making this volatile is a simple way of ensuring that guarantee.
- I strongly suspect the folks who wrote jetty have thought carefully about "happens before" with respect to various phases of the servlet lifecycle, so I view this as a test-only accommodation.
I believe I did this after seeing null in a test run. The explanation seemed to be that a thread in the test wasn't seeing the change to this field (presumably because it hadn't been forced to sync by a coordinated lock on a monitor). I wrote all this code just before we stopped for a community discuss thread, so the exact detail is now a bit foggy to me. I will remove and run the tests a few of times... maybe I'll catch it in the act again.
2 participants
https://issues.apache.org/jira/browse/SOLR-18046
In addition to Rate limiting, Renamed EssentialSolrRequestFilter as per subsequent discussion after SOLR-18044 was committed.