epugh commented on code in PR #3955:
URL: https://github.com/apache/solr/pull/3955#discussion_r2630794519
##########
solr/core/src/java/org/apache/solr/cli/HealthcheckTool.java:
##########
Review Comment:
Lovely change!
##########
solr/core/src/java/org/apache/solr/cli/StatusTool.java:
##########
@@ -292,42 +291,26 @@ public Map<String, Object> waitToSeeSolrUp(
public Map<String, Object> getStatus(String solrUrl, String credentials)
throws Exception {
try (var solrClient = CLIUtils.getSolrClient(solrUrl, credentials)) {
- return getStatus(solrClient);
+ Map<String, Object> status = reportStatus(solrClient);
+ return status;
}
}
- public Map<String, Object> getStatus(SolrClient solrClient) throws Exception
{
- Map<String, Object> status;
-
- NamedList<Object> systemInfo =
- solrClient.request(
- new GenericSolrRequest(SolrRequest.METHOD.GET,
CommonParams.SYSTEM_INFO_PATH));
- // convert raw JSON into user-friendly output
- status = reportStatus(systemInfo, solrClient);
-
- return status;
- }
-
- public static Map<String, Object> reportStatus(NamedList<Object> info,
SolrClient solrClient)
- throws Exception {
+ public static Map<String, Object> reportStatus(SolrClient solrClient) throws
Exception {
Map<String, Object> status = new LinkedHashMap<>();
Review Comment:
Does having this status map help us? If this map only ever has data sourced
from SystemInfoResponse, a thought could be to just pass back a
SystemInfoResponse object? I don't know if that is a "bad" thing from the
perspective that the http transport model object is bleeding into the app...
I hate passing maps of magic values around in a strongly type language like
java!
##########
solr/solrj/src/java/org/apache/solr/client/solrj/response/SystemInfoResponse.java:
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.client.solrj.response;
+
+import java.lang.invoke.MethodHandles;
+import java.util.Date;
+import org.apache.solr.client.solrj.request.json.JacksonContentWriter;
+import org.apache.solr.common.util.NamedList;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** This class holds the response from V1 "/admin/info/system" or V2
"/node/system" */
+public class SystemInfoResponse extends SolrResponseBase {
+
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private static final long serialVersionUID = 1L;
+
+ private org.apache.solr.client.api.model.SystemInfoResponse fullResponse;
Review Comment:
the import path being full here?
##########
solr/core/src/java/org/apache/solr/packagemanager/RepositoryManager.java:
##########
@@ -142,14 +143,15 @@ public void addRepository(String repoName, String uri)
throws Exception {
public void addKey(byte[] key, String destinationKeyFilename) throws
Exception {
// get solr_home directory from info servlet
- NamedList<Object> systemInfo =
- solrClient.request(
- new GenericSolrRequest(SolrRequest.METHOD.GET, "/solr" +
SYSTEM_INFO_PATH));
- String solrHome = (String) systemInfo.get("solr_home");
+ // This method is only called from PackageTool ("add-repo", or "add-key"),
where the Solr URL is
+ // normalized to remove the /solr path part
+ // So might as well ping the V2 API "/node/system" instead.
+ // Otherwise, this SystemInfoRequest ctr would need to set the full
/solr/admin/info/system path
Review Comment:
"ctr"??? maybe spell it out.
##########
solr/core/src/java/org/apache/solr/cli/CreateTool.java:
##########
@@ -157,14 +156,10 @@ protected void createCore(CommandLine cli, SolrClient
solrClient) throws Excepti
}
printDefaultConfigsetWarningIfNecessary(cli);
- String coreRootDirectory; // usually same as solr home, but not always
-
- NamedList<?> systemInfo =
- solrClient.request(
- new GenericSolrRequest(SolrRequest.METHOD.GET,
CommonParams.SYSTEM_INFO_PATH));
-
- // convert raw JSON into user-friendly output
- coreRootDirectory = (String) systemInfo.get("core_root");
+ SystemInfoResponse sysResponse = (new
SystemInfoRequest()).process(solrClient);
+ // usually same as solr home, but not always
+ String coreRootDirectory =
Review Comment:
can core_root ever be null? do we know we need this check?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/response/SystemInfoResponse.java:
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.client.solrj.response;
+
+import java.lang.invoke.MethodHandles;
+import java.util.Date;
+import org.apache.solr.client.solrj.request.json.JacksonContentWriter;
+import org.apache.solr.common.util.NamedList;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** This class holds the response from V1 "/admin/info/system" or V2
"/node/system" */
+public class SystemInfoResponse extends SolrResponseBase {
+
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private static final long serialVersionUID = 1L;
+
+ private org.apache.solr.client.api.model.SystemInfoResponse fullResponse;
+
+ public SystemInfoResponse(NamedList<Object> namedList) {
+ if (namedList == null) throw new IllegalArgumentException("Null NamedList
is not allowed.");
+ setResponse(namedList);
+ }
+
+ @Override
+ public void setResponse(NamedList<Object> response) {
+ if (getResponse() == null) super.setResponse(response);
+ if (fullResponse == null) {
+ try {
+ fullResponse =
+ JacksonContentWriter.DEFAULT_MAPPER.convertValue(
+ getResponse(),
org.apache.solr.client.api.model.SystemInfoResponse.class);
+ } catch (Throwable t) {
+ log.error("Cannot convert NamedList response to API model
SystemInfoResponse", t);
+ }
+ }
+ }
+
+ public String getMode() {
+ return getFullResponse().mode;
+ }
+
+ public String getZkHost() {
+ return getFullResponse().zkHost;
+ }
+
+ public String getSolrHome() {
+ return getFullResponse().solrHome;
+ }
+
+ public String getCoreRoot() {
+ return getFullResponse().coreRoot;
+ }
+
+ public String getNode() {
+ return getFullResponse().node;
+ }
+
+ public org.apache.solr.client.api.model.SystemInfoResponse getFullResponse()
{
+ return fullResponse;
+ }
+
+ // *************************
+ // Shortcuts
Review Comment:
is this a pattern in other similar classes? Mostly my own curiosity!
##########
solr/core/src/test/org/apache/solr/handler/admin/AdminHandlersProxyTest.java:
##########
@@ -91,13 +81,8 @@ public void proxySystemInfoHandlerOneNode() {
nodes.forEach(
node -> {
MapSolrParams params = new
MapSolrParams(Collections.singletonMap("nodes", node));
- GenericSolrRequest req =
- new GenericSolrRequest(
- SolrRequest.METHOD.GET,
- "/admin/info/system",
- SolrRequest.SolrRequestType.ADMIN,
- params);
- SimpleSolrResponse rsp = null;
+ SystemInfoRequest req = new SystemInfoRequest(params);
+ SystemInfoResponse rsp = null;
Review Comment:
check your ide, I think in geneal we don't do the `= null` pattern when
declaring an empty variable as it is already null.
##########
solr/core/src/java/org/apache/solr/packagemanager/RepositoryManager.java:
##########
@@ -142,14 +143,15 @@ public void addRepository(String repoName, String uri)
throws Exception {
public void addKey(byte[] key, String destinationKeyFilename) throws
Exception {
// get solr_home directory from info servlet
- NamedList<Object> systemInfo =
- solrClient.request(
- new GenericSolrRequest(SolrRequest.METHOD.GET, "/solr" +
SYSTEM_INFO_PATH));
- String solrHome = (String) systemInfo.get("solr_home");
+ // This method is only called from PackageTool ("add-repo", or "add-key"),
where the Solr URL is
+ // normalized to remove the /solr path part
+ // So might as well ping the V2 API "/node/system" instead.
+ // Otherwise, this SystemInfoRequest ctr would need to set the full
/solr/admin/info/system path
+ SystemInfoResponse sysResponse = new
SystemInfoRequest("/node/system").process(solrClient);
Review Comment:
humm.... In general we should just use V2 api's! Over time in Solr 10 x
we will migrate internal tooling to using V2 apis, which will open the door to
deprecate and remove V1 equivalents. If I am understanding here, you are
choosing to use V2 in this one use case. Could we just use V2 everywhere....
##########
solr/core/src/java/org/apache/solr/cli/StatusTool.java:
##########
@@ -292,42 +291,26 @@ public Map<String, Object> waitToSeeSolrUp(
public Map<String, Object> getStatus(String solrUrl, String credentials)
throws Exception {
try (var solrClient = CLIUtils.getSolrClient(solrUrl, credentials)) {
- return getStatus(solrClient);
+ Map<String, Object> status = reportStatus(solrClient);
+ return status;
}
}
- public Map<String, Object> getStatus(SolrClient solrClient) throws Exception
{
- Map<String, Object> status;
-
- NamedList<Object> systemInfo =
- solrClient.request(
- new GenericSolrRequest(SolrRequest.METHOD.GET,
CommonParams.SYSTEM_INFO_PATH));
- // convert raw JSON into user-friendly output
- status = reportStatus(systemInfo, solrClient);
-
- return status;
- }
-
- public static Map<String, Object> reportStatus(NamedList<Object> info,
SolrClient solrClient)
- throws Exception {
+ public static Map<String, Object> reportStatus(SolrClient solrClient) throws
Exception {
Map<String, Object> status = new LinkedHashMap<>();
+ SystemInfoResponse sysResponse = (new
SystemInfoRequest()).process(solrClient);
+ status.put("solr_home", sysResponse.getSolrHome() != null ?
sysResponse.getSolrHome() : "?");
+ status.put("version", sysResponse.getSolrImplVersion());
- String solrHome = (String) info.get("solr_home");
- status.put("solr_home", solrHome != null ? solrHome : "?");
- status.put("version", info._getStr(List.of("lucene", "solr-impl-version"),
null));
- status.put("startTime", info._getStr(List.of("jvm", "jmx", "startTime"),
null));
- status.put("uptime", SolrCLI.uptime((Long) info._get(List.of("jvm", "jmx",
"upTimeMS"), null)));
+ status.put("startTime", sysResponse.getJVMStartTime());
+ status.put("uptime", sysResponse.getJVMUpTime());
- String usedMemory = info._getStr(List.of("jvm", "memory", "used"), null);
- String totalMemory = info._getStr(List.of("jvm", "memory", "total"), null);
- status.put("memory", usedMemory + " of " + totalMemory);
+ status.put("memory", sysResponse.getJVMMemoryUsed() + " of " +
sysResponse.getJVMMemoryTtl());
// if this is a Solr in solrcloud mode, gather some basic cluster info
- if ("solrcloud".equals(info.get("mode"))) {
- String zkHost = (String) info.get("zkHost");
- status.put("cloud", getCloudStatus(solrClient, zkHost));
+ if ("solrcloud".equals(sysResponse.getMode())) {
+ status.put("cloud", getCloudStatus(solrClient, sysResponse.getZkHost()));
Review Comment:
LIkewise in a future pr, would be nice to figure out _is it cloud or
solrcloud_ and use a single term everywhere ;-)
##########
solr/core/src/java/org/apache/solr/cli/StatusTool.java:
##########
@@ -292,42 +291,26 @@ public Map<String, Object> waitToSeeSolrUp(
public Map<String, Object> getStatus(String solrUrl, String credentials)
throws Exception {
try (var solrClient = CLIUtils.getSolrClient(solrUrl, credentials)) {
- return getStatus(solrClient);
+ Map<String, Object> status = reportStatus(solrClient);
+ return status;
}
}
- public Map<String, Object> getStatus(SolrClient solrClient) throws Exception
{
- Map<String, Object> status;
-
- NamedList<Object> systemInfo =
- solrClient.request(
- new GenericSolrRequest(SolrRequest.METHOD.GET,
CommonParams.SYSTEM_INFO_PATH));
- // convert raw JSON into user-friendly output
- status = reportStatus(systemInfo, solrClient);
-
- return status;
- }
-
- public static Map<String, Object> reportStatus(NamedList<Object> info,
SolrClient solrClient)
- throws Exception {
+ public static Map<String, Object> reportStatus(SolrClient solrClient) throws
Exception {
Map<String, Object> status = new LinkedHashMap<>();
Review Comment:
also, this could be fixed in a seperate PR after this to more clearly see
the impact!
##########
solr/core/src/java/org/apache/solr/cli/CLIUtils.java:
##########
@@ -249,12 +247,7 @@ public static String getZkHost(CommandLine cli) throws
Exception {
try (SolrClient solrClient = getSolrClient(cli)) {
// hit Solr to get system info
- NamedList<Object> systemInfo =
- solrClient.request(
- new GenericSolrRequest(SolrRequest.METHOD.GET,
CommonParams.SYSTEM_INFO_PATH));
-
- // convert raw JSON into user-friendly output
- Map<String, Object> status = StatusTool.reportStatus(systemInfo,
solrClient);
+ Map<String, Object> status = StatusTool.reportStatus(solrClient);
Review Comment:
maybe for future, but is it odd a CLIUtils would call StatusTool for a
method `reportStatus`? Should it be moved to CLIUtils instead and the
StatusTool should call CLIUtils.reportStatus?
--
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]
