bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1159424945


##########
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:
   @epugh as I was searching for this in the codebase, the only place where I 
could find usage for the jetty http client was the Http2SolrClient class. I may 
have missed something, but I believe we make use of the apache client wherever 
we just need to do a simple get. If you know of any place where we do it with 
the jetty client please help me out and let me know!
   
   @dsmiley good idea for the comment, I will add it there!



##########
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:
   I was under the impression that I put it there because the validation did 
not pass, but I will remove it. Good to know this information!



##########
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:
   Great idea, done. Also changed this in all places where we used the 
constructor with null or empty ModifiableSolrParams



##########
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:
   Great suggestion, it looks much better like this, thank you!



-- 
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

Reply via email to