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.

Sign up for GitHub

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 19 commits into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
b37f45c
git apply SOLR-13681.patch
cpoerschke Sep 28, 2021
564a66d
Merge remote-tracking branch 'origin/main' into SOLR-13681
cpoerschke Sep 28, 2021
bc39779
make it compile
cpoerschke Sep 28, 2021
e8ecd26
code review feedback: logging tweak
cpoerschke Sep 29, 2021
7514d34
add (skeleton) SolrIndexConfigIndexSortTest class
cpoerschke Sep 29, 2021
8171b27
add indexConfig fragments to TestSolrConfigHandler
cpoerschke Sep 29, 2021
33a722e
Revert "add indexConfig fragments to TestSolrConfigHandler"
cpoerschke Sep 29, 2021
2247d79
SolrIndexConfigTest: add testIndexSortSolrIndexConfigCreation as coun…
cpoerschke Sep 29, 2021
1478be7
TestSegmentSorting: solrconfig-sortingmergepolicyfactory.xml/solrconf…
cpoerschke Sep 29, 2021
14b8c49
Revert "add (skeleton) SolrIndexConfigIndexSortTest class"
cpoerschke Sep 29, 2021
7feca02
SolrIndexConfigTest: assert that indexSort is not present if not conf…
cpoerschke Sep 29, 2021
736a75b
address TODO in SolrIndexSearcher.buildAndRunCollectorChain to make T…
cpoerschke Sep 30, 2021
3920768
Merge remote-tracking branch 'origin/main' into SOLR-13681
cpoerschke Sep 30, 2021
97b1206
Merge remote-tracking branch 'origin/main' into SOLR-13681
cpoerschke Dec 22, 2021
f0889c0
adjust SolrIndexConfigTest.testIndexSortSolrIndexConfigCreation() aft…
cpoerschke Dec 22, 2021
0fd2c9f
Merge remote-tracking branch 'origin/main' into SOLR-13681
cpoerschke May 27, 2022
eae8527
./gradlew spotlessApply
cpoerschke May 27, 2022
94190d5
Merge remote-tracking branch 'origin/main' into SOLR-13681
cpoerschke May 10, 2024
eee971c
Merge remote-tracking branch 'origin/main' into SOLR-13681
cpoerschke Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@
import org.apache.solr.index.MergePolicyFactoryArgs;
import org.apache.solr.index.SortingMergePolicy;
import org.apache.solr.schema.IndexSchema;
import org.apache.solr.search.SortSpec;
import org.apache.solr.search.SortSpecParsing;
import org.apache.solr.util.SolrPluginUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -83,6 +85,7 @@ public class SolrIndexConfig implements MapSerializable {

public final int writeLockTimeout;
public final String lockType;
public final String indexSort;
public final PluginInfo mergePolicyFactoryInfo;
public final PluginInfo mergeSchedulerInfo;
public final PluginInfo metricsInfo;
Expand All @@ -102,6 +105,7 @@ private SolrIndexConfig(SolrConfig solrConfig) {
maxCommitMergeWaitMillis = -1;
writeLockTimeout = -1;
lockType = DirectoryFactory.LOCK_TYPE_NATIVE;
indexSort = null;
mergePolicyFactoryInfo = null;
mergeSchedulerInfo = null;
mergedSegmentWarmerInfo = null;
Expand Down Expand Up @@ -151,6 +155,7 @@ public SolrIndexConfig(SolrConfig solrConfig, String prefix, SolrIndexConfig def

writeLockTimeout=solrConfig.getInt(prefix+"/writeLockTimeout", def.writeLockTimeout);
lockType=solrConfig.get(prefix+"/lockType", def.lockType);
indexSort=solrConfig.get(prefix+"/indexSort", def.indexSort);

List<PluginInfo> infos = solrConfig.readPluginInfos(prefix + "/metrics", false, false);
if (infos.isEmpty()) {
Expand Down Expand Up @@ -209,6 +214,9 @@ public Map<String, Object> toMap(Map<String, Object> map) {
if (metricsInfo != null) {
map.put("metrics", metricsInfo);
}
if (indexSort != null) {
map.put("indexSort", indexSort);
}
if (mergePolicyFactoryInfo != null) {
map.put("mergePolicyFactory", mergePolicyFactoryInfo);
}
Expand Down Expand Up @@ -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);
} else {
iwc.setIndexSort(mergeSort);
}
}

iwc.setUseCompoundFile(useCompoundFile);
Expand Down
1 change: 1 addition & 0 deletions solr/core/src/test-files/solr/collection1/conf/solrconfig-sortingmergepolicyfactory.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
<str name="sort">timestamp_i_dvo desc</str>
</mergePolicyFactory>
<lockType>${solr.tests.lockType:single}</lockType>
<indexSort>timestamp_i_dvo desc</indexSort>
</indexConfig>

<requestHandler name="/select" class="solr.SearchHandler" />
Expand Down
1 change: 1 addition & 0 deletions solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?

}

public void testMergedSegmentWarmerIndexConfigCreation() throws Exception {
Expand Down