-
Notifications
You must be signed in to change notification settings - Fork 798
SOLR-17136: Replace most uses of GenericSolrRequest in Solr code #3955
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 15 commits
80e2582
226448b
f79c5ae
2470367
eec0643
e219435
74df237
0c11fc1
712ce54
955fc46
f09182d
bd60a18
d335770
5b106e7
eae6b7a
8c6c75d
be9e779
edc6b23
74325ac
a8451d6
4507494
a6beb5c
0c65369
af10b78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| title: Replace most uses of GenericSolrRequest in Solr code | ||
| type: changed | ||
| authors: | ||
| - name: Isabelle Giguère | ||
| links: | ||
| - name: SOLR-17136 | ||
| url: https://issues.apache.org/jira/browse/SOLR-17136 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| public String environment; | ||
|
|
||
| @JsonProperty(value = "environment_label", isRequired = OptBoolean.FALSE) | ||
| public String environmentLabel; | ||
|
|
||
| @JsonProperty(value = "environment_color", isRequired = OptBoolean.FALSE) | ||
| public String environmentColor; | ||
|
|
||
| @JsonProperty public String node; | ||
| @JsonProperty public Lucene lucene; | ||
| @JsonProperty public JVM jvm; | ||
| @JsonProperty public Security security; | ||
| @JsonProperty public GPU gpu; | ||
| @JsonProperty public Map<String, String> system; | ||
|
|
||
| /** /node/system/security */ | ||
| public static class Security { | ||
| @JsonProperty public boolean tls; | ||
| @JsonProperty public String authenticationPlugin; | ||
| @JsonProperty public String authorizationPlugin; | ||
| @JsonProperty public String username; | ||
| @JsonProperty public List<String> roles; | ||
| @JsonProperty public List<String> permissions; | ||
| } | ||
|
|
||
| /** /node/system/lucene */ | ||
| public static class Lucene { | ||
| @JsonProperty("solr-spec-version") | ||
| public String solrSpecVersion; | ||
|
|
||
| @JsonProperty("solr-impl-version") | ||
| public String solrImplVersion; | ||
|
|
||
| @JsonProperty("lucene-spec-version") | ||
| public String luceneSpecVersion; | ||
|
|
||
| @JsonProperty("lucene-impl-version") | ||
| public String luceneImplVersion; | ||
| } | ||
|
|
||
| /** /node/system/jvm */ | ||
| public static class JVM extends Vendor { | ||
| @JsonProperty public int processors; | ||
| @JsonProperty public Vendor jre; | ||
| @JsonProperty public Vendor spec; | ||
| @JsonProperty public Vendor vm; | ||
| @JsonProperty public JvmJmx jmx; | ||
| @JsonProperty public JvmMemory memory; | ||
| } | ||
|
|
||
| public static class JvmMemory { | ||
| @JsonProperty public String free; | ||
| @JsonProperty public String total; | ||
| @JsonProperty public String max; | ||
| @JsonProperty public String used; | ||
| @JsonProperty public JvmMemoryRaw raw; | ||
| } | ||
|
|
||
| public static class JvmMemoryRaw extends MemoryRaw { | ||
| @JsonProperty public long max; | ||
|
|
||
| @JsonProperty("used%") | ||
| public double usedPercent; | ||
| } | ||
|
|
||
| public static class MemoryRaw { | ||
| @JsonProperty public long free; | ||
| @JsonProperty public long total; | ||
| @JsonProperty public long used; | ||
| } | ||
|
|
||
| public static class Vendor { | ||
| @JsonProperty(isRequired = OptBoolean.FALSE) | ||
| public String name; | ||
|
|
||
| @JsonProperty(isRequired = OptBoolean.FALSE) | ||
| public String vendor; | ||
|
|
||
| @JsonProperty public String version; | ||
| } | ||
|
|
||
| public static class JvmJmx { | ||
| @JsonProperty public String classpath; | ||
| @JsonProperty public Date startTime; | ||
| @JsonProperty public long upTimeMS; | ||
| @JsonProperty public List<String> commandLineArgs; | ||
| } | ||
|
|
||
| public static class GPU { | ||
| @JsonProperty public boolean available; | ||
| @JsonProperty public long count; | ||
| @JsonProperty public MemoryRaw memory; | ||
| @JsonProperty Map<String, Object> devices; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,6 @@ | |
|
|
||
| import static org.apache.solr.common.SolrException.ErrorCode.FORBIDDEN; | ||
| import static org.apache.solr.common.SolrException.ErrorCode.UNAUTHORIZED; | ||
| import static org.apache.solr.common.params.CommonParams.SYSTEM_INFO_PATH; | ||
|
|
||
| import java.io.IOException; | ||
| import java.net.SocketException; | ||
|
|
@@ -38,18 +37,17 @@ | |
| import org.apache.commons.cli.Option; | ||
| import org.apache.commons.exec.OS; | ||
| import org.apache.solr.client.solrj.SolrClient; | ||
| import org.apache.solr.client.solrj.SolrRequest; | ||
| import org.apache.solr.client.solrj.SolrServerException; | ||
| import org.apache.solr.client.solrj.impl.CloudSolrClient; | ||
| import org.apache.solr.client.solrj.impl.SolrZkClientTimeout; | ||
| import org.apache.solr.client.solrj.jetty.HttpJettySolrClient; | ||
| import org.apache.solr.client.solrj.request.CollectionAdminRequest; | ||
| import org.apache.solr.client.solrj.request.CoresApi; | ||
| import org.apache.solr.client.solrj.request.GenericSolrRequest; | ||
| import org.apache.solr.client.solrj.request.SystemInfoRequest; | ||
| import org.apache.solr.client.solrj.response.SystemInfoResponse; | ||
| import org.apache.solr.common.SolrException; | ||
| import org.apache.solr.common.cloud.SolrZkClient; | ||
| import org.apache.solr.common.cloud.ZkStateReader; | ||
| import org.apache.solr.common.params.CommonParams; | ||
| import org.apache.solr.common.util.EnvUtils; | ||
| import org.apache.solr.common.util.NamedList; | ||
|
|
||
|
|
@@ -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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe for future, but is it odd a CLIUtils would call StatusTool for a method
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are 17 calls to CLIUtils.getZkHost(CommandLine), where we find this call to StatusTool.reportStatus(SolrClient). In a nutchell : I would not change this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes sense, thanks for digging into this. |
||
| @SuppressWarnings("unchecked") | ||
| Map<String, Object> cloud = (Map<String, Object>) status.get("cloud"); | ||
| if (cloud != null) { | ||
|
|
@@ -357,9 +350,8 @@ public static boolean safeCheckCoreExists(String solrUrl, String coreName, Strin | |
| } | ||
|
|
||
| public static boolean isCloudMode(SolrClient solrClient) throws SolrServerException, IOException { | ||
| NamedList<Object> systemInfo = | ||
| solrClient.request(new GenericSolrRequest(SolrRequest.METHOD.GET, SYSTEM_INFO_PATH)); | ||
| return "solrcloud".equals(systemInfo.get("mode")); | ||
| SystemInfoResponse sysResponse = new SystemInfoRequest().process(solrClient); | ||
| return "solrcloud".equals(sysResponse.getMode()); | ||
| } | ||
|
|
||
| public static Path getConfigSetsDir(Path solrInstallDir) { | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lovely change! |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,10 +31,9 @@ | |
| import org.apache.commons.cli.Options; | ||
| import org.apache.solr.cli.SolrProcessManager.SolrProcess; | ||
| import org.apache.solr.client.solrj.SolrClient; | ||
| import org.apache.solr.client.solrj.SolrRequest; | ||
| import org.apache.solr.client.solrj.request.CollectionAdminRequest; | ||
| import org.apache.solr.client.solrj.request.GenericSolrRequest; | ||
| import org.apache.solr.common.params.CommonParams; | ||
| import org.apache.solr.client.solrj.request.SystemInfoRequest; | ||
| import org.apache.solr.client.solrj.response.SystemInfoResponse; | ||
| import org.apache.solr.common.util.NamedList; | ||
| import org.apache.solr.common.util.URLUtil; | ||
| import org.noggit.CharArr; | ||
|
|
@@ -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<>(); | ||
epugh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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())); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LIkewise in a future pr, would be nice to figure out is it cloud or solrcloud and use a single term everywhere ;-) |
||
| } | ||
|
|
||
| return status; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Q] I'm a little confused by these
isRequired = OptBoolean.FALSEbits. 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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I was being too cautious. I can remove the explicit setting.