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

Fix split package LazySoftDeletesDirectoryReaderWrapper #81981

Closed
ChrisHegarty opened this issue Dec 21, 2021 · 3 comments · Fixed by #82132
Closed

Fix split package LazySoftDeletesDirectoryReaderWrapper #81981

ChrisHegarty opened this issue Dec 21, 2021 · 3 comments · Fixed by #82132
Assignees
Labels
>bug :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. :Search/Search Search-related issues that do not fall into other categories Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. Team:Search Meta label for search team

Comments

@ChrisHegarty
Copy link
Contributor

ChrisHegarty commented Dec 21, 2021

The last split package issue that we have between server and lucene is LazySoftDeletesDirectoryReaderWrapper. This was added early in 2021, so not one of the legacy hangovers from years gone by.

By inspecting the code, the three package-private member dependencies in org.apache.lucene.index become clear:

  1. access to field SegmentReader.isNRT,
  2. CacheKey::new, and
  3. PendingSoftDeletes::applySoftDeletes.

First, LazySoftDeletesDirectoryReaderWrapper is only useful for read-only indices that are lazily loaded (a.k.a. frozen tier), so it is doubtful that Lucene would find it generally useful.

A note on each of the specific package-private API points:

  1. Access to field SegmentReader.isNRT is only for assertion code, and exposing an accessor from Lucene for this seems like it could be reasonable.
  2. The CacheKey constructor is deliberately non-accessible from outside the package. There were past issues where problems arose with short-lived reader wrappers that would not reuse caches across queries. Maybe there's a way to make DelegatingCacheHelper only work with a DirectoryReader, since directory readers would generally not be short-lived. Or otherwise exposing DelegatingCacheHelper as a more generally useful API in Lucene itself.
  3. For PendingSoftDeletes::applySoftDeletes one possibility is to just duplicate the code in ES. There are additional package-private issues when this is attempted, but maybe they are more straightforward to resolve.

[ the above notes are a summary of a discussion that included @romseygeek @jpountz, and @ywelsch ]

see #78166

@ChrisHegarty ChrisHegarty added >bug needs:triage Requires assignment of a team area label labels Dec 21, 2021
@romseygeek romseygeek added :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. :Search/Search Search-related issues that do not fall into other categories and removed needs:triage Requires assignment of a team area label labels Dec 21, 2021
@elasticmachine elasticmachine added Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. Team:Search Meta label for search team labels Dec 21, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@ChrisHegarty
Copy link
Contributor Author

Preparatory changes have been merged into Lucene, see PR. ES can leverage this once the Lucene snapshot has been updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. :Search/Search Search-related issues that do not fall into other categories Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. Team:Search Meta label for search team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants