[ https://issues.apache.org/jira/browse/LUCENE-7410?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15856733#comment-15856733 ]
Michael McCandless commented on LUCENE-7410: -------------------------------------------- +1, this is a great infrastructure API improvement, thanks [~jpountz]. I only saw some minor issues: Can/should we {{ensureOpen}} in the {{getReaderCacheHelper}} and {{addClosedListener}} impls? Just seems like it'd be some good extra safety to catch accidental mis-use? There is a typo (like -> live) here: {noformat} + // TODO: this is trappy as the expectation is that core keys like for a long {noformat} And a missing d in StandarDirectoryReader in the exception message here: {noformat} + if (cacheHelper == null) { + throw new IllegalStateException("StandarDirectoryReader must support caching"); + } {noformat} And extra s here: {noformat} + * valuess updates may be considered equal. Consider using {noformat} > Make cache keys and closed listeners less trappy > ------------------------------------------------ > > Key: LUCENE-7410 > URL: https://issues.apache.org/jira/browse/LUCENE-7410 > Project: Lucene - Core > Issue Type: Bug > Reporter: Adrien Grand > Attachments: LUCENE-7410.patch, LUCENE-7410.patch, LUCENE-7410.patch > > > IndexReader currently exposes getCoreCacheKey(), > getCombinedCoreAndDeletesKey(), addCoreClosedListener() and > addReaderClosedListener(). They are typically used to manage resources whose > lifetime needs to mimic the lifetime of segments/indexes, typically caches. > I think this is trappy for various reasons: > h3. Memory leaks > When maintaining a cache, entries are added to the cache based on the cache > key and then evicted using the cache key that is given back by the close > listener, so it is very important that both keys are the same. > But if a filter reader happens to delegate get*Key() and not > add*ClosedListener() or vice-versa then there is potential for a memory leak > since the closed listener will be called on a different key and entries will > never be evicted from the cache. > h3. Lifetime expectations > The expectation of using the core cache key is that it will not change in > case of deletions, but this is only true on SegmentReader and LeafReader > impls that delegate to it. Other implementations such as composite readers or > parallel leaf readers use the same key for "core" and "combined core and > deletes". > h3. Throw-away wrappers cause cache trashing > An application might want to either expose more (with a ParrallelReader or > MultiReader) or less information (by filtering fields/docs that can be seen) > depending on the user who is logged in. In that case the application would > typically maintain a DirectoryReader and then wrap it per request depending > on the logged user and throw away the wrapper once the request is completed. > The problem is that these wrappers have their own cache keys and the > application may build something costly and put it in a cache to throw it away > a couple milliseconds later. I would rather like for such readers to have a > way to opt out from caching on order to avoid this performance trap. > h3. Type safety > The keys that are exposed are plain java.lang.Object instances, which > requires caches to look like a {{Map<Object, ?>}} which makes it very easy to > either try to get, put or remove on the wrong object since any object would > be accepted. -- This message was sent by Atlassian JIRA (v6.3.15#6346) --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org