You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "janhoy (via GitHub)" <gi...@apache.org> on 2026/09/08 22:22:26 UTC
[PR] SOLR-18419: Simplify PingRequestHandler shard handling [solr]
janhoy opened a new pull request, #4890:
URL: https://github.com/apache/solr/pull/4890
This PR refactors Ping handler's handling of distributed Ping and makes it simpler, not needing to hit the ping handler on each shard, but now fans out directly to the delegate handler on each shard. Also tidy up request parameter handling vs handler configuration.
As #4874 will remove ping handler for 11.0 (main branch), this PR will land directly on `branch_10x`.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18419: Simplify PingRequestHandler shard handling [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4890:
URL: https://github.com/apache/solr/pull/4890#discussion_r3963126924
##########
solr/core/src/test/org/apache/solr/handler/PingRequestHandlerParamSetTest.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.StandardCopyOption;
+import org.apache.commons.io.file.PathUtils;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.SolrException;
+import org.junit.BeforeClass;
+
+/**
+ * The implicit {@code /admin/ping} handler declares {@code useParams=_ADMIN_PING}, so the handler
+ * it delegates to may also be configured through a paramset in {@code params.json}.
+ */
+public class PingRequestHandlerParamSetTest extends SolrTestCaseJ4 {
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ Path solrHome = createTempDir();
+ PathUtils.copyDirectory(TEST_HOME(), solrHome, StandardCopyOption.COPY_ATTRIBUTES);
+ Files.writeString(
+ solrHome.resolve("collection1").resolve("conf").resolve("params.json"),
+ "{\"params\":{\"_ADMIN_PING\":{\"_invariants_\":{\"qt\":\"/nosuchhandler\"}}}}");
Review Comment:
Multiline could be used here right? But honestly that is a nit
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18419: Simplify PingRequestHandler shard handling [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4890:
URL: https://github.com/apache/solr/pull/4890#discussion_r3963573897
##########
solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java:
##########
@@ -229,73 +232,72 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
protected void handlePing(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
- SolrParams params = req.getParams();
SolrCore core = req.getCore();
- // Get the RequestHandler
- String qt = params.get(CommonParams.QT); // optional; you get the default otherwise
+ SolrParams configParams = resolveConfiguredParams(req);
+ String qt = configParams.get(CommonParams.QT);
SolrRequestHandler handler = core.getRequestHandler(qt);
if (handler == null) {
throw new SolrException(
SolrException.ErrorCode.BAD_REQUEST, "Unknown RequestHandler (qt): " + qt);
}
-
if (handler instanceof PingRequestHandler) {
- // In case it's a query for shard, use default handler
- if (params.getBool(ShardParams.IS_SHARD, false)) {
- handler = core.getRequestHandler(null);
- ModifiableSolrParams wparams = new ModifiableSolrParams(params);
- wparams.remove(CommonParams.QT);
- req.setParams(wparams);
- } else {
- throw new SolrException(
- SolrException.ErrorCode.BAD_REQUEST,
- "Cannot execute the PingRequestHandler recursively");
- }
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST, "Cannot execute the PingRequestHandler recursively");
+ }
+
+ ModifiableSolrParams overrides = new ModifiableSolrParams();
+ boolean distrib = req.getParams().getBool(DISTRIB, false);
+ overrides.set(DISTRIB, distrib);
+ if (distrib) {
+ // target the delegate on each shard, not this ping handler
+ overrides.set(ShardParams.SHARDS_QT, qt == null ? "/select" : qt);
}
// Execute the ping query and catch any possible exception
Throwable ex = null;
-
- // In case it's a query for shard, return the result from delegated handler for distributed
- // query to merge result
- if (params.getBool(ShardParams.IS_SHARD, false)) {
- try {
- core.execute(handler, req, rsp);
- ex = rsp.getException();
- } catch (Exception e) {
- ex = e;
+ SolrQueryRequest pingReq = req.subRequest(SolrParams.wrapDefaults(overrides, configParams));
+ try {
+ SolrQueryResponse pingrsp = new SolrQueryResponse();
+ core.execute(handler, pingReq, pingrsp);
+ ex = pingrsp.getException();
+ NamedList<Object> headers = rsp.getResponseHeader();
+ if (headers != null) {
+ headers.add("zkConnected", pingrsp.getResponseHeader().get("zkConnected"));
}
- // Send an error or return
- if (ex != null) {
- throw new SolrException(
- SolrException.ErrorCode.SERVER_ERROR,
- "Ping query caused exception: " + ex.getMessage(),
- ex);
- }
- } else {
- try {
- SolrQueryResponse pingrsp = new SolrQueryResponse();
- core.execute(handler, req, pingrsp);
- ex = pingrsp.getException();
- NamedList<Object> headers = rsp.getResponseHeader();
- if (headers != null) {
- headers.add("zkConnected", pingrsp.getResponseHeader().get("zkConnected"));
- }
+ } catch (Exception e) {
+ ex = e;
+ } finally {
+ pingReq.close();
+ }
- } catch (Exception e) {
- ex = e;
- }
+ // Send an error or an 'OK' message (response code will be 200)
+ if (ex != null) {
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR,
+ "Ping query caused exception: " + ex.getMessage(),
+ ex);
+ }
- // Send an error or an 'OK' message (response code will be 200)
- if (ex != null) {
- throw new SolrException(
- SolrException.ErrorCode.SERVER_ERROR,
- "Ping query caused exception: " + ex.getMessage(),
- ex);
- }
+ rsp.add("status", "OK");
+ }
- rsp.add("status", "OK");
+ /**
+ * Resolves this handler's configured invariants, appends, defaults and {@code useParams}
+ * paramsets into a single {@link SolrParams}. The delegate handler is named by {@code qt}; a null
+ * {@code qt} means the core's default handler.
+ */
+ private SolrParams resolveConfiguredParams(SolrQueryRequest req) {
+ SolrQueryRequest configOnly = req.subRequest(new ModifiableSolrParams());
+ try {
Review Comment:
not doing try-with-resources?
##########
solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java:
##########
@@ -229,73 +232,72 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
protected void handlePing(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
- SolrParams params = req.getParams();
SolrCore core = req.getCore();
- // Get the RequestHandler
- String qt = params.get(CommonParams.QT); // optional; you get the default otherwise
+ SolrParams configParams = resolveConfiguredParams(req);
+ String qt = configParams.get(CommonParams.QT);
SolrRequestHandler handler = core.getRequestHandler(qt);
if (handler == null) {
throw new SolrException(
SolrException.ErrorCode.BAD_REQUEST, "Unknown RequestHandler (qt): " + qt);
}
-
if (handler instanceof PingRequestHandler) {
- // In case it's a query for shard, use default handler
- if (params.getBool(ShardParams.IS_SHARD, false)) {
- handler = core.getRequestHandler(null);
- ModifiableSolrParams wparams = new ModifiableSolrParams(params);
- wparams.remove(CommonParams.QT);
- req.setParams(wparams);
- } else {
- throw new SolrException(
- SolrException.ErrorCode.BAD_REQUEST,
- "Cannot execute the PingRequestHandler recursively");
- }
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST, "Cannot execute the PingRequestHandler recursively");
+ }
+
+ ModifiableSolrParams overrides = new ModifiableSolrParams();
+ boolean distrib = req.getParams().getBool(DISTRIB, false);
+ overrides.set(DISTRIB, distrib);
+ if (distrib) {
+ // target the delegate on each shard, not this ping handler
+ overrides.set(ShardParams.SHARDS_QT, qt == null ? "/select" : qt);
}
// Execute the ping query and catch any possible exception
Throwable ex = null;
-
- // In case it's a query for shard, return the result from delegated handler for distributed
- // query to merge result
- if (params.getBool(ShardParams.IS_SHARD, false)) {
- try {
- core.execute(handler, req, rsp);
- ex = rsp.getException();
- } catch (Exception e) {
- ex = e;
+ SolrQueryRequest pingReq = req.subRequest(SolrParams.wrapDefaults(overrides, configParams));
+ try {
Review Comment:
lets do try-with-resources while you're editing this line
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18419: Simplify PingRequestHandler shard handling [solr]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy merged PR #4890:
URL: https://github.com/apache/solr/pull/4890
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18419: Simplify PingRequestHandler shard handling [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4890:
URL: https://github.com/apache/solr/pull/4890#discussion_r3963567999
##########
solr/core/src/test/org/apache/solr/handler/PingRequestHandlerParamSetTest.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.StandardCopyOption;
+import org.apache.commons.io.file.PathUtils;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.SolrException;
+import org.junit.BeforeClass;
+
+/**
+ * The implicit {@code /admin/ping} handler declares {@code useParams=_ADMIN_PING}, so the handler
+ * it delegates to may also be configured through a paramset in {@code params.json}.
+ */
+public class PingRequestHandlerParamSetTest extends SolrTestCaseJ4 {
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ Path solrHome = createTempDir();
+ PathUtils.copyDirectory(TEST_HOME(), solrHome, StandardCopyOption.COPY_ATTRIBUTES);
+ Files.writeString(
+ solrHome.resolve("collection1").resolve("conf").resolve("params.json"),
+ "{\"params\":{\"_ADMIN_PING\":{\"_invariants_\":{\"qt\":\"/nosuchhandler\"}}}}");
Review Comment:
it's just one line.... so IMO not worth it.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org