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

CandidateMatcher public matching functions #13632

Merged
merged 2 commits into from
Aug 7, 2024

Conversation

bjacobowitz
Copy link
Contributor

Description

In CandidateMatcher, increase visibility of matchQuery() (from protected), finish() and reportError() (from package-private) to public, so that a CandidateMatcher can be used effectively from outside the org.apache.lucene.monitor package.

Problem

The current access protections on CandidateMatcher make it infeasible for a user of the Lucene Monitor library to use an existing CandidateMatcher if they are outside the org.apache.lucene.monitor package (e.g. as part of the implementation of their own CandidateMatcher).

For example, a user working outside of the org.apache.lucene.monitor package could not build their own version of ParallelMatcher by making use of the matcher from QueryMatch.SIMPLE_MATCHER, because they cannot access matchQuery, reportError or finish on it.

Solution

This PR takes the simplest solution of increase the visibility of those functions in CandidateMatcher to public. This also requires modifying some existing CandidateMatcher implementations that override protected matchQuery() to instead override public matchQuery().

I've also added some just-compile tests in a subpackage org.apache.lucene.monitor.otherpackage, which call the newly-public functions to verify that they are accessible from outside the org.apache.lucene.monitor package.

See previous discussion of this approach and alternatives in #13109.

Merge Request

If this PR gets merged, can you please use my [email protected] email address for the squash+merge. Thank you.

Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bjacobowitz, this looks great! Can you also create a CHANGES.txt entry under 'API changes'?


public class TestCandidateMatcherVisibility {

// Dummy empty IndexReader for use in creating a matcher
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a MemoryIndex here would probably be simpler?

@bjacobowitz bjacobowitz requested a review from romseygeek August 7, 2024 13:45
@romseygeek romseygeek merged commit 926d8f4 into apache:main Aug 7, 2024
3 checks passed
@romseygeek
Copy link
Contributor

Thanks @bjacobowitz!

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.

2 participants