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 org.apache.lucene.index.ShuffleForcedMergePolicy #78253

Closed
wants to merge 1 commit into from

Conversation

ChrisHegarty
Copy link
Contributor

To allow for the future modularization of Elasticsearch with Java Modules, there are a number preparatory tasks that need be completed. This is one part of one such task: eliminate split packages.

Since recent Lucene upgrades, ShuffleForcedMergePolicy has but a single package-private dependency on lucene, namely SegmentInfo::setDiagnostics. SetDiagnostics is being used by SFMP to set a single additional ES specific diagnostic key. It is possible to recreate the whole SegmentCommitInfo ( and the SI within, along with the additional diagnostic key ), by using just the accessible parts of the lucene API. But this clearly results in little more object allocation.

If this object allocation is unacceptable, then we can raise this issue with Lucene, with a view to improving the Lucene API to allow for such use case that ES has - to add an additional diagnostic.

relates #78166

@ChrisHegarty ChrisHegarty added v8.0.0 modularization Java Modules related labels Sep 23, 2021
@ChrisHegarty ChrisHegarty self-assigned this Sep 23, 2021
@ChrisHegarty
Copy link
Contributor Author

In terms of the relocated package, org.elasticsearch.index.engine was chosen since the only usage of SFMP is from code within that package.

@ChrisHegarty
Copy link
Contributor Author

As can be seen from maxDocOrDefault in the new code - something about re-creating the SI feels a little uncomfortable!

@jpountz
Copy link
Contributor

jpountz commented Sep 23, 2021

The object allocation is no concern at all here since this method is called infrequently.

However I think we should still raise it with Lucene. The javadocs of setMergeInfo mention explicitly that this may be used to set diagnostics, so it looks like a bug to me that the javadocs reference a method that is not accessible.

@ChrisHegarty
Copy link
Contributor Author

Closing this issue. Fixed by #78605

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modularization Java Modules related v8.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants