Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOLR-13681: make Lucene's index sorting directly configurable in Solr #313

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

cpoerschke
Copy link
Contributor

Copy link
Contributor

@janhoy janhoy left a comment

Choose a reason for hiding this comment

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

Great.

Add a CHANGES entry. I may try to re-format the RefGuide edits from the patch later...

@@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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>

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Comment on lines 178 to 181
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?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://issues.apache.org/jira/browse/SOLR-15390 is about deprecating the segmentTerminateEarly flag.

@cpoerschke
Copy link
Contributor Author

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.

@janhoy
Copy link
Contributor

janhoy commented Oct 22, 2021

Sorry for going silent on this, I'll put it on my radar for next week.

Resolved Conflicts:
	solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java
Resolved Conflicts:
	solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
	solr/core/src/test/org/apache/solr/cloud/TestSegmentSorting.java
	solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants