You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "vinayakphegde (via GitHub)" <gi...@apache.org> on 2023/02/15 18:20:23 UTC
[GitHub] [solr] vinayakphegde opened a new pull request, #1360: SOLR 16642
vinayakphegde opened a new pull request, #1360:
URL: https://github.com/apache/solr/pull/1360
https://issues.apache.org/jira/browse/SOLR-16642
draft PR
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1470828417
No need to wait for me: https://github.com/apache/solr/pull/1450
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1454217841
TestComplexPhraseQParserPlugin: I anticipated this failure. https://github.com/apache/lucene/pull/11807
This weekend I'll see if I could fix it. I'm going to look into overriding getHighlightQuery in ComplexPhraseQParserPlugin to rewrite the query.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1108647898
##########
solr/core/src/java/org/apache/solr/update/processor/UpdateRequestProcessorChain.java:
##########
@@ -16,6 +16,7 @@
*/
package org.apache.solr.update.processor;
+import java.io.IOException;
Review Comment:
I don't think `IOException` is needed in this file.
##########
solr/core/src/java/org/apache/solr/update/processor/UpdateRequestProcessorFactory.java:
##########
@@ -16,6 +16,7 @@
*/
package org.apache.solr.update.processor;
+import java.io.IOException;
Review Comment:
I don't think `IOException` is needed in this file.
##########
solr/core/src/java/org/apache/solr/update/UpdateLog.java:
##########
@@ -1940,7 +1940,12 @@ public void doReplay(TransactionLog translog) {
ThreadLocal<UpdateRequestProcessor> procThreadLocal =
ThreadLocal.withInitial(
() -> {
- var proc = processorChain.createProcessor(req, rsp);
+ UpdateRequestProcessor proc;
+ try {
+ proc = processorChain.createProcessor(req, rsp);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
Review Comment:
After removing IOException from UpdateRequest files - this change isn't needed.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1453164773
tried to address all the issues/comments except "performance degradation for any query that isn't using timeouts" mentioned by @dsmiley
now, only `org.apache.solr.search.TestComplexPhraseQParserPlugin > testPhraseHighlighter` test is failing
```
org.apache.solr.search.TestComplexPhraseQParserPlugin > testPhraseHighlighter FAILED
java.lang.AssertionError: REQUEST FAILED: xpath=//lst[@name='1']/arr[@name='name']/str[.='<em>john</em> <em>smith</em> <em>smith</em> <em>john</em>']
xml response was: <?xml version="1.0" encoding="UTF-8"?>
<response>
<lst name="responseHeader"><int name="status">0</int><int name="QTime">0</int></lst><result name="response" numFound="2" start="0" numFoundExact="true"><doc><str name="id">1</str></doc><doc><str name="id"
>2</str></doc></result><lst name="highlighting"><lst name="1"/><lst name="2"/></lst>
</response>
request was:q=name:"(john+johathon)+smith"&defType=complexphrase&qt=&hl=true&fl=id&hl.requireFieldMatch=false&hl.usePhraseHighlighter=false&hl.fragsize=0&start=0&hl.fl=name&rows=200
at __randomizedtesting.SeedInfo.seed([3D5488D220BF1F0:B454EA75135D9B1]:0)
at org.junit.Assert.fail(Assert.java:89)
at org.apache.solr.SolrTestCaseJ4.assertQ(SolrTestCaseJ4.java:1025)
at org.apache.solr.search.TestComplexPhraseQParserPlugin.testPhraseHighlighter(TestComplexPhraseQParserPlugin.java:216)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1758)
at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:946)
at com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:982)
at com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:996)
```
looking into it now.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1454899402
Thanks Rob for chiming in.
Made two commits.
I looked at the ComplexPhraseQParser test and I don't think we should bend over backwards to support `hl.usePhraseHighlighter=false`, which is what the failing test failed to do. Trying to support it could make performance worse or extra code/complexity. It's less accurate and designed to be faster in some circumstances with older highlighters but I'd be content if this flag simply didn't exist at all. Also I noticed a commented out test that should work nowadays and it does (thanks to UnifiedHighlighter probably), so I enabled it.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1454898195
Made two commits.
I looked at the ComplexPhraseQParser changes and I don't think we should bend over backwards to support hl.usePhraseHighlighter=false, which is what the failing test failed to do. Trying to support it could make performance worse or extra code/complexity. It's less accurate and designed to be faster in some circumstances with older highlighters but I'd be pleased if this flag simply didn't exist at all.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1455020504
> Thanks Rob for chiming in.
>
> Made two commits. I looked at the ComplexPhraseQParser test and I don't think we should bend over backwards to support `hl.usePhraseHighlighter=false`, which is what the failing test failed to do. Trying to support it could make performance worse or extra code/complexity. It's less accurate and designed to be faster in some circumstances with older highlighters but I'd be content if this flag simply didn't exist at all. Also I noticed a commented out test that should work nowadays and it does (thanks to UnifiedHighlighter probably), so I enabled it.
Thanks @dsmiley for helping
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1155160754
##########
solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java:
##########
@@ -370,6 +380,12 @@ public TopDocs searchNearestVectors(
return null;
}
+ @Override
+ public TopDocs searchNearestVectors(
+ String field, byte[] target, int k, Bits acceptDocs, int visitedLimit) throws IOException {
+ return null;
Review Comment:
returning null is a lie; shouldn't we throw UnsupportedOperationException?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1122086282
##########
solr/core/src/java/org/apache/solr/core/MMapDirectoryFactory.java:
##########
@@ -63,7 +63,7 @@ protected Directory create(String path, LockFactory lockFactory, DirContext dirC
throws IOException {
MMapDirectory mapDirectory = new MMapDirectory(Path.of(path), lockFactory, maxChunk);
try {
- mapDirectory.setUseUnmap(unmapHack);
+ System.setProperty(MMapDirectory.ENABLE_UNMAP_HACK_SYSPROP, String.valueOf(unmapHack));
Review Comment:
I vote to remove the `unmapHack` references. `unmap` isn't documented in Solr codebase anyway. https://github.com/search?q=repo%3Aapache%2Fsolr%20unmap&type=code
If someone is down at this level - they can follow the Lucene docs for disabling this - https://lucene.apache.org/core/9_5_0/core/org/apache/lucene/store/MMapDirectory.html#ENABLE_UNMAP_HACK_SYSPROP
If we want to be nicer to users - then logging a warning is a valid option.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1432592878
that's nice!
let me also look into it
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1113288086
##########
solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java:
##########
@@ -328,6 +331,17 @@ public Fields getTermVectors(int docID) throws IOException {
return in.getTermVectors(docID);
}
+ @Override
+ public TermVectors termVectors() throws IOException {
+ return in.termVectors();
Review Comment:
`ensureOpen()` was removed as part of [this commit](https://github.com/apache/solr/commit/797bb38c50fed9e533ca20de920c63c6c35256a9#diff-a3934253a307f6bc36771c8d549e5b5bb7bad12fd5dbe4de06e7b5ef176e2083).
Looks like an oversight perhaps. I'd be inclined to put it back (for consistency with the other methods). @markrmiller any recollection why this was removed? Any objection to restoring it?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1112873846
##########
solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java:
##########
@@ -328,6 +331,17 @@ public Fields getTermVectors(int docID) throws IOException {
return in.getTermVectors(docID);
}
+ @Override
+ public TermVectors termVectors() throws IOException {
+ return in.termVectors();
Review Comment:
Yeah, all the other functions seem to have `ensureOpen()` apart from `getTermVectors(int docID)` and `termVectors()` (of course I added this one).
Should I add for both functions or should I add only for `termVectors()` and will take care of `getTermVectors(int docID)` in some other PR?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] uschindler commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1125416874
##########
solr/core/src/java/org/apache/solr/core/MMapDirectoryFactory.java:
##########
@@ -63,7 +63,7 @@ protected Directory create(String path, LockFactory lockFactory, DirContext dirC
throws IOException {
MMapDirectory mapDirectory = new MMapDirectory(Path.of(path), lockFactory, maxChunk);
try {
- mapDirectory.setUseUnmap(unmapHack);
+ System.setProperty(MMapDirectory.ENABLE_UNMAP_HACK_SYSPROP, String.valueOf(unmapHack));
Review Comment:
Like `if (params.get("unmap") != null)` (this is a good check for existence in SolrParams.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1112862200
##########
solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java:
##########
@@ -359,14 +373,26 @@ public PointValues getPointValues(String field) {
}
@Override
- public VectorValues getVectorValues(String field) {
+ public FloatVectorValues getFloatVectorValues(String field) {
+ ensureOpen();
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public ByteVectorValues getByteVectorValues(String field) {
ensureOpen();
- return VectorValues.EMPTY;
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public TopDocs searchNearestVectors(
+ String field, float[] target, int k, Bits acceptDocs, int visitedLimit) {
+ return null;
}
@Override
public TopDocs searchNearestVectors(
- String field, float[] target, int k, Bits acceptDocs, int visitedLimit) throws IOException {
+ String field, byte[] target, int k, Bits acceptDocs, int visitedLimit) {
Review Comment:
Okay, Will add back the `throws IOException` to the `float[]` variant as well as the `byte[]` variant of `searchNearestVectors()`
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1118110352
##########
solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java:
##########
@@ -328,6 +331,17 @@ public Fields getTermVectors(int docID) throws IOException {
return in.getTermVectors(docID);
}
+ @Override
+ public TermVectors termVectors() throws IOException {
+ return in.termVectors();
Review Comment:
Hey @magibney, should we wait for his reply or should we add it back?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1433165273
even with the new change also we are returning the same `Term` instance, right?
How does that make the `Term` instance thread-safe?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1125810099
##########
solr/core/src/java/org/apache/solr/search/SolrQueryTimeoutImpl.java:
##########
@@ -45,7 +45,6 @@ public static Long getTimeoutAtNs() {
return timeoutAt.get();
}
- @Override
Review Comment:
Okay, let's. It may be a release blocker; hopefully there are benchmarks that can show yes/no. FYI @chatman
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1454873263
I'll be sad to see the Terms caching go in SlowCompositeReaderWrapper but maybe it has to be this way. Although I added the test of this, removed here, the underlying caching pre-dated me (hiding in MultiFields) in Lucene and I don't recall when it was added. I [asked in Lucene](https://github.com/apache/lucene/pull/11998#issuecomment-1454862916) about the change which brought about the new assertion which prompted reverting this caching. I don't like the idea of adding more thread-locals as a work-around. Instead, maybe Solr can avoid some uses of SlowCompositeReaderWrapper where terms is used and retain reasonable performance? I'm thinking of JoinUtils which could change its algorithm to loop over the leaves instead of assuming one leaf. For another issue; it's too complicated / exploratory to be in this PR. There are uses elsewhere too like in Collapsing when top_fc is used. BTW SlowCompositeReaderWrapper is named this on purpose so that people don't use it ;-)
Ideally we'd have some benchmark somewhere that uses join queries. I know @gerlowskija and @joel-bernstein have worked on such as I've code-reviewed related things from them. They are probably proprietary / not readily available but doesn't hurt to ping them :-). After this change merges, we could broadcast to the users list that there is a change that maybe has a perf regression, inviting users to test it for themselves soon. https://hub.docker.com/r/apache/solr-nightly/tags
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1433033954
The map is explicitly designed to lazily cache these `Terms` instances (accumulating them over time for a given `SolrIndexSearcher`). The returned `Terms` instances are _not_ safe for concurrent use -- considering which, I'm surprised we didn't see actual errors resulting from this usage!
I suspect the correct solution will be to just skip caching the `Terms` instances altogether. We could also upgrade from Map to use a proper cache (with evictions, etc.) or some other caching strategy that effectively incorporates "creation thread" into the cache key. Not sure whether that would be worth it.
It's also possible that the _way_ in which `Terms` instances returned from `SlowCompositeReaderWrapper` are used in practice actually _is_ thread-safe? In that case there might be other options, such as pulling the required thread-safe derivative of the `Terms` instance, and wrapping it in a synthetic `Terms` instance that mostly throws `UnsupportedOperationException`, and caching/returning that synthetic instance?
Curious what others think here ... perhaps @dsmiley?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1125618121
##########
solr/core/src/java/org/apache/solr/search/SolrQueryTimeoutImpl.java:
##########
@@ -45,7 +45,6 @@ public static Long getTimeoutAtNs() {
return timeoutAt.get();
}
- @Override
Review Comment:
hi @dsmiley, can we address this issue in separate Jira or should we include it in this only?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1121805468
##########
solr/core/src/java/org/apache/solr/search/SolrQueryTimeoutImpl.java:
##########
@@ -45,7 +45,6 @@ public static Long getTimeoutAtNs() {
return timeoutAt.get();
}
- @Override
Review Comment:
Thanks, @dsmiley helping us here.
didn't understand fully, but will try again :)
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] uschindler commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1125416518
##########
solr/core/src/java/org/apache/solr/core/MMapDirectoryFactory.java:
##########
@@ -63,7 +63,7 @@ protected Directory create(String path, LockFactory lockFactory, DirContext dirC
throws IOException {
MMapDirectory mapDirectory = new MMapDirectory(Path.of(path), lockFactory, maxChunk);
try {
- mapDirectory.setUseUnmap(unmapHack);
+ System.setProperty(MMapDirectory.ENABLE_UNMAP_HACK_SYSPROP, String.valueOf(unmapHack));
Review Comment:
No not in create().
On init() just check for existence of directory property "unmap", if it exists show exactly this warning. Maybe add something "Per directory unmapping is no longer supported with Lucene 9.5. ...."
If user does not specify anything in the XML file, no warning is printed.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1155251746
##########
solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java:
##########
@@ -370,6 +380,12 @@ public TopDocs searchNearestVectors(
return null;
}
+ @Override
+ public TopDocs searchNearestVectors(
+ String field, byte[] target, int k, Bits acceptDocs, int visitedLimit) throws IOException {
+ return null;
Review Comment:
there is an overloaded function with `float[] target`, which was returning null, So I just copied that. But UnsupportedOperationException makes more sense. Should I change both function?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1122863133
##########
solr/core/src/java/org/apache/solr/core/MMapDirectoryFactory.java:
##########
@@ -63,7 +63,7 @@ protected Directory create(String path, LockFactory lockFactory, DirContext dirC
throws IOException {
MMapDirectory mapDirectory = new MMapDirectory(Path.of(path), lockFactory, maxChunk);
try {
- mapDirectory.setUseUnmap(unmapHack);
+ System.setProperty(MMapDirectory.ENABLE_UNMAP_HACK_SYSPROP, String.valueOf(unmapHack));
Review Comment:
Okay, will remove the `unmapHack` references.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] uschindler commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1122054880
##########
solr/core/src/java/org/apache/solr/core/MMapDirectoryFactory.java:
##########
@@ -63,7 +63,7 @@ protected Directory create(String path, LockFactory lockFactory, DirContext dirC
throws IOException {
MMapDirectory mapDirectory = new MMapDirectory(Path.of(path), lockFactory, maxChunk);
try {
- mapDirectory.setUseUnmap(unmapHack);
+ System.setProperty(MMapDirectory.ENABLE_UNMAP_HACK_SYSPROP, String.valueOf(unmapHack));
Review Comment:
If somebody wants to use this, starting with Lucene 9.5, you have to pass this sysprop in `solr.in.sh`.
I was not aware that this setting was still available in Solr's configs.... oh man oh man. When deprecating it I checked Github search to find usages. I found it in older Solr versions but no third party code. Also nowhere in user's solrconfig.xml I found any instance of this flag.
So maybe just ignore it an log a warning that you need to pass the setting in Solr's startup script to have an effect. The current implementation won't work at all, because whe this DirectoryFactory is loaded, it implicitely loads MMapDirectory. This causes static initializer to run and the setting is read from sysprop. Changing it later has no effect anymore.
##########
solr/core/src/java/org/apache/solr/core/MMapDirectoryFactory.java:
##########
@@ -63,7 +63,7 @@ protected Directory create(String path, LockFactory lockFactory, DirContext dirC
throws IOException {
MMapDirectory mapDirectory = new MMapDirectory(Path.of(path), lockFactory, maxChunk);
try {
- mapDirectory.setUseUnmap(unmapHack);
+ System.setProperty(MMapDirectory.ENABLE_UNMAP_HACK_SYSPROP, String.valueOf(unmapHack));
Review Comment:
this wont work at all, because it is not dynamic. It only works if it is populated at startup time, because the property is read on early booting. Remove this completely.
You can still keep the setting but make it throw UOE. That is what happens when you call the deprecated method on Lucene 9.5.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on pull request #1360: SOLR 16642
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1431899551
Thanks @magibney
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley merged pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #1360:
URL: https://github.com/apache/solr/pull/1360
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1360: SOLR 16642
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1107551800
##########
solr/core/src/java/org/apache/solr/handler/component/TermVectorComponent.java:
##########
@@ -277,15 +277,15 @@ public void process(ResponseBuilder rb) throws IOException {
if (null != fields) {
for (Map.Entry<String, FieldOptions> entry : fieldOptions.entrySet()) {
final String field = entry.getKey();
- final Terms vector = reader.getTermVector(docId, field);
+ final Terms vector = reader.termVectors().get(docId).terms(field);
Review Comment:
I think `reader.termVectors()` can be pulled out of the while loop? near line 253
##########
solr/core/src/java/org/apache/solr/handler/component/ResponseLogComponent.java:
##########
@@ -92,7 +92,8 @@ protected void processIds(
Set<String> fields = Collections.singleton(schema.getUniqueKeyField().getName());
for (DocIterator iter = dl.iterator(); iter.hasNext(); ) {
- sb.append(schema.printableUniqueKey(searcher.doc(iter.nextDoc(), fields))).append(',');
+ sb.append(schema.printableUniqueKey(searcher.storedFields().document(iter.nextDoc(), fields)))
Review Comment:
I think `searcher.storedFields()` can be pulled out of the for loop?
##########
solr/core/src/java/org/apache/solr/handler/component/TermVectorComponent.java:
##########
@@ -277,15 +277,15 @@ public void process(ResponseBuilder rb) throws IOException {
if (null != fields) {
for (Map.Entry<String, FieldOptions> entry : fieldOptions.entrySet()) {
final String field = entry.getKey();
- final Terms vector = reader.getTermVector(docId, field);
+ final Terms vector = reader.termVectors().get(docId).terms(field);
if (vector != null) {
TermsEnum termsEnum = vector.iterator();
mapOneVector(docNL, entry.getValue(), reader, docId, termsEnum, field);
}
}
} else {
// extract all fields
- final Fields vectors = reader.getTermVectors(docId);
+ final Fields vectors = reader.termVectors().get(docId);
Review Comment:
I think reader.termVectors() can be pulled out of the while loop? near line 253
##########
solr/core/src/java/org/apache/solr/handler/component/ResponseLogComponent.java:
##########
@@ -106,7 +107,7 @@ protected void processScores(
StringBuilder sb = new StringBuilder();
Set<String> fields = Collections.singleton(schema.getUniqueKeyField().getName());
for (DocIterator iter = dl.iterator(); iter.hasNext(); ) {
- sb.append(schema.printableUniqueKey(searcher.doc(iter.nextDoc(), fields)))
+ sb.append(schema.printableUniqueKey(searcher.storedFields().document(iter.nextDoc(), fields)))
Review Comment:
I think `searcher.storedFields()` can be pulled out of the for loop? probably reused with the one from line 95?
##########
solr/core/src/java/org/apache/solr/handler/component/MoreLikeThisComponent.java:
##########
@@ -390,7 +390,7 @@ NamedList<DocList> getMoreLikeThese(
DocListAndSet similarDocuments =
mltHelper.getMoreLikeThis(id, 0, rows, null, interestingTerms, flags);
- String name = schema.printableUniqueKey(searcher.doc(id));
+ String name = schema.printableUniqueKey(searcher.storedFields().document(id));
Review Comment:
`searcher.storedFields()` can be pulled out of the while loop?
##########
solr/core/src/java/org/apache/solr/handler/MoreLikeThisHandler.java:
##########
@@ -472,7 +472,7 @@ public NamedList<BooleanQuery> getMoreLikeTheseQuery(DocList docs) throws IOExce
DocIterator iterator = docs.iterator();
while (iterator.hasNext()) {
int id = iterator.nextDoc();
- String uniqueId = schema.printableUniqueKey(reader.document(id));
+ String uniqueId = schema.printableUniqueKey(reader.storedFields().document(id));
Review Comment:
`reader.storedFields()` should be pulled out of the while loop.
##########
solr/core/src/java/org/apache/solr/handler/admin/IndexSizeEstimator.java:
##########
@@ -388,7 +388,7 @@ private void estimateTermVectors(Map<String, Object> result) throws IOException
if (liveDocs != null && !liveDocs.get(docId)) {
continue;
}
- Fields termVectors = leafReader.getTermVectors(docId);
Review Comment:
`leafReader.termVectors()` should be pulled out of the docId for loop
##########
solr/core/src/java/org/apache/solr/handler/admin/IndexSizeEstimator.java:
##########
@@ -612,7 +612,7 @@ private void estimateStoredFields(Map<String, Object> result) throws IOException
if (liveDocs != null && !liveDocs.get(docId)) {
continue;
}
- leafReader.document(docId, visitor);
+ leafReader.storedFields().document(docId, visitor);
Review Comment:
`leafReader.storedFields()` should be pulled out of the docId for loop
##########
solr/core/src/test/org/apache/solr/search/function/TestOrdValues.java:
##########
@@ -96,7 +96,7 @@ private void doTestRank(String field, boolean inOrder) throws Exception {
: "IC"; // smaller than all ids of docs in this test ("ID0001", etc.)
for (int i = 0; i < h.length; i++) {
- String resID = s.doc(h[i].doc).get(ID_FIELD);
+ String resID = s.storedFields().document(h[i].doc).get(ID_FIELD);
Review Comment:
`s.storedFields()` should be pulled out of the for loop.
##########
solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java:
##########
@@ -252,7 +252,7 @@ public static void optimizePreFetchDocs(
// get documents
DocIterator iter = docs.iterator();
for (int i = 0; i < docs.size(); i++) {
- searcher.doc(iter.nextDoc(), fieldFilter);
+ searcher.storedFields().document(iter.nextDoc(), fieldFilter);
Review Comment:
`searcher.storedFields()` should be pulled out of the for loop.
##########
solr/core/src/test/org/apache/solr/search/function/TestOrdValues.java:
##########
@@ -139,7 +139,7 @@ private void doTestExactScore(String field, boolean inOrder) throws Exception {
ScoreDoc sd[] = td.scoreDocs;
for (int i = 0; i < sd.length; i++) {
float score = sd[i].score;
- String id = s.getIndexReader().document(sd[i].doc).get(ID_FIELD);
+ String id = s.getIndexReader().storedFields().document(sd[i].doc).get(ID_FIELD);
Review Comment:
`s.getIndexReader().storedFields()` should be pulled out of the for loop.
##########
solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java:
##########
@@ -409,7 +409,7 @@ public static NamedList<Explanation> getExplanations(
for (int i = 0; i < docs.size(); i++) {
int id = iterator.nextDoc();
- Document doc = searcher.doc(id);
+ Document doc = searcher.storedFields().document(id);
Review Comment:
`searcher.storedFields()` should be pulled out of the for loop.
##########
solr/core/src/test/org/apache/solr/uninverting/TestFieldCacheVsDocValues.java:
##########
@@ -175,7 +175,7 @@ public void testHugeBinaryValues() throws Exception {
BinaryDocValues s = FieldCache.DEFAULT.getTerms(ar, "field");
for (int docID = 0; docID < docBytes.size(); docID++) {
- Document doc = ar.document(docID);
+ Document doc = ar.storedFields().document(docID);
Review Comment:
`ar.storedFields()` should be pulled out of the for loop
##########
solr/core/src/java/org/apache/solr/update/processor/ClassificationUpdateProcessor.java:
##########
@@ -60,7 +60,8 @@ public ClassificationUpdateProcessor(
ClassificationUpdateProcessorParams classificationParams,
UpdateRequestProcessor next,
IndexReader indexReader,
- IndexSchema schema) {
+ IndexSchema schema)
+ throws IOException {
Review Comment:
I see a bunch of these new `IOException` throws are they required?
##########
solr/core/src/java/org/apache/solr/core/QuerySenderListener.java:
##########
@@ -86,7 +86,7 @@ public void close() {}
if (o instanceof DocList) {
DocList docs = (DocList) o;
for (DocIterator iter = docs.iterator(); iter.hasNext(); ) {
- newSearcher.doc(iter.nextDoc());
+ newSearcher.storedFields().document(iter.nextDoc());
Review Comment:
`newSearcher.storedFields()` should be pulled out of the for loop.
##########
solr/core/src/test/org/apache/solr/handler/tagger/TaggerTestCase.java:
##########
@@ -144,7 +144,7 @@ protected TestTag[] pullTagsFromResponse(SolrQueryRequest req, SolrQueryResponse
DocIterator iter = docList.iterator();
while (iter.hasNext()) {
int docId = iter.next();
- Document doc = searcher.doc(docId);
+ Document doc = searcher.storedFields().document(docId);
Review Comment:
`searcher.storedFields()` should be pulled out of the for loop.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1432998084
> Sharing early here (not immediately sure about the _right_ way to fix this); it looks like there's a problem with `Terms` caching in `SlowCompositeReaderWrapper`; tests pass (aside from the test that explicitly tests for `Terms` caching in `SlowCompositeReaderWrapper` 🙂) with:
>
> ```diff
> diff --git a/solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java b/solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java
> index f638fbd664f..5765b1e6d71 100644
> --- a/solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java
> +++ b/solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java
> @@ -140,6 +140,9 @@ public final class SlowCompositeReaderWrapper extends LeafReader {
> public Terms terms(String field) throws IOException {
> ensureOpen();
> try {
> + if (true) {
> + return MultiTerms.getTerms(in, field);
> + }
> return cachedTerms.computeIfAbsent(
> field,
> f -> {
> ```
>
> I also happened to hit an unrelated reproducible failure:
>
> ```
> $ ./gradlew :solr:core:test --tests "org.apache.solr.search.TestComplexPhraseQParserPlugin.testPhraseHighlighter" -Ptests.jvms=5 "-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC -XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" -Ptests.seed=56C83FBC07AAE2C3 -Ptests.file.encoding=ISO-8859-1
> ```
yeah, this change solved the problem, But I just want to know
what was the problem here and how this change(not involving the map) solved the problem?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1118137048
##########
solr/core/src/java/org/apache/solr/search/SolrQueryTimeoutImpl.java:
##########
@@ -45,7 +45,6 @@ public static Long getTimeoutAtNs() {
return timeoutAt.get();
}
- @Override
Review Comment:
This innocuous looking removal was very curious to me and I went digging into Lucene's changes that led to this. Based on what I learned, I predict a performance degradation for any query that isn't using timeouts. The Lucene issue is https://github.com/apache/lucene/issues/11914 and it means that if Solr does nothing about this, then ExitableDirectoryReader.wrap (used in SolrIndexSearcher) will wrap all underlying low level index components (Terms, ...) because Lucene's ExitableDirectoryReader no longer guards against this with a "isTimeoutEnabled" check as it no longer exists. Solr could fork the old ExitableDirectoryReader, which would be sad but pretty easy. Alternatively, maybe SolrIndexSearcher could only wrap for the execution of a query -- not sure as I haven't explored this.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1489687637
Lets get this updated and merged; ehh?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1110386683
##########
solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java:
##########
@@ -359,14 +373,26 @@ public PointValues getPointValues(String field) {
}
@Override
- public VectorValues getVectorValues(String field) {
+ public FloatVectorValues getFloatVectorValues(String field) {
+ ensureOpen();
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public ByteVectorValues getByteVectorValues(String field) {
ensureOpen();
- return VectorValues.EMPTY;
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public TopDocs searchNearestVectors(
+ String field, float[] target, int k, Bits acceptDocs, int visitedLimit) {
+ return null;
}
@Override
public TopDocs searchNearestVectors(
- String field, float[] target, int k, Bits acceptDocs, int visitedLimit) throws IOException {
+ String field, byte[] target, int k, Bits acceptDocs, int visitedLimit) {
Review Comment:
I think removing the `throws IOException` from the `float[]` variant of `searchNearestVectors()` is out-of-scope for this PR (the method it inherits throws IOException). Among other things, it makes the diff less clear wrt what's been added:
To clarify, this upgrade adds a `byte[]` variant of `searchNearestVectors()` and leaves the existing `float[]` variant as-is. True, because this class is final, there's probably no negative consequence to removing the `throws IOException` (from either variant, `byte[]` or `float[]`).
But my inclination would be to try to keep changes as tightly scoped as possible, and probably even follow existing precedent and add `throws IOException` to the new new `byte[]` variant.
Curious what others think? I definitely appreciate the clarity in the diff that results from restoring the `throws IOException` here.
##########
solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java:
##########
@@ -328,6 +331,17 @@ public Fields getTermVectors(int docID) throws IOException {
return in.getTermVectors(docID);
}
+ @Override
+ public TermVectors termVectors() throws IOException {
+ return in.termVectors();
Review Comment:
I know this follows precedent from the existing `termVectors()` method above, but I wonder why these don't call `ensureOpen()` the wall all the other "content-access" methods seem to do?
##########
solr/core/src/java/org/apache/solr/update/processor/ClassificationUpdateProcessor.java:
##########
@@ -77,17 +77,21 @@ public ClassificationUpdateProcessor(
}
switch (classificationAlgorithm) {
case KNN:
- classifier =
- new KNearestNeighborDocumentClassifier(
- indexReader,
- null,
- classificationParams.getTrainingFilterQuery(),
- classificationParams.getK(),
- classificationParams.getMinDf(),
- classificationParams.getMinTf(),
- trainingClassField,
- field2analyzer,
- inputFieldNamesWithBoost);
+ try {
+ classifier =
+ new KNearestNeighborDocumentClassifier(
+ indexReader,
+ null,
+ classificationParams.getTrainingFilterQuery(),
+ classificationParams.getK(),
+ classificationParams.getMinDf(),
+ classificationParams.getMinTf(),
+ trainingClassField,
+ field2analyzer,
+ inputFieldNamesWithBoost);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
Review Comment:
Prefer to wrap this in SolrException with more information about context (as opposed to a generic RuntimeException).
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1125349206
##########
solr/core/src/java/org/apache/solr/core/MMapDirectoryFactory.java:
##########
@@ -63,7 +63,7 @@ protected Directory create(String path, LockFactory lockFactory, DirContext dirC
throws IOException {
MMapDirectory mapDirectory = new MMapDirectory(Path.of(path), lockFactory, maxChunk);
try {
- mapDirectory.setUseUnmap(unmapHack);
+ System.setProperty(MMapDirectory.ENABLE_UNMAP_HACK_SYSPROP, String.valueOf(unmapHack));
Review Comment:
you mean something like this?
```
@Override
protected Directory create(String path, LockFactory lockFactory, DirContext dirContext)
throws IOException {
MMapDirectory mapDirectory = new MMapDirectory(Path.of(path), lockFactory, maxChunk);
log.warn("to disable the unmap hack globally, see this Lucene documentation" +
" https://lucene.apache.org/core/9_5_0/core/org/apache/lucene/store/MMapDirectory.html#ENABLE_UNMAP_HACK_SYSPROP");
mapDirectory.setPreload(preload);
return mapDirectory;
}
```
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] uschindler commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1125444982
##########
solr/core/src/java/org/apache/solr/core/MMapDirectoryFactory.java:
##########
@@ -63,7 +63,7 @@ protected Directory create(String path, LockFactory lockFactory, DirContext dirC
throws IOException {
MMapDirectory mapDirectory = new MMapDirectory(Path.of(path), lockFactory, maxChunk);
try {
- mapDirectory.setUseUnmap(unmapHack);
+ System.setProperty(MMapDirectory.ENABLE_UNMAP_HACK_SYSPROP, String.valueOf(unmapHack));
Review Comment:
Do it like that:
```java
public class MMapDirectoryFactory extends StandardDirectoryFactory {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
boolean preload;
private long maxChunk;
@Override
public void init(NamedList<?> args) {
super.init(args);
SolrParams params = args.toSolrParams();
maxChunk = params.getLong("maxChunkSize", MMapDirectory.DEFAULT_MAX_CHUNK_SIZE);
if (maxChunk <= 0) {
throw new IllegalArgumentException("maxChunk must be greater than 0");
}
if (params.get("unmap") != null) {
log.warn("It is no longer possible to configure unmapping of index files on DirectoryFactory level in solrconfig.xml.");
log.warn("To disable unmapping, pass -Dorg.apache.lucene.store.MMapDirectory.enableUnmapHack=false on Solr's command line.");
}
preload = params.getBool("preload", false); // default turn-off
}
@Override
protected Directory create(String path, LockFactory lockFactory, DirContext dirContext)
throws IOException {
MMapDirectory mapDirectory = new MMapDirectory(Path.of(path), lockFactory, maxChunk);
mapDirectory.setPreload(preload);
return mapDirectory;
}
}
```
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on pull request #1360: SOLR 16642
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1431886125
@magibney you are right. I will revert that change.
at this moment I am not sure what causes these test failures
`Thread[searcherExecutor-320-thread-1,5,TGRP-TestJoin] and consumed in Thread[TEST-TestJoin.testRandomJoin-seed#[CA39E6C650AD9BC9],5,TGRP-TestJoin]`
but I have seen a lot of test cases failing with this error
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #1360: SOLR 16642
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1431890777
Sounds good! Definitely happy to take a look at the test failures, that's part of why I was thinking that a draft PR would be helpful (thanks for opening it btw!).
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1136611886
##########
solr/core/src/java/org/apache/solr/search/SolrQueryTimeoutImpl.java:
##########
@@ -45,7 +45,6 @@ public static Long getTimeoutAtNs() {
return timeoutAt.get();
}
- @Override
Review Comment:
Hi @dsmiley, any update on this one?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1108868836
##########
solr/core/src/java/org/apache/solr/update/UpdateLog.java:
##########
@@ -1940,7 +1940,12 @@ public void doReplay(TransactionLog translog) {
ThreadLocal<UpdateRequestProcessor> procThreadLocal =
ThreadLocal.withInitial(
() -> {
- var proc = processorChain.createProcessor(req, rsp);
+ UpdateRequestProcessor proc;
+ try {
+ proc = processorChain.createProcessor(req, rsp);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
Review Comment:
Yea lets not propagate `IOEXception` everywhere if we don't need to.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1108754672
##########
solr/core/src/java/org/apache/solr/update/UpdateLog.java:
##########
@@ -1940,7 +1940,12 @@ public void doReplay(TransactionLog translog) {
ThreadLocal<UpdateRequestProcessor> procThreadLocal =
ThreadLocal.withInitial(
() -> {
- var proc = processorChain.createProcessor(req, rsp);
+ UpdateRequestProcessor proc;
+ try {
+ proc = processorChain.createProcessor(req, rsp);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
Review Comment:
Hi @risdenk
if we see this lucene commit https://github.com/apache/lucene/pull/11998/files , the `KNearestNeighborDocumentClassifier` constructor now throws IOException
we are using that in `ClassificationUpdateProcessor`'s constructor
so that's why I added `throws IOExceptio`n in the `ClassificationUpdateProcessor` constructor
and we are calling `ClassificationUpdateProcessor` from ClassificationUpdateProcessorFactory#getInstance()
so I had to throw IOException from ClassificationUpdateProcessorFactory#getInstance()
since it is inherited from UpdateRequestProcessorFactory, I had to add it there as well.
and so on for other related files like this.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1118127401
##########
solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java:
##########
@@ -328,6 +331,17 @@ public Fields getTermVectors(int docID) throws IOException {
return in.getTermVectors(docID);
}
+ @Override
+ public TermVectors termVectors() throws IOException {
+ return in.termVectors();
Review Comment:
I'd recommend to go ahead and add it back. Thanks for following up!
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1125445848
##########
solr/core/src/java/org/apache/solr/core/MMapDirectoryFactory.java:
##########
@@ -63,7 +63,7 @@ protected Directory create(String path, LockFactory lockFactory, DirContext dirC
throws IOException {
MMapDirectory mapDirectory = new MMapDirectory(Path.of(path), lockFactory, maxChunk);
try {
- mapDirectory.setUseUnmap(unmapHack);
+ System.setProperty(MMapDirectory.ENABLE_UNMAP_HACK_SYSPROP, String.valueOf(unmapHack));
Review Comment:
Sure, will do that. Thanks
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #1360: SOLR 16642
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1432117591
Sharing early here (not immediately sure about the _right_ way to fix this); it looks like there's a problem with `Terms` caching in `SlowCompositeReaderWrapper`; tests pass (aside from the test that explicitly tests for `Terms` caching in `SlowCompositeReaderWrapper` 🙂) with:
```patch
diff --git a/solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java b/solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java
index f638fbd664f..5765b1e6d71 100644
--- a/solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java
+++ b/solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java
@@ -140,6 +140,9 @@ public final class SlowCompositeReaderWrapper extends LeafReader {
public Terms terms(String field) throws IOException {
ensureOpen();
try {
+ if (true) {
+ return MultiTerms.getTerms(in, field);
+ }
return cachedTerms.computeIfAbsent(
field,
f -> {
```
I also happened to hit an unrelated reproducible failure:
```
$ ./gradlew :solr:core:test --tests "org.apache.solr.search.TestComplexPhraseQParserPlugin.testPhraseHighlighter" -Ptests.jvms=5 "-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC -XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" -Ptests.seed=56C83FBC07AAE2C3 -Ptests.file.encoding=ISO-8859-1
```
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1469456142
can I make it a regular PR now?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1489754905
> Lets get this updated and merged; ehh?
Sure, need anything from my side? I mean, at this moment?
I think I addressed all the previous comments!!
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1108757653
##########
solr/core/src/java/org/apache/solr/update/UpdateLog.java:
##########
@@ -1940,7 +1940,12 @@ public void doReplay(TransactionLog translog) {
ThreadLocal<UpdateRequestProcessor> procThreadLocal =
ThreadLocal.withInitial(
() -> {
- var proc = processorChain.createProcessor(req, rsp);
+ UpdateRequestProcessor proc;
+ try {
+ proc = processorChain.createProcessor(req, rsp);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
Review Comment:
or we can handle the exception in ClassificationUpdateProcessor's constructor itself. so that we don't have to propagate these changes everywhere.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1433204163
Okay, got it.
yeah, if you find any solution, ping me. I will try that out and test it in my system.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1433170936
No, the change results in never consulting the map at all; any caller of `SlowCompositeReaderWrapper.terms(String field)` will get a freshly-constructed (uncached) `Terms` instance. The caching is what allowed the `Terms` instances to "leak" across thread boundaries, so this change fixes the issue by removing caching.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1112907433
##########
solr/core/src/java/org/apache/solr/update/processor/ClassificationUpdateProcessor.java:
##########
@@ -77,17 +77,21 @@ public ClassificationUpdateProcessor(
}
switch (classificationAlgorithm) {
case KNN:
- classifier =
- new KNearestNeighborDocumentClassifier(
- indexReader,
- null,
- classificationParams.getTrainingFilterQuery(),
- classificationParams.getK(),
- classificationParams.getMinDf(),
- classificationParams.getMinTf(),
- trainingClassField,
- field2analyzer,
- inputFieldNamesWithBoost);
+ try {
+ classifier =
+ new KNearestNeighborDocumentClassifier(
+ indexReader,
+ null,
+ classificationParams.getTrainingFilterQuery(),
+ classificationParams.getK(),
+ classificationParams.getMinDf(),
+ classificationParams.getMinTf(),
+ trainingClassField,
+ field2analyzer,
+ inputFieldNamesWithBoost);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
Review Comment:
okay.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] uschindler commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1124940846
##########
solr/core/src/java/org/apache/solr/core/MMapDirectoryFactory.java:
##########
@@ -63,7 +63,7 @@ protected Directory create(String path, LockFactory lockFactory, DirContext dirC
throws IOException {
MMapDirectory mapDirectory = new MMapDirectory(Path.of(path), lockFactory, maxChunk);
try {
- mapDirectory.setUseUnmap(unmapHack);
+ System.setProperty(MMapDirectory.ENABLE_UNMAP_HACK_SYSPROP, String.valueOf(unmapHack));
Review Comment:
I'd still add a warning message.
The default was always "true". If somebody changed it from the default, I'd log a warning with with the URL to the Lucene documentation as stated before.
> If we want to be nicer to users - then logging a warning is a valid option.
This would bring both together.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rmuir commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1454886054
Creating this MultiTerms doesn't really do anything. It doesn't do any IO or anything like that. It essentially makes a list of reader.terms() for each segment. It doesn't sort this list or anything.
I wouldn't suggest doing anything crazy to cache this thing, as it doesn't make sense to cache: there's no "hot" methods on Terms (nextDoc or nextTerm or anything) that you'd call a ton of times, and hopefully use of slow wrapper is minimized anyway.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1494471486
Thanks for the work all of y'all have put in here!
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on PR #1360:
URL: https://github.com/apache/solr/pull/1360#issuecomment-1493249446
> You forgot [Uwe's comment](https://issues.apache.org/jira/browse/SOLR-16642?focusedCommentId=17683095&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17683095) out reverting the `--enable-preview`. I'll do this now.
>
> I'll also throw UOE as I suggested in this review.
thanks @dsmiley for working on @uschindler comment
> Can you comment on why org.apache.solr.update.processor.UpdateRequestProcessor#extractTypedNamedEntities needed to be updated? Did the API change and you just updated it to comply with the change? It's not apparently purely looking at the PR.
>
> I'll add a CHANGES.txt as well and probably merge Monday.
that is no longer needed, we are handling the exception in between. will change it.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org