You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "dsmiley (via GitHub)" <gi...@apache.org> on 2023/03/03 19:29:19 UTC
[GitHub] [solr] dsmiley commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
dsmiley commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1124880964
##########
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:
Why HTTP 1.1? if there is a reason to keep it, it deserves a comment.
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java:
##########
@@ -268,19 +270,21 @@ public Map<String, SolrPackageInstance> getPackagesDeployedAsClusterLevelPlugins
MultiValuedMap<String, PluginMeta> packagePlugins = new HashSetValuedHashMap<>();
Map<String, Object> result;
try {
- result =
- (Map<String, Object>)
- Utils.executeGET(
- solrClient.getHttpClient(),
- solrBaseUrl + PackageUtils.CLUSTERPROPS_PATH,
- Utils.JSONCONSUMER);
- } catch (SolrException ex) {
- if (ex.code() == ErrorCode.NOT_FOUND.code) {
+ NamedList<Object> response =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ PackageUtils.CLUSTERPROPS_PATH,
+ new ModifiableSolrParams()));
+ Integer statusCode = (Integer) ((NamedList) response.get("responseHeader")).get("status");
Review Comment:
Would `NamedList.findRecursive` be nicer to read? I suspect so.
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -582,59 +571,41 @@ public static boolean checkCommunicationError(Exception exc) {
Throwable rootCause = SolrException.getRootCause(exc);
boolean wasCommError =
(rootCause instanceof ConnectException
- || rootCause instanceof ConnectTimeoutException
- || rootCause instanceof NoHttpResponseException
+ || rootCause instanceof SolrServerException
|| rootCause instanceof SocketException);
return wasCommError;
}
- /**
- * Tries a simple HEAD request and throws SolrException in case of Authorization error
- *
- * @param url the url to do a HEAD request to
- * @param httpClient the http client to use (make sure it has authentication optinos set)
- * @return the HTTP response code
- * @throws SolrException if auth/autz problems
- * @throws IOException if connection failure
- */
- private static int attemptHttpHead(String url, HttpClient httpClient)
- throws SolrException, IOException {
- HttpResponse response =
- httpClient.execute(new HttpHead(url), HttpClientUtil.createNewHttpClientRequestContext());
- int code = response.getStatusLine().getStatusCode();
+ private static void checkCodeForAuthError(int code) {
if (code == UNAUTHORIZED.code || code == FORBIDDEN.code) {
throw new SolrException(
SolrException.ErrorCode.getErrorCode(code),
- "Solr requires authentication for "
- + url
- + ". Please supply valid credentials. HTTP code="
+ "Solr requires authentication for request. Please supply valid credentials. HTTP code="
+ code);
}
- return code;
}
private static boolean exceptionIsAuthRelated(Exception exc) {
return (exc instanceof SolrException
&& Arrays.asList(UNAUTHORIZED.code, FORBIDDEN.code).contains(((SolrException) exc).code()));
}
- public static CloseableHttpClient getHttpClient() {
- ModifiableSolrParams params = new ModifiableSolrParams();
- params.set(HttpClientUtil.PROP_MAX_CONNECTIONS, 128);
- params.set(HttpClientUtil.PROP_MAX_CONNECTIONS_PER_HOST, 32);
- params.set(HttpClientUtil.PROP_FOLLOW_REDIRECTS, false);
- return HttpClientUtil.createClient(params);
+ public static SolrClient getSolrClient(String solrUrl) {
+ return new Http2SolrClient.Builder(solrUrl).maxConnectionsPerHost(32).build();
}
- @SuppressWarnings("deprecation")
- public static void closeHttpClient(CloseableHttpClient httpClient) {
- if (httpClient != null) {
- try {
- HttpClientUtil.close(httpClient);
- } catch (Exception exc) {
- // safe to ignore, we're just shutting things down
- }
+ public static String getSolrUrlFromUri(URI uri) {
Review Comment:
Woah; why not just uri.toString? A javadoc/comment with an example would help.
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -1059,12 +942,23 @@ 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);
+ ModifiableSolrParams paramsMap = getSolrParamsFromUri(uri);
Review Comment:
Simply "params"
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java:
##########
@@ -140,17 +143,16 @@ public static String getFileFromJarsAsString(List<Path> jars, String filename) {
return null;
}
- /** Returns JSON string from a given URL */
- public static String getJsonStringFromUrl(HttpClient client, String url) {
+ /** Returns JSON string from a given Solr URL */
+ public static String getJsonStringFromUrl(
+ SolrClient client, String path, Map<String, String[]> params) {
Review Comment:
Shouldn't "params" here by SolrParams? This is pervasive in Solr, and the underlying implementation wants it any way.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -505,6 +505,9 @@ private NamedList<Object> processErrorsAndResponse(
if (contentType != null) {
mimeType = MimeTypes.getContentTypeWithoutCharset(contentType);
encoding = MimeTypes.getCharsetFromContentType(contentType);
+ if (parser.getWriterType().equals("json") && encoding == null) {
+ encoding = FALLBACK_CHARSET.name();
+ }
Review Comment:
interesting; what led to this change?
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java:
##########
@@ -94,29 +92,34 @@ public static void postFile(SolrClient client, ByteBuffer buffer, String name, S
if (sig != null) {
params.add("sig", sig);
}
- V2Response rsp =
- new V2Request.Builder(resource)
- .withMethod(SolrRequest.METHOD.PUT)
- .withPayload(buffer)
- .forceV2(true)
- .withMimeType("application/octet-stream")
- .withParams(params)
- .build()
- .process(client);
- if (!name.equals(rsp.getResponse().get(CommonParams.FILE))) {
- throw new SolrException(
- ErrorCode.BAD_REQUEST,
- "Mismatch in file uploaded. Uploaded: "
- + rsp.getResponse().get(CommonParams.FILE)
- + ", Original: "
- + name);
- }
+ GenericSolrRequest request =
+ new GenericSolrRequest(SolrRequest.METHOD.PUT, resource, params) {
+ @Override
+ public RequestWriter.ContentWriter getContentWriter(String expectedType) {
+ return new RequestWriter.ContentWriter() {
+ public final ByteBuffer payload = buffer;
+
+ @Override
+ public void write(OutputStream os) throws IOException {
+ if (payload == null) return;
+ os.write(payload.array());
Review Comment:
array() is an optional method, and even if it it implemented, you'd still need to respect the offset & length.
Instead: https://stackoverflow.com/a/579616/92186
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -582,59 +571,41 @@ public static boolean checkCommunicationError(Exception exc) {
Throwable rootCause = SolrException.getRootCause(exc);
boolean wasCommError =
(rootCause instanceof ConnectException
- || rootCause instanceof ConnectTimeoutException
- || rootCause instanceof NoHttpResponseException
+ || rootCause instanceof SolrServerException
|| rootCause instanceof SocketException);
return wasCommError;
}
- /**
- * Tries a simple HEAD request and throws SolrException in case of Authorization error
- *
- * @param url the url to do a HEAD request to
- * @param httpClient the http client to use (make sure it has authentication optinos set)
- * @return the HTTP response code
- * @throws SolrException if auth/autz problems
- * @throws IOException if connection failure
- */
- private static int attemptHttpHead(String url, HttpClient httpClient)
- throws SolrException, IOException {
- HttpResponse response =
- httpClient.execute(new HttpHead(url), HttpClientUtil.createNewHttpClientRequestContext());
- int code = response.getStatusLine().getStatusCode();
+ private static void checkCodeForAuthError(int code) {
if (code == UNAUTHORIZED.code || code == FORBIDDEN.code) {
throw new SolrException(
SolrException.ErrorCode.getErrorCode(code),
- "Solr requires authentication for "
- + url
- + ". Please supply valid credentials. HTTP code="
+ "Solr requires authentication for request. Please supply valid credentials. HTTP code="
+ code);
}
- return code;
}
private static boolean exceptionIsAuthRelated(Exception exc) {
return (exc instanceof SolrException
&& Arrays.asList(UNAUTHORIZED.code, FORBIDDEN.code).contains(((SolrException) exc).code()));
}
- public static CloseableHttpClient getHttpClient() {
- ModifiableSolrParams params = new ModifiableSolrParams();
- params.set(HttpClientUtil.PROP_MAX_CONNECTIONS, 128);
- params.set(HttpClientUtil.PROP_MAX_CONNECTIONS_PER_HOST, 32);
- params.set(HttpClientUtil.PROP_FOLLOW_REDIRECTS, false);
- return HttpClientUtil.createClient(params);
+ public static SolrClient getSolrClient(String solrUrl) {
+ return new Http2SolrClient.Builder(solrUrl).maxConnectionsPerHost(32).build();
}
- @SuppressWarnings("deprecation")
- public static void closeHttpClient(CloseableHttpClient httpClient) {
- if (httpClient != null) {
- try {
- HttpClientUtil.close(httpClient);
- } catch (Exception exc) {
- // safe to ignore, we're just shutting things down
- }
+ public static String getSolrUrlFromUri(URI uri) {
+ return uri.getScheme() + "://" + uri.getAuthority() + "/" + uri.getPath().split("/")[1];
+ }
+
+ public static ModifiableSolrParams getSolrParamsFromUri(URI uri) {
+ ModifiableSolrParams paramsMap = new ModifiableSolrParams();
+ String[] params = uri.getQuery() == null ? new String[] {} : uri.getQuery().split("&");
+ for (String param : params) {
+ String[] paramSplit = param.split("=");
+ paramsMap.add(paramSplit[0], paramSplit[1]);
Review Comment:
no percent URL decoding? See `java.net.URLDecoder`
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java:
##########
@@ -228,20 +230,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);
+ Map<String, String[]> paramsMap = new LinkedHashMap<>();
+ paramsMap.put("omitHeader", new String[] {"true"});
+ paramsMap.put("wt", new String[] {"javabin"});
Review Comment:
This is fine but it's more direct/simple to create ModifiableSolrParams and add stuff to it. `add` is chain-able as well (returns `this`).
##########
solr/core/src/test/org/apache/solr/cloud/SolrCloudExampleTest.java:
##########
@@ -222,109 +217,111 @@ protected void doTestDeleteAction(String testCollectionName, String solrUrl) thr
* Uses the SolrCLI config action to activate soft auto-commits for the getting started
* collection.
*/
+ @SuppressWarnings("unchecked")
protected void doTestConfigUpdate(String testCollectionName, String solrUrl) throws Exception {
if (!solrUrl.endsWith("/")) solrUrl += "/";
- String configUrl = solrUrl + testCollectionName + "/config";
-
- Map<String, Object> configJson = SolrCLI.getJson(configUrl);
- Object maxTimeFromConfig =
- SolrCLI.atPath("/config/updateHandler/autoSoftCommit/maxTime", configJson);
- assertNotNull(maxTimeFromConfig);
- assertEquals(-1L, maxTimeFromConfig);
-
- String prop = "updateHandler.autoSoftCommit.maxTime";
- Long maxTime = 3000L;
- String[] args =
- new String[] {
- "-collection", testCollectionName,
- "-property", prop,
- "-value", maxTime.toString(),
- "-solrUrl", solrUrl
- };
-
- Map<String, Long> startTimes = getSoftAutocommitInterval(testCollectionName);
-
- SolrCLI.ConfigTool tool = new SolrCLI.ConfigTool();
- CommandLine cli =
- SolrCLI.processCommandLineArgs(SolrCLI.joinCommonAndToolOptions(tool.getOptions()), args);
- log.info("Sending set-property '{}'={} to SolrCLI.ConfigTool.", prop, maxTime);
- assertEquals("Set config property failed!", 0, tool.runTool(cli));
- configJson = SolrCLI.getJson(configUrl);
- maxTimeFromConfig = SolrCLI.atPath("/config/updateHandler/autoSoftCommit/maxTime", configJson);
- assertNotNull(maxTimeFromConfig);
- assertEquals(maxTime, maxTimeFromConfig);
+ try (SolrClient solrClient = SolrCLI.getSolrClient(solrUrl)) {
+ NamedList<Object> configJson =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ "/" + testCollectionName + "/config",
+ new ModifiableSolrParams()));
+ Object maxTimeFromConfig =
+ SolrCLI.atPath(
+ "/updateHandler/autoSoftCommit/maxTime",
+ (Map<String, Object>) configJson.get("config"));
+ assertNotNull(maxTimeFromConfig);
+ assertEquals(-1, maxTimeFromConfig);
+
+ String prop = "updateHandler.autoSoftCommit.maxTime";
+ Integer maxTime = 3000;
+ String[] args =
+ new String[] {
+ "-collection", testCollectionName,
+ "-property", prop,
+ "-value", maxTime.toString(),
+ "-solrUrl", solrUrl
+ };
+
+ Map<String, Integer> startTimes = getSoftAutocommitInterval(testCollectionName, solrClient);
+
+ SolrCLI.ConfigTool tool = new SolrCLI.ConfigTool();
+ CommandLine cli =
+ SolrCLI.processCommandLineArgs(SolrCLI.joinCommonAndToolOptions(tool.getOptions()), args);
+ log.info("Sending set-property '{}'={} to SolrCLI.ConfigTool.", prop, maxTime);
+ assertEquals("Set config property failed!", 0, tool.runTool(cli));
+
+ configJson =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ "/" + testCollectionName + "/config",
+ new ModifiableSolrParams()));
+ maxTimeFromConfig =
+ SolrCLI.atPath(
+ "/updateHandler/autoSoftCommit/maxTime",
+ (Map<String, Object>) configJson.get("config"));
Review Comment:
This atPath utility looks vaguely redundant with NamedList.findRecursively or Utils.getObjectByPath
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -828,7 +665,54 @@ public static <T> T pathAs(Class<T> clazz, String jsonPath, Map<String, Object>
}
/**
- * Helper function for reading an Object of unknown type from a JSON Object tree.
+ * Helper function for reading an Object of unknown type from a JSON Object tree stored in
+ * NamedList format.
+ *
+ * <p>To find a path to a child that starts with a slash (e.g. queryHandler named /query) you must
Review Comment:
The term queryHandler is an ancient Solr word; requestHandler is the general replacement.
##########
solr/core/src/test/org/apache/solr/cloud/SolrCloudExampleTest.java:
##########
@@ -222,109 +217,111 @@ protected void doTestDeleteAction(String testCollectionName, String solrUrl) thr
* Uses the SolrCLI config action to activate soft auto-commits for the getting started
* collection.
*/
+ @SuppressWarnings("unchecked")
protected void doTestConfigUpdate(String testCollectionName, String solrUrl) throws Exception {
if (!solrUrl.endsWith("/")) solrUrl += "/";
- String configUrl = solrUrl + testCollectionName + "/config";
-
- Map<String, Object> configJson = SolrCLI.getJson(configUrl);
- Object maxTimeFromConfig =
- SolrCLI.atPath("/config/updateHandler/autoSoftCommit/maxTime", configJson);
- assertNotNull(maxTimeFromConfig);
- assertEquals(-1L, maxTimeFromConfig);
-
- String prop = "updateHandler.autoSoftCommit.maxTime";
- Long maxTime = 3000L;
- String[] args =
- new String[] {
- "-collection", testCollectionName,
- "-property", prop,
- "-value", maxTime.toString(),
- "-solrUrl", solrUrl
- };
-
- Map<String, Long> startTimes = getSoftAutocommitInterval(testCollectionName);
-
- SolrCLI.ConfigTool tool = new SolrCLI.ConfigTool();
- CommandLine cli =
- SolrCLI.processCommandLineArgs(SolrCLI.joinCommonAndToolOptions(tool.getOptions()), args);
- log.info("Sending set-property '{}'={} to SolrCLI.ConfigTool.", prop, maxTime);
- assertEquals("Set config property failed!", 0, tool.runTool(cli));
- configJson = SolrCLI.getJson(configUrl);
- maxTimeFromConfig = SolrCLI.atPath("/config/updateHandler/autoSoftCommit/maxTime", configJson);
- assertNotNull(maxTimeFromConfig);
- assertEquals(maxTime, maxTimeFromConfig);
+ try (SolrClient solrClient = SolrCLI.getSolrClient(solrUrl)) {
+ NamedList<Object> configJson =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ "/" + testCollectionName + "/config",
+ new ModifiableSolrParams()));
+ Object maxTimeFromConfig =
+ SolrCLI.atPath(
+ "/updateHandler/autoSoftCommit/maxTime",
+ (Map<String, Object>) configJson.get("config"));
+ assertNotNull(maxTimeFromConfig);
+ assertEquals(-1, maxTimeFromConfig);
+
+ String prop = "updateHandler.autoSoftCommit.maxTime";
+ Integer maxTime = 3000;
+ String[] args =
+ new String[] {
+ "-collection", testCollectionName,
+ "-property", prop,
+ "-value", maxTime.toString(),
+ "-solrUrl", solrUrl
+ };
+
+ Map<String, Integer> startTimes = getSoftAutocommitInterval(testCollectionName, solrClient);
+
+ SolrCLI.ConfigTool tool = new SolrCLI.ConfigTool();
+ CommandLine cli =
+ SolrCLI.processCommandLineArgs(SolrCLI.joinCommonAndToolOptions(tool.getOptions()), args);
+ log.info("Sending set-property '{}'={} to SolrCLI.ConfigTool.", prop, maxTime);
+ assertEquals("Set config property failed!", 0, tool.runTool(cli));
+
+ configJson =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ "/" + testCollectionName + "/config",
+ new ModifiableSolrParams()));
+ maxTimeFromConfig =
+ SolrCLI.atPath(
+ "/updateHandler/autoSoftCommit/maxTime",
+ (Map<String, Object>) configJson.get("config"));
+ assertNotNull(maxTimeFromConfig);
+ assertEquals(maxTime, maxTimeFromConfig);
- // Just check that we can access paths with slashes in them both through an intermediate method
- // and explicitly using atPath.
Review Comment:
Why were these tests removed?
--
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