You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "vigyasharma (via GitHub)" <gi...@apache.org> on 2024/04/17 05:47:56 UTC
[PR] Use jdk11 primitives to allow backport to branch_9x [lucene]
vigyasharma opened a new pull request, #13311:
URL: https://github.com/apache/lucene/pull/13311
`BaseKnnVectorQueryTestCase#testTimeLimitingKnnCollectorManager` uses some new java primitives that are breaking on backport to branch_9x, which builds with OpenJdk 11. We don't really need the new APIs, this small change allows us to backport without conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Use jdk11 primitives to allow backport to branch_9x [lucene]
Posted by "vigyasharma (via GitHub)" <gi...@apache.org>.
vigyasharma commented on code in PR #13311:
URL: https://github.com/apache/lucene/pull/13311#discussion_r1569337501
##########
lucene/core/src/test/org/apache/lucene/search/BaseKnnVectorQueryTestCase.java:
##########
@@ -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));
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Use jdk11 primitives to allow backport to branch_9x [lucene]
Posted by "vigyasharma (via GitHub)" <gi...@apache.org>.
vigyasharma merged PR #13311:
URL: https://github.com/apache/lucene/pull/13311
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Use jdk11 primitives to allow backport to branch_9x [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13311:
URL: https://github.com/apache/lucene/pull/13311#discussion_r1569381543
##########
lucene/core/src/test/org/apache/lucene/search/BaseKnnVectorQueryTestCase.java:
##########
@@ -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));
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Use jdk11 primitives to allow backport to branch_9x [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13311:
URL: https://github.com/apache/lucene/pull/13311#discussion_r1568758563
##########
lucene/core/src/test/org/apache/lucene/search/BaseKnnVectorQueryTestCase.java:
##########
@@ -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));
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org