dsmiley commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1159167924
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java:
##########
@@ -228,20 +229,20 @@ public List<SolrPackageInstance>
fetchInstalledPackageInstances() throws SolrExc
public Map<String, SolrPackageInstance> getPackagesDeployed(String
collection) {
Map<String, String> packages = null;
try {
- NavigableObject result =
- (NavigableObject)
- Utils.executeGET(
- solrClient.getHttpClient(),
- solrBaseUrl
- + PackageUtils.getCollectionParamsPath(collection)
- + "/PKG_VERSIONS?omitHeader=true&wt=javabin",
- Utils.JAVABINCONSUMER);
+ NamedList<Object> result =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ PackageUtils.getCollectionParamsPath(collection) +
"/PKG_VERSIONS",
+ new ModifiableSolrParams().add("omitHeader",
"true").add("wt", "javabin")));
Review Comment:
I suspect no params are needed at all, whereas maybe javabin used to be
necessary before. This is because you're using SolrJ instead of HttpClient
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java:
##########
@@ -727,22 +726,19 @@ private Map<String, String>
getCollectionParameterOverrides(
@SuppressWarnings({"rawtypes", "unchecked"})
Map<String, String> getPackageParams(String packageName, String collection) {
try {
+ NamedList<Object> response =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ PackageUtils.getCollectionParamsPath(collection) +
"/packages",
+ new ModifiableSolrParams()));
return (Map<String, String>)
- ((Map)
- ((Map)
- ((Map)
- PackageUtils.getJson(
- solrClient.getHttpClient(),
- solrBaseUrl
- +
PackageUtils.getCollectionParamsPath(collection)
- + "/packages",
- Map.class)
- .get("response"))
- .get("params"))
- .get("packages"))
- .get(packageName);
+ response._get("/response/params/packages/" + packageName,
Collections.emptyMap());
Review Comment:
Massive improvement here; thank you!
##########
solr/core/src/java/org/apache/solr/util/PackageTool.java:
##########
@@ -360,15 +365,18 @@ private String getZkHost(CommandLine cli) throws
Exception {
String zkHost = cli.getOptionValue("zkHost");
if (zkHost != null) return zkHost;
- String systemInfoUrl = solrUrl + "/admin/info/system";
- CloseableHttpClient httpClient = SolrCLI.getHttpClient();
- try {
+ try (SolrClient solrClient = getSolrClient(solrUrl)) {
// hit Solr to get system info
- Map<String, Object> systemInfo = SolrCLI.getJson(httpClient,
systemInfoUrl, 2, true);
+ NamedList<Object> systemInfo =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ CommonParams.SYSTEM_INFO_PATH,
+ new ModifiableSolrParams()));
Review Comment:
I've seen this *so* many times that I think this constructor should be
overloaded without params for when there aren't any. Feel free to do or not do
as you wish.
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -1059,12 +812,22 @@ public Option[] getOptions() {
protected void runImpl(CommandLine cli) throws Exception {
String getUrl = cli.getOptionValue("get");
if (getUrl != null) {
- Map<String, Object> json = getJson(getUrl);
-
- // pretty-print the response to stdout
- CharArr arr = new CharArr();
- new JSONWriter(arr, 2).write(json);
- echo(arr.toString());
+ URI uri = new URI(getUrl);
+ String solrUrl = getSolrUrlFromUri(uri);
+ String path = uri.getPath();
+ try (var solrClient = getSolrClient(solrUrl)) {
+ NamedList<Object> response =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ path.substring(path.indexOf("/", path.indexOf("/") + 1)),
Review Comment:
Needs a comment, perhaps with a trivial realistic example.
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java:
##########
@@ -727,22 +726,19 @@ private Map<String, String>
getCollectionParameterOverrides(
@SuppressWarnings({"rawtypes", "unchecked"})
Map<String, String> getPackageParams(String packageName, String collection) {
try {
+ NamedList<Object> response =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ PackageUtils.getCollectionParamsPath(collection) +
"/packages",
+ new ModifiableSolrParams()));
return (Map<String, String>)
- ((Map)
- ((Map)
- ((Map)
- PackageUtils.getJson(
- solrClient.getHttpClient(),
- solrBaseUrl
- +
PackageUtils.getCollectionParamsPath(collection)
- + "/packages",
- Map.class)
- .get("response"))
- .get("params"))
- .get("packages"))
- .get(packageName);
+ response._get("/response/params/packages/" + packageName,
Collections.emptyMap());
} catch (Exception ex) {
// This should be because there are no parameters. Be tolerant here.
+ if (log.isWarnEnabled()) {
Review Comment:
FYI log.isXXXEnabled is only mandated by our tooling/validation when the
string template params are not plain references. But here it's plain
references. Not to mention, I recall warning and louder has no limit.
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -1316,19 +1076,20 @@ protected void runCloudTool(CloudLegacySolrClient
cloudSolrClient, CommandLine c
q = new SolrQuery("*:*");
q.setRows(0);
q.set(DISTRIB, "false");
- try (HttpSolrClient solr = new
HttpSolrClient.Builder(coreUrl).build()) {
-
- String solrUrl = solr.getBaseURL();
-
- qr = solr.query(q);
+ int lastSlash = coreUrl.substring(0, coreUrl.length() -
1).lastIndexOf('/');
+ try (var solrClient = getSolrClient(coreUrl.substring(0,
lastSlash))) {
+ qr = solrClient.query(coreUrl.substring(lastSlash + 1,
coreUrl.length() - 1), q);
Review Comment:
I would strongly recommend declaring a variable "collectionName" so that the
variable itself can document *what* you are parsing
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -1316,19 +1076,20 @@ protected void runCloudTool(CloudLegacySolrClient
cloudSolrClient, CommandLine c
q = new SolrQuery("*:*");
q.setRows(0);
q.set(DISTRIB, "false");
- try (HttpSolrClient solr = new
HttpSolrClient.Builder(coreUrl).build()) {
-
- String solrUrl = solr.getBaseURL();
-
- qr = solr.query(q);
+ int lastSlash = coreUrl.substring(0, coreUrl.length() -
1).lastIndexOf('/');
+ try (var solrClient = getSolrClient(coreUrl.substring(0,
lastSlash))) {
+ qr = solrClient.query(coreUrl.substring(lastSlash + 1,
coreUrl.length() - 1), q);
Review Comment:
I see more what you are doing. You don't actually have to do any of this
parsing. Just call getSolrClient(coreUrl) (no substring stuff), and then when
you call solrClient.query, call the overlaoded version without the collection /
core name.
##########
solr/core/src/java/org/apache/solr/packagemanager/DefaultPackageRepository.java:
##########
@@ -101,16 +106,21 @@ public Path download(String artifactName) throws
SolrException, IOException {
}
private void initPackages() {
- try (CloseableHttpClient client = HttpClientBuilder.create().build()) {
+ try (Http2SolrClient client =
+ new Http2SolrClient.Builder(repositoryURL).useHttp1_1(true).build()) {
Review Comment:
You got it working; seems fine. A comment would be helpful to future
readers.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
