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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
