dsmiley commented on code in PR #4276:
URL: https://github.com/apache/solr/pull/4276#discussion_r3107629640
##########
solr/core/src/test/org/apache/solr/handler/component/CombinedQuerySolrCloudTest.java:
##########
@@ -319,17 +319,17 @@ public void testQueriesWithFacetAndHighlightsCollapse()
throws Exception {
}""";
handle.put("expanded", UNORDERED);
QueryResponse rsp = query(CommonParams.JSON, jsonQuery, CommonParams.QT,
"/search");
- assertEquals(1, rsp.getResults().size());
- assertFieldValues(rsp.getResults(), id, "2!2");
+ assertEquals(3, rsp.getResults().size());
Review Comment:
Can you elaborate briefly on what the flaky issue was?
##########
solr/server/solr/configsets/sample_techproducts_configs/conf/solrconfig.xml:
##########
@@ -640,7 +640,14 @@
</lst>
</requestHandler>
- <initParams path="/update/**,/query,/select,/tvrh,/elevate,/spell,update">
+ <requestHandler name="/search" class="solr.CombinedQuerySearchHandler">
+ </requestHandler>
+
+ <searchComponent class="solr.CombinedQueryComponent" name="combined_query">
Review Comment:
I did a little experiment to see if it's *really* necessary to define this.
After all, we don't define our other components explicitly. Turns out it is,
which is a little sad. The UX was bad with an NPE so I tweaked SearchHandler
to at leave give a clear error.
##########
solr/server/solr/configsets/sample_techproducts_configs/conf/solrconfig.xml:
##########
@@ -640,7 +640,14 @@
</lst>
</requestHandler>
- <initParams path="/update/**,/query,/select,/tvrh,/elevate,/spell,update">
+ <requestHandler name="/search" class="solr.CombinedQuerySearchHandler">
Review Comment:
I understand you are touching the techproducts config because it's necessary
in order to demonstrate RRF. And you seem to have chosen "/search" because 2
other names "/select" and "/query" were chosen. If another feature were to
come along, would yet another SearchHandler need to be defined for it with an
attempt to find yet another synonym?
Two counter-proposals:
(A) Expand the scope of "/query". It's harmless to use
CombinedQuerySearchHandler without any combining. CombinedQuerySearchHandler
just means it's *capable* of combining.
(B) Use "/rrf" instead.
(C) The tutorial/documentation can show how to use our config API to
manipulate the config at runtime by adding a handler and component (even in one
request). And use a name like "/rrf". You can find this approach for the
TaggerHandler:
https://solr.apache.org/guide/solr/latest/query-guide/tagger-handler.html#create-and-configure-a-solr-collection
I prefer C. But not as-is with "/search"!
--
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]
