[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.

["bszabo97 (via GitHub)" <gi...@apache.org>]

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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org