dsmiley commented on code in PR #3931:
URL: https://github.com/apache/solr/pull/3931#discussion_r2600935781
##########
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java:
##########
@@ -241,6 +241,30 @@ public void changed(SolrPackageLoader.SolrPackage pkg, Ctx
ctx) {
}
}
+ @Override
+ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp)
throws Exception {
Review 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.
##########
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java:
##########
@@ -304,9 +328,9 @@ public List<SearchComponent> getComponents() {
return result;
}
- private boolean isDistrib(SolrQueryRequest req) {
- 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();
Review 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?
##########
solr/test-framework/src/java/org/apache/solr/BaseDistributedSearchTestCase.java:
##########
@@ -624,19 +624,18 @@ protected QueryResponse query(boolean setDistribParams,
Object[] q) throws Excep
/** Returns the distributed QueryResponse */
protected QueryResponse query(boolean setDistribParams, SolrParams p) throws
Exception {
+ if (p.get("distrib") != null) {
+ throw new IllegalArgumentException("don't pass distrib param");
+ }
- final ModifiableSolrParams params = new ModifiableSolrParams(p);
-
- // TODO: look into why passing true causes fails
- params.set("distrib", "false");
Review 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.
##########
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java:
##########
@@ -395,354 +419,363 @@ protected boolean checkCircuitBreakers(
return false;
}
- @Override
- public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp)
throws Exception {
- List<SearchComponent> components = getComponents();
- ResponseBuilder rb = newResponseBuilder(req, rsp, components);
- if (rb.requestInfo != null) {
- rb.requestInfo.setResponseBuilder(rb);
- }
-
- rb.isDistrib = isDistrib(req);
- tagRequestWithRequestId(rb);
+ protected void processComponents(
+ SolrQueryRequest req,
+ SolrQueryResponse rsp,
+ ResponseBuilder rb,
+ List<SearchComponent> components)
+ throws IOException {
+ final RTimerTree timer = rb.isDebug() ? req.getRequestTimer() : null;
+ // return a ShardHandler only if doing distributed search (equivalent to
rb.isDistrib)
+ final ShardHandler shardHandler = getAndPrepShardHandler(req, rb);
- boolean dbg = req.getParams().getBool(CommonParams.DEBUG_QUERY, false);
- rb.setDebug(dbg);
- if (dbg == false) { // if it's true, we are doing everything anyway.
-
SolrPluginUtils.getDebugInterests(req.getParams().getParams(CommonParams.DEBUG),
rb);
- }
+ if (!prepareComponents(req, rb, timer, components)) return;
- final RTimerTree timer = rb.isDebug() ? req.getRequestTimer() : null;
+ postPrepareComponents(rb);
- if (checkCircuitBreakers(req, rsp, rb)) {
- return; // Circuit breaker tripped, return immediately
+ if (shardHandler == null) {
+ processComponentsLocal(req, rsp, rb, timer, components);
+ } else {
+ processComponentsDistrib(req, rsp, rb, timer, components, shardHandler);
}
+ }
- processComponents(req, rsp, rb, timer, components);
-
- // SOLR-5550: still provide shards.info if requested even for a
short-circuited distrib request
- if (!rb.isDistrib
- && req.getParams().getBool(ShardParams.SHARDS_INFO, false)
- && rb.shortCircuitedURL != null) {
- NamedList<Object> shardInfo = new SimpleOrderedMap<>();
- SimpleOrderedMap<Object> nl = new SimpleOrderedMap<>();
- if (rsp.getException() != null) {
- Throwable cause = rsp.getException();
- if (cause instanceof SolrServerException) {
- cause = ((SolrServerException) cause).getRootCause();
- } else {
- if (cause.getCause() != null) {
- cause = cause.getCause();
- }
+ private static boolean prepareComponents(
+ SolrQueryRequest req, ResponseBuilder rb, RTimerTree timer,
List<SearchComponent> components)
+ throws IOException {
+ if (timer == null) {
+ // non-debugging prepare phase
+ for (SearchComponent component : components) {
+ if (checkLimitsBefore(component, "prepare", rb.req, rb.rsp,
components)) {
+ shortCircuitedResults(req, rb);
+ return false;
}
- nl.add("error", cause.toString());
- if (!core.getCoreContainer().hideStackTrace()) {
- StringWriter trace = new StringWriter();
- cause.printStackTrace(new PrintWriter(trace));
- nl.add("trace", trace.toString());
+ component.prepare(rb);
+ }
+ } else {
+ // debugging prepare phase
+ RTimerTree subt = timer.sub("prepare");
+ for (SearchComponent c : components) {
+ if (checkLimitsBefore(c, "prepare debug", rb.req, rb.rsp, components))
{
+ shortCircuitedResults(req, rb);
+ return false;
}
- } else if (rb.getResults() != null) {
- nl.add("numFound", rb.getResults().docList.matches());
- nl.add(
- "numFoundExact",
- rb.getResults().docList.hitCountRelation() ==
TotalHits.Relation.EQUAL_TO);
- nl.add("maxScore", rb.getResults().docList.maxScore());
+ rb.setTimer(subt.sub(c.getName()));
+ c.prepare(rb);
+ rb.getTimer().stop();
}
- nl.add("shardAddress", rb.shortCircuitedURL);
- nl.add("time", req.getRequestTimer().getTime()); // elapsed time of this
request so far
+ subt.stop();
+ }
+ return true;
+ }
- int pos = rb.shortCircuitedURL.indexOf("://");
- String shardInfoName =
- pos != -1 ? rb.shortCircuitedURL.substring(pos + 3) :
rb.shortCircuitedURL;
- shardInfo.add(shardInfoName, nl);
- rsp.getValues().add(ShardParams.SHARDS_INFO, shardInfo);
+ /**
+ * Called after {@link #prepareComponents(SolrQueryRequest, ResponseBuilder,
RTimerTree, List)}
+ */
+ protected void postPrepareComponents(ResponseBuilder rb) {
+ // Once all of our components have been prepared, check if this request
involves a SortSpec.
+ // If it does, and if our request includes a cursorMark param, then parse
& init the
+ // CursorMark state (This must happen after the prepare() of all
components, because any
+ // component may have modified the SortSpec)
+ final SortSpec spec = rb.getSortSpec();
+ final String cursorStr =
rb.req.getParams().get(CursorMarkParams.CURSOR_MARK_PARAM);
+ if (null != spec && null != cursorStr) {
+ final CursorMark cursorMark = new CursorMark(rb.req.getSchema(), spec);
+ cursorMark.parseSerializedTotem(cursorStr);
+ rb.setCursorMark(cursorMark);
}
}
- private void processComponents(
Review Comment:
method method was way too long and clearly had 2 implementations
--
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]
