Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LUCENE-10369: Move DelegatingCacheHelper to FilterDirectoryReader #596

Merged
merged 12 commits into from Jan 11, 2022

Conversation

grcevski
Copy link
Contributor

@grcevski grcevski commented Jan 10, 2022

Description

To support building long-lived readers without IndexReader to delegate caching to, it would be beneficial to refactor SoftDeletesDirectoryReaderWrapper, such that its caching behaviour (DelegatingCacheHelper) is moved into the superclass FilterDirectoryReader.

With this change, external implementations of such readers, e.g. LazySoftDeletesDirectoryReaderWrapper in Elasticsearch could properly modularize their codebase and remove split packages.

Solution

The solution proposed here moves the delegating CacheHelper implementation from SoftDeletesDirectoryReaderWrapper to the superclass, so that it can be used by other subclasses.

An alternative approach could possibly be a new public class specialization of the FilterDirectoryReader class, which brings in the DelegatingCacheHelper and enforces the caching behaviour.

Tests

I added the following unit tests for the moved DelegatingCacheHelper class:

  • Tests that ensure the DelegatingCacheHelper is using unique CacheKey
  • Tests that ensure that, when the onClose listener is invoked on the delegate, the correct CacheKey is used to invalidate the cache.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Lucene maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants