[GitHub] [solr] magibney commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0

["magibney (via GitHub)" <gi...@apache.org>]

magibney commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1433033954

   The map is explicitly designed to lazily cache these `Terms` instances (accumulating them over time for a given `SolrIndexSearcher`). The returned `Terms` instances are _not_ safe for concurrent use -- considering which, I'm surprised we didn't see actual errors resulting from this usage!
   
   I suspect the correct solution will be to just skip caching the `Terms` instances altogether. We could also upgrade from Map to use a proper cache (with evictions, etc.) or some other caching strategy that effectively incorporates "creation thread" into the cache key. Not sure whether that would be worth it.
   
   It's also possible that the _way_ in which `Terms` instances returned from `SlowCompositeReaderWrapper` are used in practice actually _is_ thread-safe? In that case there might be other options, such as pulling the required thread-safe derivative of the `Terms` instance, and wrapping it in a synthetic `Terms` instance that mostly throws `UnsupportedOperationException`, and caching/returning that synthetic instance?
   
   Curious what others think here ... perhaps @dsmiley?


-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org