gerlowskija commented on code in PR #3955:
URL: https://github.com/apache/solr/pull/3955#discussion_r2640132242
##########
solr/core/src/test/org/apache/solr/cloud/BasicDistributedZkTest.java:
##########
@@ -1283,12 +1282,7 @@ private Long getNumCommits(HttpSolrClient sourceClient)
throws SolrServerExcepti
.withConnectionTimeout(15000, TimeUnit.MILLISECONDS)
.withSocketTimeout(60000, TimeUnit.MILLISECONDS)
.build()) {
- var req =
- new GenericSolrRequest(
- SolrRequest.METHOD.GET,
- "/admin/metrics",
- SolrRequest.SolrRequestType.ADMIN,
- SolrParams.of("wt", "prometheus"));
+ var req = new MetricsRequest(SolrParams.of("wt", "prometheus"));
Review Comment:
[Q] It looks like many (most?) of the MetricsRequest usages provide
`SolrParams.of("wt", "prometheus")`. I'm not super familiar with the metrics
endpoint, but if that's our norm in practice in these tests, maybe it'd make
sense to just have it be the default within MetricsRequest?
##########
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:
+1, given more time we could maybe come up with something better than
NodeSystemResponse. But better to get this merged in and we can iterate on it
going forward...
##########
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:
Another factor in where we use "v2" on the server side is that, currently,
users can disable the v2 API with a system property: `solr.api.v2.enabled`.
I think we're ready to remove that capability, so that we can start relying
on "v2" endpoints being available for inter-node calls, tooling, etc. But
while it exists it's probably brittle to have tooling, etc. use v2 endpoints.
Coincidentally, the package-store code here is one of the few places that
assuming v2 is "safe". The package-manager has now v1 APIs - it only has v2
APIs. So it's safe for us to call other v2 endpoints from the package store
code, because if v2 is disabled then all this code is unreachable anyways.
##########
solr/core/src/test/org/apache/solr/client/solrj/embedded/TestEmbeddedSolrServerAdminHandler.java:
##########
@@ -39,25 +36,8 @@ public void testPathIsAddedToContext() throws IOException,
SolrServerException {
try (final EmbeddedSolrServer server = new EmbeddedSolrServer(config,
"collection1")) {
final SystemInfoRequest info = new SystemInfoRequest();
- final NamedList<?> response = server.request(info);
- assertTrue(response.size() > 0);
- }
- }
-
- private static class SystemInfoRequest extends SolrRequest<QueryResponse> {
-
- public SystemInfoRequest() {
- super(METHOD.GET, "/admin/info/system",
SolrRequest.SolrRequestType.ADMIN);
- }
-
- @Override
- public SolrParams getParams() {
- return new ModifiableSolrParams();
- }
-
- @Override
- protected QueryResponse createResponse(final NamedList<Object> namedList) {
- return new QueryResponse();
+ final SystemInfoResponse response = info.process(server);
+ assertTrue(response.getResponse().size() > 0);
Review Comment:
[+1] Really awesome to see how much cleaner a lot of these usages look
without GenericSolrRequest!
##########
solr/solrj/src/java/org/apache/solr/client/solrj/response/SystemInfoResponse.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.api.model.NodeSystemResponse;
+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 NodeSystemResponse 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(), NodeSystemResponse.class);
+ } catch (Throwable t) {
+ log.error("Cannot convert NamedList response to API model
NodeSystemResponse", 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 NodeSystemResponse getFullResponse() {
+ return fullResponse;
+ }
+
+ public String getSolrImplVersion() {
+ return getFullResponse() != null && getFullResponse().lucene != null
+ ? getFullResponse().lucene.solrImplVersion
+ : null;
+ }
+
+ public String getSolrSpecVersion() {
+ return getFullResponse() != null && getFullResponse().lucene != null
+ ? getFullResponse().lucene.solrSpecVersion
+ : null;
+ }
+
+ public Date getJVMStartTime() {
+ return getFullResponse() != null
+ && getFullResponse().jvm != null
+ && getFullResponse().jvm.jmx != null
+ ? getFullResponse().jvm.jmx.startTime
+ : null;
+ }
+
+ public Long getJVMUpTime() {
+ return getFullResponse() != null
+ && getFullResponse().jvm != null
+ && getFullResponse().jvm.jmx != null
+ ? getFullResponse().jvm.jmx.upTimeMS
+ : null;
+ }
+
+ public String getJVMMemoryUsed() {
+ return getFullResponse() != null
+ && getFullResponse().jvm != null
+ && getFullResponse().jvm.memory != null
+ ? getFullResponse().jvm.memory.used
+ : null;
+ }
+
+ public String getJVMMemoryTtl() {
Review Comment:
[0] Could this be "Total" instead of "Ttl"? "Ttl" could reasonably be
interpreted as an abbreviation of "time-to-live"
Also, what is the "unit" being returned here. Is it the number of bytes?
kb? gb? Is it a human readable string that includes the unit in it? Might be
worth choosing a name that indicates the answer, so callers don't have to dig
through the implementation themselves to find out. e.g.
`getHumanReadableJvmMemoryTotal` or `getJvmMemoryTotalBytes` or something
similar.
##########
solr/core/src/test/org/apache/solr/handler/admin/AdminHandlersProxyTest.java:
##########
@@ -91,22 +81,16 @@ 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);
try {
- rsp = req.process(solrClient, null);
+ SystemInfoResponse rsp = req.process(solrClient, null);
+ NamedList<Object> nl = rsp.getResponse();
Review Comment:
[0] We could get rid of some ugly casting if we changed these assertions to
work on the NodeSystemResponse typed object, rather than inspecting the
NamedList. Unless there's a particular reason we're working with NL here?
##########
solr/api/src/java/org/apache/solr/client/api/model/NodeSystemResponse.java:
##########
@@ -0,0 +1,132 @@
+/*
+ * 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.api.model;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.OptBoolean;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+
+/** Response from /node/system */
+public class NodeSystemResponse extends SolrJerseyResponse {
+
+ @JsonProperty public String mode;
+ @JsonProperty public String zkHost;
+
+ @JsonProperty("solr_home")
+ public String solrHome;
+
+ @JsonProperty("core_root")
+ public String coreRoot;
+
+ @JsonProperty(isRequired = OptBoolean.FALSE)
Review Comment:
[Q] I'm a little confused by these `isRequired = OptBoolean.FALSE` bits. My
understanding was that these annotations defaulted to treat properties as
"non-required" unless otherwise specified. Is there a purpose to agreeing with
the default so explicitly?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/SystemInfoRequest.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.request;
+
+import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.response.SystemInfoResponse;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.common.util.NamedList;
+
+/** Class to get a system info response. */
+public class SystemInfoRequest extends SolrRequest<SystemInfoResponse> {
+
+ private static final long serialVersionUID = 1L;
+
+ private final SolrParams params;
+
+ /** Request to "/admin/info/system" by default, without params. */
+ public SystemInfoRequest() {
+ // TODO: support V2 by default. Requires refactoring throughout the CLI
tools, at least
+ this(CommonParams.SYSTEM_INFO_PATH);
+ }
+
+ /**
+ * @param path the HTTP path to use for this request. Supports V1
"/admin/info/system" (default)
+ * or V2 "/node/system"
Review Comment:
[0] I'm all for adding v2 API coverage to SolrJ, but I wonder whether in
this specific case it's better to stick to only supporting v1.
Generally we've been leaning on code-generation to create SolrRequest impl's
for our v2 APIs. We don't have that for "/node/system" just yet, true. But
the `NodeSystemResponse` model class added in this PR is actually most of the
work to getting us there! So IMO it feels very do-able to have coverage for
the v2 API soon in a way that's more in line with our long-term plan around
these APIs.
Skipping v2 support in this class keeps us out of the awkward position later
on of having to deprecate this ctor and shift usage over to using the generated
v2 SolrRequest. What do you guys think?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/response/SystemInfoResponse.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.api.model.NodeSystemResponse;
+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 NodeSystemResponse 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(), NodeSystemResponse.class);
+ } catch (Throwable t) {
+ log.error("Cannot convert NamedList response to API model
NodeSystemResponse", 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 NodeSystemResponse getFullResponse() {
+ return fullResponse;
+ }
+
+ public String getSolrImplVersion() {
+ return getFullResponse() != null && getFullResponse().lucene != null
+ ? getFullResponse().lucene.solrImplVersion
+ : null;
+ }
+
+ public String getSolrSpecVersion() {
+ return getFullResponse() != null && getFullResponse().lucene != null
+ ? getFullResponse().lucene.solrSpecVersion
+ : null;
+ }
+
+ public Date getJVMStartTime() {
+ return getFullResponse() != null
+ && getFullResponse().jvm != null
+ && getFullResponse().jvm.jmx != null
+ ? getFullResponse().jvm.jmx.startTime
+ : null;
+ }
+
+ public Long getJVMUpTime() {
Review Comment:
[Q] What is the unit being returned here? seconds? milliseconds? Might be
nice to indicate that in the method name to save callers from needing to dig
into the implementation to discover for themselves. e.g. `getJvmUpTimeSeconds`.
--
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]
