SOLR-17982: setForcedDistrib & SearchHandler refactor#3931
SOLR-17982: setForcedDistrib & SearchHandler refactor#3931dsmiley merged 1 commit intoapache:mainfrom
Conversation
Provide an internal API to force distributed search (even when one shard). Also, refactor/reorganize SearchHandler for clarity & extensibility.
| } | ||
|
|
||
| @Override | ||
| public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception { |
There was a problem hiding this comment.
Moved this up high and moved some other methods to read in a more natural ordering in breadth-first way from top to bottom.
| boolean isZkAware = req.getCoreContainer().isZooKeeperAware(); | ||
| boolean isDistrib = req.getParams().getBool(DISTRIB, isZkAware); | ||
| protected boolean isDistrib(SolrQueryRequest req, ResponseBuilder rb) { | ||
| boolean theDefault = req.getCoreContainer().isZooKeeperAware() || rb.isForcedDistrib(); |
There was a problem hiding this comment.
This change allows a component using setForcedDistrib to also be a condition for distrib becoming true, which is relevant in standalone. But as I look at this again... I now question the decision. Arguably, it's nicer to keep the story of standalone consistent to be distrib is by default false in standalone mode and not in SolrCloud. No exceptions. Such components which can only work in a distributed mode could throw an exception 400, saying distrib must be enabled (since that's how standalone mode works). To better support that, isDistrib could be initialized first thing in handleRequestBody. WDYT?
There was a problem hiding this comment.
I'm fine with it as it is here. It's an invisible matter to a user -- conceptually the request remains not distributed in the sense that no other server is involved processing the request.
| } | ||
| } | ||
|
|
||
| private void processComponents( |
There was a problem hiding this comment.
method method was way too long and clearly had 2 implementations
| final ModifiableSolrParams params = new ModifiableSolrParams(p); | ||
|
|
||
| // TODO: look into why passing true causes fails | ||
| params.set("distrib", "false"); |
There was a problem hiding this comment.
I didn't think an explicit value should have been passed to the control client. I made this change to help the tests in the other PR work when standalone was being tested, in which distrib has a different default based on setForcedDistrib
true makes no sense here to me -- I don't get the TODO above it.
But if we don't accept the change in the first rev of this PR that changes the default distrib value, then maybe we don't need this either. This could go in the other PR.
|
I'm going to merge this in a couple days or so. I tagged you @cpoerschke as I thought you might find the extensibility in SearchHandler interesting/useful (as you have worked on this in the past maybe) but a refactoring like this is hard to review. |
Provide an internal API to force distributed search (even when one shard). Also, refactor/reorganize SearchHandler for clarity & extensibility. (cherry picked from commit 5c9cab2)
Provide an internal API to force distributed search (even when one shard). (cherry picked from commit 5c9cab2)
1 participant
Provide an internal API to force distributed search (even when one shard). Also, refactor/reorganize SearchHandler for clarity & extensibility.
https://issues.apache.org/jira/browse/SOLR-17982
(split off from SOLR-17319 / #3418 )
CC @ercsonusharma