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.

Sign up for GitHub

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

Jump to bottom

Use jdk11 primitives to allow backport to branch_9x #13311

Merged
merged 1 commit into from
Apr 17, 2024
Merged

Use jdk11 primitives to allow backport to branch_9x #13311

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lucene/core/src/test/org/apache/lucene/search/BaseKnnVectorQueryTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -781,7 +781,7 @@ public void testTimeLimitingKnnCollectorManager() throws IOException {
TimeLimitingKnnCollectorManager noTimeoutManager =
new TimeLimitingKnnCollectorManager(delegate, null);
KnnCollector noTimeoutCollector =
noTimeoutManager.newCollector(Integer.MAX_VALUE, searcher.leafContexts.getFirst());
noTimeoutManager.newCollector(Integer.MAX_VALUE, searcher.leafContexts.get(0));
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't really make sense to me :/. Main requires JDK21 and using all the nice things 21 provides is, well, nice.

Its pretty common for backports to have one or more weird issues due to compilation levels. The biggest one I have seen is if (klass instanceof ClassFoo classFoo) or the new switch statement syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is our preferred approach for handling such compilation issues during backport? I was worried about recurring conflicts if we just change it in branch_9x, is that not the case? Skipping backport for something like this didn't seem right either. I'd like to learn what we consider as good practice for Lucene codebase.

Copy link
Member

Choose a reason for hiding this comment

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

I was worried about recurring conflicts if we just change it in branch_9x, is that not the case?

I wouldn't expect the conflicts to be to horrible. The biggest ones I have ran into is the new switch statement usages.

Skipping backport for something like this didn't seem right either.

No, backporting the test fix is good. Allows them to be unmuted & ensures we are actually testing these paths.

I'd like to learn what we consider as good practice for Lucene codebase.

There is no prevalent pattern. Its a "gut" thing, and I would rather there not be any mandate.

Its fine that you did this, just wanted to indicate that in this particular case, seemed like it wasn't necessary.


// Check that a normal collector is created without timeout
assertTrue(noTimeoutCollector instanceof TopKnnCollector);
Expand All @@ -797,7 +797,7 @@ public void testTimeLimitingKnnCollectorManager() throws IOException {
TimeLimitingKnnCollectorManager timeoutManager =
new TimeLimitingKnnCollectorManager(delegate, () -> true);
KnnCollector timeoutCollector =
timeoutManager.newCollector(Integer.MAX_VALUE, searcher.leafContexts.getFirst());
timeoutManager.newCollector(Integer.MAX_VALUE, searcher.leafContexts.get(0));

// Check that a time limiting collector is created, which returns partial results
assertFalse(timeoutCollector instanceof TopKnnCollector);
Expand Down