You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/09/28 13:15:35 UTC
[GitHub] [solr] cpoerschke opened a new pull request #313: SOLR-13681: make Lucene's index sorting directly configurable in Solr
cpoerschke opened a new pull request #313:
URL: https://github.com/apache/solr/pull/313
https://issues.apache.org/jira/browse/SOLR-13681
--
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] cpoerschke commented on pull request #313: SOLR-13681: make Lucene's index sorting directly configurable in Solr
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #313:
URL: https://github.com/apache/solr/pull/313#issuecomment-930143165
> Great.
>
> Add a CHANGES entry. I may try to re-format the RefGuide edits from the patch later...
Thanks for attaching SOLR-13681-refguide-skel.patch to the JIRA ticket!
If you wish, `Allow edits and access to secrets by maintainers` is enabled for this pull request, feel free to add documentation or code or test change commits directly to the pull request branch 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] cpoerschke commented on a change in pull request #313: SOLR-13681: make Lucene's index sorting directly configurable in Solr
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #313:
URL: https://github.com/apache/solr/pull/313#discussion_r718467688
##########
File path: solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java
##########
@@ -146,6 +146,7 @@ public void testSortingMPSolrIndexConfigCreation() throws Exception {
final Sort expected = new Sort(new SortField(expectedFieldName, expectedFieldType, expectedFieldSortDescending));
final Sort actual = sortingMergePolicy.getSort();
assertEquals("SortingMergePolicy.getSort", expected, actual);
+ assertEquals("indexSort", expected, iwc.getIndexSort());
Review comment:
Yes, and perhaps changing some `indexConfig` parameters after collection creation would be fine but not so for others. Also the merge policy (factory) configuration is nested, not sure if config-api would easily support that.
Added a new method, modelled on the existing sorting-merge-policy one that will go away with sorting merge policy. There is existing `TestSegmentSorting` coverage w.r.t. index sorting working in a test, so it might make sense to tap into that somehow?
--
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] cpoerschke commented on a change in pull request #313: SOLR-13681: make Lucene's index sorting directly configurable in Solr
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #313:
URL: https://github.com/apache/solr/pull/313#discussion_r718271490
##########
File path: solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java
##########
@@ -146,6 +146,7 @@ public void testSortingMPSolrIndexConfigCreation() throws Exception {
final Sort expected = new Sort(new SortField(expectedFieldName, expectedFieldType, expectedFieldSortDescending));
final Sort actual = sortingMergePolicy.getSort();
assertEquals("SortingMergePolicy.getSort", expected, actual);
+ assertEquals("indexSort", expected, iwc.getIndexSort());
Review comment:
Yes, I would also favour not creating yet another solrconfig-xyz.xml file. Have used the config-api in tests before but from my research done so far it seems `indexConfig` is not yet supported by it, or rather 'get' works but 'set' does not?
--
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] janhoy commented on a change in pull request #313: SOLR-13681: make Lucene's index sorting directly configurable in Solr
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #313:
URL: https://github.com/apache/solr/pull/313#discussion_r717856962
##########
File path: solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java
##########
@@ -261,9 +269,19 @@ public IndexWriterConfig toIndexWriterConfig(SolrCore core) throws IOException {
iwc.setMergeScheduler(mergeScheduler);
iwc.setInfoStream(infoStream);
+ if (indexSort != null) {
+ SortSpec indexSortSpec = SortSpecParsing.parseSortSpec(indexSort, schema);
+ iwc.setIndexSort(indexSortSpec.getSort());
+ }
+
if (mergePolicy instanceof SortingMergePolicy) {
- Sort indexSort = ((SortingMergePolicy) mergePolicy).getSort();
- iwc.setIndexSort(indexSort);
+ Sort mergeSort = ((SortingMergePolicy) mergePolicy).getSort();
+ Sort indexSort = iwc.getIndexSort();
+ if (indexSort != null && !indexSort.equals(mergeSort)) {
+ log.warn("indexSort={} differs from mergePolicySort={}", indexSort, mergeSort);
Review comment:
Perhaps clarify that `indexSort` is the one picked?
##########
File path: solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java
##########
@@ -146,6 +146,7 @@ public void testSortingMPSolrIndexConfigCreation() throws Exception {
final Sort expected = new Sort(new SortField(expectedFieldName, expectedFieldType, expectedFieldSortDescending));
final Sort actual = sortingMergePolicy.getSort();
assertEquals("SortingMergePolicy.getSort", expected, actual);
+ assertEquals("indexSort", expected, iwc.getIndexSort());
Review comment:
Let's create a new test method for the new config option, which will survive removal of SMP. Instead of creating yet another solrconfig-xyz.xml I think it should be possible to let the test invoke config-api to set the new setting.
--
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] cpoerschke commented on a change in pull request #313: SOLR-13681: make Lucene's index sorting directly configurable in Solr
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #313:
URL: https://github.com/apache/solr/pull/313#discussion_r718271490
##########
File path: solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java
##########
@@ -146,6 +146,7 @@ public void testSortingMPSolrIndexConfigCreation() throws Exception {
final Sort expected = new Sort(new SortField(expectedFieldName, expectedFieldType, expectedFieldSortDescending));
final Sort actual = sortingMergePolicy.getSort();
assertEquals("SortingMergePolicy.getSort", expected, actual);
+ assertEquals("indexSort", expected, iwc.getIndexSort());
Review comment:
Yes, I would also favour not creating yet another solrconfig-xyz.xml file. Have used the config-api in tests before but from my research done so far it seems `indexConfig` is not yet supported by it, or rather 'get' works but 'set' does not?
##########
File path: solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java
##########
@@ -146,6 +146,7 @@ public void testSortingMPSolrIndexConfigCreation() throws Exception {
final Sort expected = new Sort(new SortField(expectedFieldName, expectedFieldType, expectedFieldSortDescending));
final Sort actual = sortingMergePolicy.getSort();
assertEquals("SortingMergePolicy.getSort", expected, actual);
+ assertEquals("indexSort", expected, iwc.getIndexSort());
Review comment:
Also interestingly V2 'get' works but V1 does not, for `indexConfig` but not for (say) `requestHandler`
* both work
```
curl "http://localhost:8983/solr/gettingstarted/config/requestHandler"
curl "http://localhost:8983/api/collections/gettingstarted/config/requestHandler"
```
* latter works, former does not
```
curl "http://localhost:8983/solr/gettingstarted/config/indexConfig"
curl "http://localhost:8983/api/collections/gettingstarted/config/indexConfig"
```
https://solr.apache.org/guide/8_10/config-api.html#retrieving-the-config lists `indexConfig` as available top-level section.
```
$ curl "http://localhost:8983/solr/gettingstarted/config/indexConfig"
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8"/>
<title>Error 404 Not Found</title>
</head>
<body><h2>HTTP ERROR 404 Not Found</h2>
<table>
<tr><th>URI:</th><td>/solr/gettingstarted/config/indexConfig</td></tr>
<tr><th>STATUS:</th><td>404</td></tr>
<tr><th>MESSAGE:</th><td>Not Found</td></tr>
<tr><th>SERVLET:</th><td>default</td></tr>
</table>
</body>
</html>
```
##########
File path: solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java
##########
@@ -146,6 +146,7 @@ public void testSortingMPSolrIndexConfigCreation() throws Exception {
final Sort expected = new Sort(new SortField(expectedFieldName, expectedFieldType, expectedFieldSortDescending));
final Sort actual = sortingMergePolicy.getSort();
assertEquals("SortingMergePolicy.getSort", expected, actual);
+ assertEquals("indexSort", expected, iwc.getIndexSort());
Review comment:
Yes, and perhaps changing some `indexConfig` parameters after collection creation would be fine but not so for others. Also the merge policy (factory) configuration is nested, not sure if config-api would easily support that.
Added a new method, modelled on the existing sorting-merge-policy one that will go away with sorting merge policy. There is existing `TestSegmentSorting` coverage w.r.t. index sorting working in a test, so it might make sense to tap into that somehow?
##########
File path: solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
##########
@@ -178,7 +178,7 @@ private Collector buildAndRunCollectorChain(QueryResult qr, Query query, Collect
if (cmd.getSegmentTerminateEarly()) {
final Sort cmdSort = cmd.getSort();
final int cmdLen = cmd.getLen();
- final Sort mergeSort = core.getSolrCoreState().getMergePolicySort();
+ final Sort mergeSort = core.getSolrCoreState().getMergePolicySort(); // TODO: need this account for indexSort also?
Review comment:
https://issues.apache.org/jira/browse/SOLR-15390 is about deprecating the `segmentTerminateEarly` flag.
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestSegmentSorting.java
##########
@@ -71,8 +71,10 @@ public void createCollection() throws Exception {
final String collectionName = testName.getMethodName();
final CloudSolrClient cloudSolrClient = cluster.getSolrClient();
+ final String solrConfigFileName = random().nextBoolean() ? "solrconfig-sortingmergepolicyfactory.xml" : "solrconfig-indexSort.xml";
+
final Map<String, String> collectionProperties = new HashMap<>();
- collectionProperties.put(CoreDescriptor.CORE_CONFIG, "solrconfig-sortingmergepolicyfactory.xml");
+ collectionProperties.put(CoreDescriptor.CORE_CONFIG, solrConfigFileName);
Review comment:
The `solrconfig-indexSort.xml` pathway here is currently failing, likely due to the `SolrIndexSearcher` TODO-annotated area of code.
--
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] cpoerschke commented on a change in pull request #313: SOLR-13681: make Lucene's index sorting directly configurable in Solr
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #313:
URL: https://github.com/apache/solr/pull/313#discussion_r718470061
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestSegmentSorting.java
##########
@@ -71,8 +71,10 @@ public void createCollection() throws Exception {
final String collectionName = testName.getMethodName();
final CloudSolrClient cloudSolrClient = cluster.getSolrClient();
+ final String solrConfigFileName = random().nextBoolean() ? "solrconfig-sortingmergepolicyfactory.xml" : "solrconfig-indexSort.xml";
+
final Map<String, String> collectionProperties = new HashMap<>();
- collectionProperties.put(CoreDescriptor.CORE_CONFIG, "solrconfig-sortingmergepolicyfactory.xml");
+ collectionProperties.put(CoreDescriptor.CORE_CONFIG, solrConfigFileName);
Review comment:
The `solrconfig-indexSort.xml` pathway here is currently failing, likely due to the `SolrIndexSearcher` TODO-annotated area of code.
--
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] janhoy commented on pull request #313: SOLR-13681: make Lucene's index sorting directly configurable in Solr
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #313:
URL: https://github.com/apache/solr/pull/313#issuecomment-949360944
Sorry for going silent on this, I'll put it on my radar for next week.
--
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] cpoerschke commented on a change in pull request #313: SOLR-13681: make Lucene's index sorting directly configurable in Solr
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #313:
URL: https://github.com/apache/solr/pull/313#discussion_r719397117
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestSegmentSorting.java
##########
@@ -71,8 +71,10 @@ public void createCollection() throws Exception {
final String collectionName = testName.getMethodName();
final CloudSolrClient cloudSolrClient = cluster.getSolrClient();
+ final String solrConfigFileName = random().nextBoolean() ? "solrconfig-sortingmergepolicyfactory.xml" : "solrconfig-indexSort.xml";
+
final Map<String, String> collectionProperties = new HashMap<>();
- collectionProperties.put(CoreDescriptor.CORE_CONFIG, "solrconfig-sortingmergepolicyfactory.xml");
+ collectionProperties.put(CoreDescriptor.CORE_CONFIG, solrConfigFileName);
Review comment:
> The `solrconfig-indexSort.xml` pathway here is currently failing, likely due to the `SolrIndexSearcher` TODO-annotated area of code.
The TODO is now done and the pathway is no longer failing then.
--
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] janhoy commented on a change in pull request #313: SOLR-13681: make Lucene's index sorting directly configurable in Solr
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #313:
URL: https://github.com/apache/solr/pull/313#discussion_r718350709
##########
File path: solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java
##########
@@ -146,6 +146,7 @@ public void testSortingMPSolrIndexConfigCreation() throws Exception {
final Sort expected = new Sort(new SortField(expectedFieldName, expectedFieldType, expectedFieldSortDescending));
final Sort actual = sortingMergePolicy.getSort();
assertEquals("SortingMergePolicy.getSort", expected, actual);
+ assertEquals("indexSort", expected, iwc.getIndexSort());
Review comment:
Ok, perhaps they are R/O because it is not supported to change these after collection creation?
In that case, a new xml file is probably better anyway.
Are you planning to validate that index sorting is working in the test, or simply that the config is set? I think perhaps a new method in the existing IndexConfig test would be enough?
--
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] cpoerschke commented on a change in pull request #313: SOLR-13681: make Lucene's index sorting directly configurable in Solr
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #313:
URL: https://github.com/apache/solr/pull/313#discussion_r718280422
##########
File path: solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java
##########
@@ -146,6 +146,7 @@ public void testSortingMPSolrIndexConfigCreation() throws Exception {
final Sort expected = new Sort(new SortField(expectedFieldName, expectedFieldType, expectedFieldSortDescending));
final Sort actual = sortingMergePolicy.getSort();
assertEquals("SortingMergePolicy.getSort", expected, actual);
+ assertEquals("indexSort", expected, iwc.getIndexSort());
Review comment:
Also interestingly V2 'get' works but V1 does not, for `indexConfig` but not for (say) `requestHandler`
* both work
```
curl "http://localhost:8983/solr/gettingstarted/config/requestHandler"
curl "http://localhost:8983/api/collections/gettingstarted/config/requestHandler"
```
* latter works, former does not
```
curl "http://localhost:8983/solr/gettingstarted/config/indexConfig"
curl "http://localhost:8983/api/collections/gettingstarted/config/indexConfig"
```
https://solr.apache.org/guide/8_10/config-api.html#retrieving-the-config lists `indexConfig` as available top-level section.
```
$ curl "http://localhost:8983/solr/gettingstarted/config/indexConfig"
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8"/>
<title>Error 404 Not Found</title>
</head>
<body><h2>HTTP ERROR 404 Not Found</h2>
<table>
<tr><th>URI:</th><td>/solr/gettingstarted/config/indexConfig</td></tr>
<tr><th>STATUS:</th><td>404</td></tr>
<tr><th>MESSAGE:</th><td>Not Found</td></tr>
<tr><th>SERVLET:</th><td>default</td></tr>
</table>
</body>
</html>
```
--
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] cpoerschke commented on pull request #313: SOLR-13681: make Lucene's index sorting directly configurable in Solr
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #313:
URL: https://github.com/apache/solr/pull/313#issuecomment-930143165
> Great.
>
> Add a CHANGES entry. I may try to re-format the RefGuide edits from the patch later...
Thanks for attaching SOLR-13681-refguide-skel.patch to the JIRA ticket!
If you wish, `Allow edits and access to secrets by maintainers` is enabled for this pull request, feel free to add documentation or code or test change commits directly to the pull request branch 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] janhoy commented on a change in pull request #313: SOLR-13681: make Lucene's index sorting directly configurable in Solr
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #313:
URL: https://github.com/apache/solr/pull/313#discussion_r717856962
##########
File path: solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java
##########
@@ -261,9 +269,19 @@ public IndexWriterConfig toIndexWriterConfig(SolrCore core) throws IOException {
iwc.setMergeScheduler(mergeScheduler);
iwc.setInfoStream(infoStream);
+ if (indexSort != null) {
+ SortSpec indexSortSpec = SortSpecParsing.parseSortSpec(indexSort, schema);
+ iwc.setIndexSort(indexSortSpec.getSort());
+ }
+
if (mergePolicy instanceof SortingMergePolicy) {
- Sort indexSort = ((SortingMergePolicy) mergePolicy).getSort();
- iwc.setIndexSort(indexSort);
+ Sort mergeSort = ((SortingMergePolicy) mergePolicy).getSort();
+ Sort indexSort = iwc.getIndexSort();
+ if (indexSort != null && !indexSort.equals(mergeSort)) {
+ log.warn("indexSort={} differs from mergePolicySort={}", indexSort, mergeSort);
Review comment:
Perhaps clarify that `indexSort` is the one picked?
##########
File path: solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java
##########
@@ -146,6 +146,7 @@ public void testSortingMPSolrIndexConfigCreation() throws Exception {
final Sort expected = new Sort(new SortField(expectedFieldName, expectedFieldType, expectedFieldSortDescending));
final Sort actual = sortingMergePolicy.getSort();
assertEquals("SortingMergePolicy.getSort", expected, actual);
+ assertEquals("indexSort", expected, iwc.getIndexSort());
Review comment:
Let's create a new test method for the new config option, which will survive removal of SMP. Instead of creating yet another solrconfig-xyz.xml I think it should be possible to let the test invoke config-api to set the new setting.
--
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] cpoerschke commented on a change in pull request #313: SOLR-13681: make Lucene's index sorting directly configurable in Solr
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #313:
URL: https://github.com/apache/solr/pull/313#discussion_r718468409
##########
File path: solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
##########
@@ -178,7 +178,7 @@ private Collector buildAndRunCollectorChain(QueryResult qr, Query query, Collect
if (cmd.getSegmentTerminateEarly()) {
final Sort cmdSort = cmd.getSort();
final int cmdLen = cmd.getLen();
- final Sort mergeSort = core.getSolrCoreState().getMergePolicySort();
+ final Sort mergeSort = core.getSolrCoreState().getMergePolicySort(); // TODO: need this account for indexSort also?
Review comment:
https://issues.apache.org/jira/browse/SOLR-15390 is about deprecating the `segmentTerminateEarly` flag.
--
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] janhoy commented on a change in pull request #313: SOLR-13681: make Lucene's index sorting directly configurable in Solr
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #313:
URL: https://github.com/apache/solr/pull/313#discussion_r718350709
##########
File path: solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java
##########
@@ -146,6 +146,7 @@ public void testSortingMPSolrIndexConfigCreation() throws Exception {
final Sort expected = new Sort(new SortField(expectedFieldName, expectedFieldType, expectedFieldSortDescending));
final Sort actual = sortingMergePolicy.getSort();
assertEquals("SortingMergePolicy.getSort", expected, actual);
+ assertEquals("indexSort", expected, iwc.getIndexSort());
Review comment:
Ok, perhaps they are R/O because it is not supported to change these after collection creation?
In that case, a new xml file is probably better anyway.
Are you planning to validate that index sorting is working in the test, or simply that the config is set? I think perhaps a new method in the existing IndexConfig test would be enough?
--
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