You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/11/17 17:55:47 UTC
[GitHub] [solr] risdenk commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
risdenk commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1025507593
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -968,38 +983,42 @@ public Map<String, Object> getStatus(String solrUrl) throws Exception {
if (!solrUrl.endsWith("/")) solrUrl += "/";
- String systemInfoUrl = solrUrl + "admin/info/system";
- CloseableHttpClient httpClient = getHttpClient();
+ Http2SolrClient http2SolrClient = getHttpSolrClient(solrUrl);
Review Comment:
I think this could be:
```
try (var http2SolrClient = getHttpSolrClient(solrUrl)) {
...
}
```
then you don't even need the finally?
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -1059,12 +1080,38 @@ public Option[] getOptions() {
protected void runImpl(CommandLine cli) throws Exception {
String getUrl = cli.getOptionValue("get");
if (getUrl != null) {
- Map<String, Object> json = getJson(getUrl);
+ String[] urlAndParams = getUrl.split("\\?");
+ String[] getUrlSplit = urlAndParams[0].split("/");
+ String[] params = null;
+ if (urlAndParams.length > 1) {
+ params = urlAndParams[1].split("&");
+ }
+ String baseUrl = getUrlSplit[0] + "//" + getUrlSplit[2] + "/" + getUrlSplit[3];
+ StringBuilder getUrlBuilder = new StringBuilder();
+ for (int i = 4; i < getUrlSplit.length; i++) {
+ getUrlBuilder.append("/").append(getUrlSplit[i]);
+ }
+ ModifiableSolrParams paramsMap = new ModifiableSolrParams();
+ if (params != null) {
+ for (String param : params) {
+ String[] paramSplit = param.split("=");
+ paramsMap.add(paramSplit[0], paramSplit[1]);
+ }
+ }
+ Http2SolrClient http2SolrClient = getHttpSolrClient(baseUrl);
+ try {
Review Comment:
This should be try-with-resources and avoid the finally.
##########
solr/core/src/java/org/apache/solr/util/PackageTool.java:
##########
@@ -360,15 +366,19 @@ 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();
+ Http2SolrClient http2SolrClient = getHttpSolrClient(solrUrl);
Review Comment:
I think this could be:
```
try (var http2SolrClient = getHttpSolrClient(solrUrl)) {
...
}
```
then you don't even need the finally?
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -1316,19 +1364,24 @@ 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('/');
+ Http2SolrClient http2SolrClient =
+ new Http2SolrClient.Builder(coreUrl.substring(0, lastSlash)).build();
+ try {
Review Comment:
`try-with-resources` and shouldn't this use `getHttpSolrClient` method?
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -1470,15 +1525,19 @@ public static String getZkHost(CommandLine cli) throws Exception {
if (!solrUrl.endsWith("/")) solrUrl += "/";
- String systemInfoUrl = solrUrl + "admin/info/system";
- CloseableHttpClient httpClient = getHttpClient();
+ Http2SolrClient http2SolrClient = getHttpSolrClient(solrUrl);
try {
Review Comment:
`try-with-resources`
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -1489,27 +1548,32 @@ public static String getZkHost(CommandLine cli) throws Exception {
zkHost = zookeeper;
}
} finally {
- HttpClientUtil.close(httpClient);
+ closeHttpSolrClient(http2SolrClient);
}
return zkHost;
}
- public static boolean safeCheckCollectionExists(String url, String collection) {
+ public static boolean safeCheckCollectionExists(String baseUrl, String collection) {
boolean exists = false;
+ Http2SolrClient http2SolrClient = getHttpSolrClient(baseUrl);
try {
Review Comment:
`try-with-resources`
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -1059,12 +1080,38 @@ public Option[] getOptions() {
protected void runImpl(CommandLine cli) throws Exception {
String getUrl = cli.getOptionValue("get");
if (getUrl != null) {
- Map<String, Object> json = getJson(getUrl);
+ String[] urlAndParams = getUrl.split("\\?");
+ String[] getUrlSplit = urlAndParams[0].split("/");
+ String[] params = null;
+ if (urlAndParams.length > 1) {
+ params = urlAndParams[1].split("&");
+ }
+ String baseUrl = getUrlSplit[0] + "//" + getUrlSplit[2] + "/" + getUrlSplit[3];
+ StringBuilder getUrlBuilder = new StringBuilder();
Review Comment:
This seems really wonky. I would NOT recommend doing url parsing this way.
I think what you might be looking for is `new URI(String)` and then pull out the specific parts as needed from the URI?
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -626,6 +634,12 @@ public static CloseableHttpClient getHttpClient() {
return HttpClientUtil.createClient(params);
}
+ public static void closeHttpSolrClient(Http2SolrClient http2SolrClient) {
+ if (http2SolrClient != null) {
+ http2SolrClient.close();
+ }
+ }
Review Comment:
Not sure this is needed...
--
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