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:19:03 UTC
[GitHub] [solr] bszabo97 opened a new pull request, #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
bszabo97 opened a new pull request, #1182:
URL: https://github.com/apache/solr/pull/1182
https://issues.apache.org/jira/browse/SOLR-16504
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Solr:
* https://issues.apache.org/jira/projects/SOLR-16504
For something minor (i.e. that wouldn't be worth putting in release notes), you can skip JIRA.
To create a Jira issue, you will need to create an account there first.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# Description
This is a subtask of SOLR-16367 Switch from CloudLegacySolrClient to CloudSolrClient with description:
CloudLegacySolrClient is deprecated; it uses the Apache HTTP client. CloudSolrClient uses the Jetty HTTP client, which supports HTTP 2. In this issue, switch all of Solr to not use CloudLegacySolrClient anymore.
This PR is just a work-in-progress solution for migrating the SolrCLI tool from the legacy apache http client to jetty http client.
# Solution
The solution implements the previous functionalities but with the use of jetty http client instead of the http client. It also processes the response of requests with the use of NamedList as it is done in the jetty client, not using the regular Map as the apache client does.
# Tests
No additional tests were added. I performed manual tests for all SolrCLI functionalities that I could think except the ones which are done by separate classes like `package`.
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [x] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1368214720
I didn't have any luck debugging.... I did test that on this branch, the package manager works, following the steps in the packagemanager manager api internals page...
I did see this error:
```
/home/runner/work/solr/solr/solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java:111: warning: [ByteBufferBackingArray] ByteBuffer.array() shouldn't be called unless ByteBuffer.arrayOffset() is used or if the ByteBuffer was initialized using ByteBuffer.wrap() or ByteBuffer.allocate().
```
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1507665297
@bszabo97 do the tests run for you? I am getting failures for:
```
./gradlew :solr:core:test --tests "org.apache.solr.util.TestSolrCLIRunExample.testTechproductsExample" -Ptests.jvms=4 "-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC -XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" -Ptests.seed=64B964B8A1FDC6B8 -Ptests.nightly=true -Ptests.file.encoding=ISO-8859-1
```
--
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1126305516
##########
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:
These two tests were redundant because they tested the `pathAt` and `asString` methods from SolrCLI. In this implementation I only kept the `atPath` method so I removed the `asString` test but kept the other.
In the new implementation with findRecursive I will also just keep one of these tests since the `atPath` method will be removed as well.
--
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
[GitHub] [solr] bszabo97 commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1476344039
I have extracted an existing ResponseParser which parsed JSON responses and using that I was able to work around the hack which we talked about in Http2SolrClient, but this raised another concern: the `writeRawFile` function is (of course) also used for getting other raw files, not just json formatted ones, so I had to leave the original method which uses filestream. It is not quite pretty but I was not able to come up with a nicer solution.
The other thing is that DistribPackageStore is also making use of the apache http client in some places which I was not yet able to get rid of, because firstly I wanted to solve this problem around the PackageStoreAPI.
Any suggestion is very welcomed!
--
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
[GitHub] [solr] bszabo97 commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1453772240
Sorry that an update from me took so long, I was pretty stuck on a problem regarding the package manager.
Now I have uploaded a new commit which resolved that problem and also removed the use of apache client I believe from all of the classes that are in the scope of this ticket.
I would be grateful for any reviews on this.
As for the integration tests I am sure we can add them, but at the moment I did not put much thought into it yet, I would also be happy for some guidance on that side.
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1453825905
This is really exciting work! I did a bit of poking, and we have these integration tests in `./solr/packaging/test/` that use BATS for testing the SolrCLI. I wonder if this is the right place for some integration tests? It looks like we already have some integration tests that exercise the solrcli.....
--
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1126325809
##########
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:
Without the correct encoding specified here I got an exception when requesting the manifest.json file, since the SHA did not match the expected.
I tried to set the content type for the response to contain the encoding but when I dug deeper it turned out the we get if from the BinaryResponseParser and I was more afraid to change it there since it would affect much more then just the manifest.json so I added this if here.
The other reason behind this was that we make use of this `FALLBACK_CHARSET` in many places in this class if we do not have an encoding specified, so I figured it would not mess up things if I add it here as well.
--
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
[GitHub] [solr] dsmiley commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1128021493
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java:
##########
@@ -268,19 +266,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) response.findRecursive("responseHeader", "status");
+ if (statusCode == ErrorCode.NOT_FOUND.code) {
Review Comment:
this is a valid finding
--
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
[GitHub] [solr] dsmiley commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
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-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
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1050663419
##########
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:
<picture><img alt="19% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/19/display.svg"></picture>
π¬ 3 similar findings have been found in this PR
---
*NULL_DEREFERENCE:* object returned by `response.get("responseHeader")` could be null and is dereferenced at line 279.
---
<details><summary><b>π Expand here to view all instances of this finding</b></summary><br/>
<div align=\"center\">
| **File Path** | **Line Number** |
| ------------- | ------------- |
| solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java | [280](https://github.com/apache/solr/blob/6d3950de6ee760a6cd7fa93014fd9533fb25ea85/solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java#L280) |
| solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java | [201](https://github.com/apache/solr/blob/6d3950de6ee760a6cd7fa93014fd9533fb25ea85/solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java#L201) |
| solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java | [201](https://github.com/apache/solr/blob/6d3950de6ee760a6cd7fa93014fd9533fb25ea85/solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java#L201) |
<p><a href="https://lift.sonatype.com/results/github.com/apache/solr/01GMD9JPHAWQ4DBYVB5ZQF0BNB?t=Infer|NULL_DEREFERENCE" target="_blank">Visit the Lift Web Console</a> to find more details in your report.</p></div></details>
---
<details><summary><b>βΉοΈ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [π Not relevant](https://www.sonatype.com/lift-comment-rating?comment=362095266&lift_comment_rating=1) ] - [ [π Won't fix](https://www.sonatype.com/lift-comment-rating?comment=362095266&lift_comment_rating=2) ] - [ [π Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=362095266&lift_comment_rating=3) ] - [ [π Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=362095266&lift_comment_rating=4) ] - [ [π Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=362095266&lift_comment_rating=5) ]
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1025529342
##########
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);
try {
- // hit Solr to get system info
- Map<String, Object> systemInfo = getJson(httpClient, systemInfoUrl, 2, true);
+ NamedList<Object> systemInfo =
+ http2SolrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ CommonParams.SYSTEM_INFO_PATH,
+ new ModifiableSolrParams()));
// convert raw JSON into user-friendly output
- status = reportStatus(solrUrl, systemInfo, httpClient);
+ status = reportStatus(systemInfo, http2SolrClient);
} finally {
- closeHttpClient(httpClient);
+ closeHttpSolrClient(http2SolrClient);
}
return status;
}
- public Map<String, Object> reportStatus(
- String solrUrl, Map<String, Object> info, HttpClient httpClient) throws Exception {
+ public Map<String, Object> reportStatus(NamedList<Object> info, Http2SolrClient http2SolrClient)
+ throws Exception {
Map<String, Object> status = new LinkedHashMap<>();
String solrHome = (String) info.get("solr_home");
status.put("solr_home", solrHome != null ? solrHome : "?");
- status.put("version", asString("/lucene/solr-impl-version", info));
- status.put("startTime", asString("/jvm/jmx/startTime", info));
- status.put("uptime", uptime(asLong("/jvm/jmx/upTimeMS", info)));
+ status.put("version", ((NamedList) info.get("lucene")).get("solr-impl-version"));
- String usedMemory = asString("/jvm/memory/used", info);
- String totalMemory = asString("/jvm/memory/total", info);
+ @SuppressWarnings("unchecked")
+ NamedList<Object> jvm = (NamedList<Object>) info.get("jvm");
+ status.put("startTime", ((NamedList) jvm.get("jmx")).get("startTime"));
+ status.put("uptime", uptime((Long) ((NamedList) jvm.get("jmx")).get("upTimeMS")));
+ String usedMemory = (String) ((NamedList) jvm.get("memory")).get("used");
+ String totalMemory = (String) ((NamedList) jvm.get("memory")).get("total");
Review Comment:
π¬ 13 similar findings have been found in this PR
---
*NULL_DEREFERENCE:* object returned by `jvm.get("memory")` could be null and is dereferenced at line 1016.
---
<details><summary><b>π Expand here to view all instances of this finding</b></summary><br/>
<div align="center">
| **File Path** | **Line Number** |
| ------------- | ------------- |
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1014](https://github.com/bszabo97/solr/blob/fd249f90b7b872672b2f1026aa1c1f015550c6a6/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1014)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1015](https://github.com/bszabo97/solr/blob/fd249f90b7b872672b2f1026aa1c1f015550c6a6/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1015)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1013](https://github.com/bszabo97/solr/blob/fd249f90b7b872672b2f1026aa1c1f015550c6a6/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1013)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1009](https://github.com/bszabo97/solr/blob/fd249f90b7b872672b2f1026aa1c1f015550c6a6/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1009)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [2762](https://github.com/bszabo97/solr/blob/fd249f90b7b872672b2f1026aa1c1f015550c6a6/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L2762)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1588](https://github.com/bszabo97/solr/blob/fd249f90b7b872672b2f1026aa1c1f015550c6a6/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1588)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1014](https://github.com/bszabo97/solr/blob/fd249f90b7b872672b2f1026aa1c1f015550c6a6/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1014)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1044](https://github.com/bszabo97/solr/blob/fd249f90b7b872672b2f1026aa1c1f015550c6a6/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1044)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1593](https://github.com/bszabo97/solr/blob/fd249f90b7b872672b2f1026aa1c1f015550c6a6/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1593)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1042](https://github.com/bszabo97/solr/blob/fd249f90b7b872672b2f1026aa1c1f015550c6a6/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1042)|
<p> Showing <b>10</b> of <b>13</b> findings. <a href="https://lift.sonatype.com/results/github.com/apache/solr/01GJ39KRQHRPDPXDNF11160VHK?t=Infer|NULL_DEREFERENCE" target="_blank">Visit the Lift Web Console</a> to see all.</p></div></details>
---
Fix rate: 36%
---
<details><summary><b>βΉοΈ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [π Not relevant](https://www.sonatype.com/lift-comment-rating?comment=353794355&lift_comment_rating=1) ] - [ [π Won't fix](https://www.sonatype.com/lift-comment-rating?comment=353794355&lift_comment_rating=2) ] - [ [π Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=353794355&lift_comment_rating=3) ] - [ [π Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=353794355&lift_comment_rating=4) ] - [ [π Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=353794355&lift_comment_rating=5) ]
--
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1126310561
##########
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:
I was not aware of the existence of this method but this helps A LOT, thank you! I was really unhappy with all these castings and `atPath` methods. Using `findRecursive` is much cleaner in these places
--
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
[GitHub] [solr] dsmiley commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1128029913
##########
solr/core/src/test/org/apache/solr/cloud/SolrCloudExampleTest.java:
##########
@@ -260,22 +259,22 @@ protected void doTestConfigUpdate(String testCollectionName, String solrUrl) thr
"/" + testCollectionName + "/config",
new ModifiableSolrParams()));
maxTimeFromConfig =
- SolrCLI.atPath(
- "/updateHandler/autoSoftCommit/maxTime",
- (Map<String, Object>) configJson.get("config"));
+ configJson._get("/config/updateHandler/autoSoftCommit/maxTime", Collections.emptyMap());
assertNotNull(maxTimeFromConfig);
assertEquals(maxTime, maxTimeFromConfig);
if (log.isInfoEnabled()) {
log.info("live_nodes_count : {}", cloudClient.getClusterState().getLiveNodes());
}
+ // Need to use the _get(List, Object) here because of /query in the path
assertEquals(
"Should have been able to get a value from the /query request handler",
"explicit",
- SolrCLI.atPath(
- "/requestHandler/\\/query/defaults/echoParams",
- (Map<String, Object>) configJson.get("config")));
+ configJson._get(
+ new ArrayList<>(
+ Arrays.asList("config", "requestHandler", "/query", "defaults", "echoParams")),
Review Comment:
why new ArrayList ?
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1501129913
Thanks for sharing the update (and the detailed test notes!).... Maybe for another day, but having more .bats style tests for these would be great.
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1499266749
> I am right now running the tests locally after the rebase to check if everything is still working then I will execute some manual testing. I push the changes and post and update with the results later today.
>
> Thank you @dsmiley and @epugh for the great suggestions and for pushing this PR forward :)
I think you are right about the HttpClient thing...
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1368048938
@bszabo97 I am poking at this, and it *may* be that in PackageStoreAPI.java we attempt to write the raw data back via `writeRawFile`, and it wants to use filestream, but we have specified `wt=javabin`??? not sure.
--
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
[GitHub] [solr] risdenk commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1502044312
Okay, it looks like the reason that the `SQL Module` test is failing is that the we need now to use `%20` instead of `+` for the spaces in the url...
```
@@ -32,7 +32,7 @@ teardown() {
@test "SQL Module" {
run solr start -c -Dsolr.modules=sql
run solr create_collection -c COLL_NAME
- run solr api -get http://localhost:8983/solr/COLL_NAME/sql?stmt=select+id+from+COLL_NAME+limit+10
+ run solr api -get http://localhost:8983/solr/COLL_NAME/sql?stmt=select%20id%20from%20COLL_NAME%20limit%2010
```
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1497461482
Awesome! I used the BATS tests for the first timeβ¦. You may find that
itβs worth adding more of them to cover anything requiring manual testingβ¦.
On Wed, Apr 5, 2023 at 1:19 PM bszabo97 ***@***.***> wrote:
> Thank you @epugh <https://github.com/epugh> for having a look at this!
>
> I was just about to do some manual testing and rebase this PR to the
> current master, unfortunately I had lots of other stuff on my plate
> recently and was not able to proceed with this. I hope I can do it tomorrow
> and also look into you recent review.
>
> β
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/solr/pull/1182#issuecomment-1497321076>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAAFO647GNUIYWSECSIO47TW7VIKXANCNFSM6AAAAAASDTTERU>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1508396109
> Woow somehow having a broken CreateCoreTool did not cause this test to fail, but since the safeCheckCoreExists was also broken when the CreateCoreTool got fixed it broke the test. Nice catch @epugh thank you! I uploaded a fix, now this test is successful for me and I am running all the tests just now
thanks! I was just stepipng htrough the debugger, pulling my hair out... you are ahead of me! So glad you found the bug.
--
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
[GitHub] [solr] bszabo97 commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1479921161
I am right now working on a bigger cleanup because there is plenty to do and afterwards if you are also happy with how this looks I am all in for starting the testing.
--
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
[GitHub] [solr] dsmiley commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1126525931
##########
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:
This is very much a hack (a needless special case) so I'm trying to avoid this so that Http level code needn't have assumptions on formats when we already have abstractions for those (the ResponseParser hierarchy) where such logic might go. Since it seems your special case is JSON specific, how is it that BinaryResponseParser (JAVABIN) is relevant?
--
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
[GitHub] [solr] dsmiley commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1126325809
##########
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:
Without the correct encoding specified here I got an exception when requesting the manifest.json file, since the SHA did not match the expected.
I tried to set the content type for the response to contain the encoding but when I dug deeper it turned out the we get if from the BinaryResponseParser and I was more afraid to change it there since it would affect much more then just the manifest.json so I added this if here.
--
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1126768208
##########
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:
I can agree with you that this feels a bit hacky. Let me dig a bit deeper into it
--
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
[GitHub] [solr] dsmiley commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1126492302
##########
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:
I wonder how the HTTP protocol change (a low level thing) alters the the higher level response format? Seems like there is something to root-cause there.
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1127940444
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java:
##########
@@ -268,19 +266,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) response.findRecursive("responseHeader", "status");
+ if (statusCode == ErrorCode.NOT_FOUND.code) {
Review Comment:
<picture><img alt="6% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/6/display.svg"></picture>
<b>*NULLPTR_DEREFERENCE:</b>* `statusCode` could be null (last assigned on line 275) and is dereferenced.
---
<details><summary>βΉοΈ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[β[π Not relevant](https://www.sonatype.com/lift-comment-rating?comment=426196408&lift_comment_rating=1)β] - [β[π Won't fix](https://www.sonatype.com/lift-comment-rating?comment=426196408&lift_comment_rating=2)β] - [β[π Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=426196408&lift_comment_rating=3)β] - [β[π Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=426196408&lift_comment_rating=4)β] - [β[π Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=426196408&lift_comment_rating=5)β]
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java:
##########
@@ -268,19 +266,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) response.findRecursive("responseHeader", "status");
+ if (statusCode == ErrorCode.NOT_FOUND.code) {
Review Comment:
<picture><img alt="6% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/6/display.svg"></picture>
<b>*NULLPTR_DEREFERENCE:</b>* `statusCode` could be null (from the call to `NamedList.findRecursive(...)` on line 275) and is dereferenced.
---
<details><summary>βΉοΈ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[β[π Not relevant](https://www.sonatype.com/lift-comment-rating?comment=426197139&lift_comment_rating=1)β] - [β[π Won't fix](https://www.sonatype.com/lift-comment-rating?comment=426197139&lift_comment_rating=2)β] - [β[π Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=426197139&lift_comment_rating=3)β] - [β[π Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=426197139&lift_comment_rating=4)β] - [β[π Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=426197139&lift_comment_rating=5)β]
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1504046891
@bszabo97 I was able to run export just fine:
```
(base) β solr-10.0.0-SNAPSHOT git:(SOLR-16367) β ./bin/solr export -url http://localhost:8983/solr/films
NO: of shards : 1
.
Total Docs exported: 100. Time taken: 0secs
```
However, with a larger number output, I get a different count:
```
(base) β solr-10.0.0-SNAPSHOT git:(SOLR-16367) β ./bin/solr export -url http://localhost:8983/solr/films -limit -1
NO: of shards : 1
..
Export complete for : films_shard1_replica_n1, docs : 1,100
Total Docs exported: 1099. Time taken: 0secs
```
--
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
[GitHub] [solr] dsmiley commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1142492764
##########
solr/core/src/java/org/apache/solr/filestore/DistribPackageStore.java:
##########
@@ -181,14 +189,17 @@ private boolean fetchFileFromNodeAndPersist(String fromNode) {
ByteBuffer metadata = null;
Map<?, ?> m = null;
- try {
- metadata =
- Utils.executeGET(
- coreContainer.getUpdateShardHandler().getDefaultHttpClient(),
- baseUrl + "/node/files" + getMetaPath(),
- Utils.newBytesConsumer((int) MAX_PKG_SIZE));
+ try (SolrClient solrClient = new Http2SolrClient.Builder(baseUrl).build()) {
+ GenericSolrRequest request =
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ "/node/files" + getMetaPath(),
+ new ModifiableSolrParams().add(CommonParams.WT, CommonParams.JSON));
Review Comment:
Looks like you were hunting for string constants wherever you could find them. In this line of code, we want "json" as a value, not as a parameter name (which it is also, actually). You can just use "json" string literal.
--
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "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
[GitHub] [solr] risdenk commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1161962963
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -804,6 +804,7 @@ public List<Option> getOptions() {
protected void runImpl(CommandLine cli) throws Exception {
String getUrl = cli.getOptionValue("get");
if (getUrl != null) {
+ getUrl = getUrl.replaceAll("\\+", "%20");
Review Comment:
`.replace` should work. no need to use regex replaceAll.
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1166774519
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -1504,26 +1260,24 @@ public static boolean safeCheckCollectionExists(String url, String collection) {
return exists;
}
- public static boolean safeCheckCoreExists(String coreStatusUrl, String coreName) {
+ @SuppressWarnings("unchecked")
+ public static boolean safeCheckCoreExists(String solrUrl, String coreName) {
boolean exists = false;
- try {
+ try (var solrClient = getSolrClient(solrUrl)) {
boolean wait = false;
final long startWaitAt = System.nanoTime();
do {
if (wait) {
final int clamPeriodForStatusPollMs = 1000;
Thread.sleep(clamPeriodForStatusPollMs);
}
- Map<String, Object> existsCheckResult = getJson(coreStatusUrl);
- @SuppressWarnings("unchecked")
- Map<String, Object> status = (Map<String, Object>) existsCheckResult.get("status");
- @SuppressWarnings("unchecked")
- Map<String, Object> coreStatus = (Map<String, Object>) status.get(coreName);
- @SuppressWarnings("unchecked")
- Map<String, Object> failureStatus =
- (Map<String, Object>) existsCheckResult.get("initFailures");
- String errorMsg = (String) failureStatus.get(coreName);
- final boolean hasName = coreStatus != null && coreStatus.containsKey(NAME);
+ NamedList<Object> existsCheckResult = CoreAdminRequest.getStatus(coreName, solrClient).getResponse();
+ NamedList<Object> status =
+ (NamedList)existsCheckResult.get("status");
+ NamedList<Object> coreStatus = (NamedList) status.get(coreName);
+ Map<String, Object> failureStatus = (Map<String, Object>) existsCheckResult.get("initFailures");
+ String errorMsg = (String)failureStatus.get(coreName);
Review Comment:
<picture><img alt="14% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/14/display.svg"></picture>
<b>*NULL_DEREFERENCE:</b>* object `failureStatus` last assigned on line 1278 could be null and is dereferenced at line 1279.
---
<details><summary>βΉοΈ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[β[π Not relevant](https://www.sonatype.com/lift-comment-rating?comment=491179443&lift_comment_rating=1)β] - [β[π Won't fix](https://www.sonatype.com/lift-comment-rating?comment=491179443&lift_comment_rating=2)β] - [β[π Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=491179443&lift_comment_rating=3)β] - [β[π Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=491179443&lift_comment_rating=4)β] - [β[π Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=491179443&lift_comment_rating=5)β]
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -1504,26 +1260,24 @@ public static boolean safeCheckCollectionExists(String url, String collection) {
return exists;
}
- public static boolean safeCheckCoreExists(String coreStatusUrl, String coreName) {
+ @SuppressWarnings("unchecked")
+ public static boolean safeCheckCoreExists(String solrUrl, String coreName) {
boolean exists = false;
- try {
+ try (var solrClient = getSolrClient(solrUrl)) {
boolean wait = false;
final long startWaitAt = System.nanoTime();
do {
if (wait) {
final int clamPeriodForStatusPollMs = 1000;
Thread.sleep(clamPeriodForStatusPollMs);
}
- Map<String, Object> existsCheckResult = getJson(coreStatusUrl);
- @SuppressWarnings("unchecked")
- Map<String, Object> status = (Map<String, Object>) existsCheckResult.get("status");
- @SuppressWarnings("unchecked")
- Map<String, Object> coreStatus = (Map<String, Object>) status.get(coreName);
- @SuppressWarnings("unchecked")
- Map<String, Object> failureStatus =
- (Map<String, Object>) existsCheckResult.get("initFailures");
- String errorMsg = (String) failureStatus.get(coreName);
- final boolean hasName = coreStatus != null && coreStatus.containsKey(NAME);
+ NamedList<Object> existsCheckResult = CoreAdminRequest.getStatus(coreName, solrClient).getResponse();
+ NamedList<Object> status =
+ (NamedList)existsCheckResult.get("status");
+ NamedList<Object> coreStatus = (NamedList) status.get(coreName);
Review Comment:
<picture><img alt="14% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/14/display.svg"></picture>
<b>*NULL_DEREFERENCE:</b>* object `status` last assigned on line 1276 could be null and is dereferenced at line 1277.
---
<details><summary>βΉοΈ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[β[π Not relevant](https://www.sonatype.com/lift-comment-rating?comment=491180267&lift_comment_rating=1)β] - [β[π Won't fix](https://www.sonatype.com/lift-comment-rating?comment=491180267&lift_comment_rating=2)β] - [β[π Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=491180267&lift_comment_rating=3)β] - [β[π Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=491180267&lift_comment_rating=4)β] - [β[π Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=491180267&lift_comment_rating=5)β]
--
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
[GitHub] [solr] dsmiley commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1161954013
##########
solr/core/src/test/org/apache/solr/util/ApiToolTest.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.util;
+
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.Arrays;
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.junit.Test;
+
+/** Unit test for SolrCLI's ApiTool */
+public class ApiToolTest extends SolrTestCase {
Review Comment:
If we have an integration test that caught this, isn't that enough?
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1504093505
I can duplicate the error with `bin/solr create -c test`, and it actually does work on `main` ;-)
```
(base) β solr-10.0.0-SNAPSHOT git:(SOLR-16367) β ./bin/solr create -c test
WARNING: Using _default configset with data driven schema functionality. NOT RECOMMENDED for production use.
To turn off: bin/solr config -c test -p 8983 -action set-user-property -property update.autoCreateFields -value false
Exception in thread "main" java.lang.NullPointerException
at org.apache.solr.client.solrj.response.CoreAdminResponse.getCoreStatus(CoreAdminResponse.java:32)
at org.apache.solr.util.SolrCLI$CreateCoreTool.runImpl(SolrCLI.java:1570)
at org.apache.solr.util.SolrCLI$CreateTool.runImpl(SolrCLI.java:1623)
at org.apache.solr.util.SolrCLI$ToolBase.runTool(SolrCLI.java:159)
at org.apache.solr.util.SolrCLI.main(SolrCLI.java:293)
```
--
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
[GitHub] [solr] bszabo97 commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1499143172
gradle check was successful, I pushed the rebased commits. I am starting the manual testing now.
--
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1035700491
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -618,6 +623,10 @@ private static boolean exceptionIsAuthRelated(Exception exc) {
&& Arrays.asList(UNAUTHORIZED.code, FORBIDDEN.code).contains(((SolrException) exc).code()));
}
+ public static SolrClient getHttpSolrClient(String baseUrl) {
+ return new Http2SolrClient.Builder(baseUrl).maxConnectionsPerHost(32).build();
Review Comment:
In the original method there were some basic settings set for the new client, and I wanted to keep as much of them as I could. This was the only one for which I have found a corresponding setting, so I set it to 32 as it was before. You can see it here: https://github.com/apache/solr/blob/5ebc3776d6aeafc9ecdd74226380f2fc9e438365/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L624
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1124750911
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -648,164 +619,30 @@ public static NamedList<Object> postJsonToSolr(
return solrClient.request(req);
}
- /** Useful when a tool just needs to send one request to Solr. */
- public static Map<String, Object> getJson(String getUrl) throws Exception {
- Map<String, Object> json = null;
- CloseableHttpClient httpClient = getHttpClient();
- try {
- json = getJson(httpClient, getUrl, 2, true);
- } finally {
- closeHttpClient(httpClient);
- }
- return json;
- }
-
- /** Utility function for sending HTTP GET request to Solr with built-in retry support. */
- public static Map<String, Object> getJson(
- HttpClient httpClient, String getUrl, int attempts, boolean isFirstAttempt) throws Exception {
- Map<String, Object> json = null;
- if (attempts >= 1) {
- try {
- json = getJson(httpClient, getUrl);
- } catch (Exception exc) {
- if (exceptionIsAuthRelated(exc)) {
- throw exc;
- }
- if (--attempts > 0 && checkCommunicationError(exc)) {
- if (!isFirstAttempt) // only show the log warning after the second attempt fails
- log.warn(
- "Request to {} failed, sleeping for 5 seconds before re-trying the request ...",
- getUrl,
- exc);
- try {
- Thread.sleep(5000);
- } catch (InterruptedException ie) {
- Thread.interrupted();
- }
-
- // retry using recursion with one-less attempt available
- json = getJson(httpClient, getUrl, attempts, false);
- } else {
- // no more attempts or error is not retry-able
- throw exc;
- }
- }
- }
-
- return json;
- }
-
- @SuppressWarnings("unchecked")
- private static class SolrResponseHandler implements ResponseHandler<Map<String, Object>> {
- @Override
- public Map<String, Object> handleResponse(HttpResponse response)
- throws ClientProtocolException, IOException {
- HttpEntity entity = response.getEntity();
- if (entity != null) {
-
- String respBody = EntityUtils.toString(entity);
- Object resp = null;
- try {
- resp = fromJSONString(respBody);
- } catch (JSONParser.ParseException pe) {
- throw new ClientProtocolException(
- "Expected JSON response from server but received: "
- + respBody
- + "\nTypically, this indicates a problem with the Solr server; check the Solr server logs for more information.");
- }
- if (resp instanceof Map) {
- return (Map<String, Object>) resp;
- } else {
- throw new ClientProtocolException(
- "Expected JSON object in response but received " + resp);
- }
- } else {
- StatusLine statusLine = response.getStatusLine();
- throw new HttpResponseException(statusLine.getStatusCode(), statusLine.getReasonPhrase());
- }
- }
- }
-
- /**
- * Utility function for sending HTTP GET request to Solr and then doing some validation of the
- * response.
- */
- public static Map<String, Object> getJson(HttpClient httpClient, String getUrl) throws Exception {
- try {
- // ensure we're requesting JSON back from Solr
- HttpGet httpGet =
- new HttpGet(
- new URIBuilder(getUrl).setParameter(CommonParams.WT, CommonParams.JSON).build());
-
- // make the request and get back a parsed JSON object
- Map<String, Object> json =
- httpClient.execute(
- httpGet,
- new SolrResponseHandler(),
- HttpClientUtil.createNewHttpClientRequestContext());
- // check the response JSON from Solr to see if it is an error
- Long statusCode = asLong("/responseHeader/status", json);
- if (statusCode != null && statusCode == -1) {
- throw new SolrServerException(
- "Unable to determine outcome of GET request to: " + getUrl + "! Response: " + json);
- } else if (statusCode != null && statusCode != 0) {
- String errMsg = asString("/error/msg", json);
- if (errMsg == null) errMsg = String.valueOf(json);
- throw new SolrServerException(errMsg);
- } else {
- // make sure no "failure" object in there either
- Object failureObj = json.get("failure");
- if (failureObj != null) {
- if (failureObj instanceof Map) {
- Object err = ((Map) failureObj).get("");
- if (err != null) throw new SolrServerException(err.toString());
- }
- throw new SolrServerException(failureObj.toString());
- }
- }
- return json;
- } catch (ClientProtocolException cpe) {
- // Currently detecting authentication by string-matching the HTTP response
- // Perhaps SolrClient should have thrown an exception itself??
- if (cpe.getMessage().contains("HTTP ERROR 401")
- || cpe.getMessage().contentEquals("HTTP ERROR 403")) {
- int code = cpe.getMessage().contains("HTTP ERROR 401") ? 401 : 403;
- throw new SolrException(
- SolrException.ErrorCode.getErrorCode(code),
- "Solr requires authentication for "
- + getUrl
- + ". Please supply valid credentials. HTTP code="
- + code);
- } else {
- throw cpe;
- }
- }
- }
-
/** Helper function for reading a String value from a JSON Object tree. */
- public static String asString(String jsonPath, Map<String, Object> json) {
+ public static String asString(String jsonPath, NamedList<Object> json) {
return pathAs(String.class, jsonPath, json);
}
/** Helper function for reading a Long value from a JSON Object tree. */
- public static Long asLong(String jsonPath, Map<String, Object> json) {
+ public static Long asLong(String jsonPath, NamedList<Object> json) {
return pathAs(Long.class, jsonPath, json);
}
/** Helper function for reading a List of Strings from a JSON Object tree. */
@SuppressWarnings("unchecked")
- public static List<String> asList(String jsonPath, Map<String, Object> json) {
+ public static List<String> asList(String jsonPath, NamedList<Object> json) {
return pathAs(List.class, jsonPath, json);
}
/** Helper function for reading a Map from a JSON Object tree. */
@SuppressWarnings("unchecked")
- public static Map<String, Object> asMap(String jsonPath, Map<String, Object> json) {
- return pathAs(Map.class, jsonPath, json);
+ public static Map<String, Object> asMap(String jsonPath, NamedList<Object> json) {
+ return pathAs(SimpleOrderedMap.class, jsonPath, json).asMap();
Review Comment:
<picture><img alt="16% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/16/display.svg"></picture>
<b>*NULL_DEREFERENCE:</b>* object returned by `pathAs(org.apache.solr.common.util.SimpleOrderedMap,jsonPath,json)` could be null and is dereferenced at line 641.
ββ <b>3 similar findings have been found in this PR</b>
<details><summary>π Expand here to view all instances of this finding</summary><br/>
<div align=\"center\">
| **File Path** | **Line Number** |
| ------------- | ------------- |
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [830](https://github.com/apache/solr/blob/6baff1ad921f3a3b81783f49e35678005b69734c/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L830) |
| solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java | [177](https://github.com/apache/solr/blob/6baff1ad921f3a3b81783f49e35678005b69734c/solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java#L177) |
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [829](https://github.com/apache/solr/blob/6baff1ad921f3a3b81783f49e35678005b69734c/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L829) |
<p><a href="https://lift.sonatype.com/results/github.com/apache/solr/01GTM3XJXXFSMNMCW4MT9QFXZG?t=Infer|NULL_DEREFERENCE" target="_blank">Visit the Lift Web Console</a> to find more details in your report.</p></div></details>
---
<details><summary>βΉοΈ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[β[π Not relevant](https://www.sonatype.com/lift-comment-rating?comment=421839049&lift_comment_rating=1)β] - [β[π Won't fix](https://www.sonatype.com/lift-comment-rating?comment=421839049&lift_comment_rating=2)β] - [β[π Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=421839049&lift_comment_rating=3)β] - [β[π Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=421839049&lift_comment_rating=4)β] - [β[π Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=421839049&lift_comment_rating=5)β]
##########
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");
+ if (statusCode == ErrorCode.NOT_FOUND.code) {
Review Comment:
<picture><img alt="6% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/6/display.svg"></picture>
<b>*NULLPTR_DEREFERENCE:</b>* `statusCode` could be null (from the call to `NamedList.get(...)` on line 279) and is dereferenced.
ββ <b>2 similar findings have been found in this PR</b>
<details><summary>π Expand here to view all instances of this finding</summary><br/>
<div align=\"center\">
| **File Path** | **Line Number** |
| ------------- | ------------- |
| solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java | [279](https://github.com/apache/solr/blob/6baff1ad921f3a3b81783f49e35678005b69734c/solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java#L279) |
| solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java | [177](https://github.com/apache/solr/blob/6baff1ad921f3a3b81783f49e35678005b69734c/solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java#L177) |
<p><a href="https://lift.sonatype.com/results/github.com/apache/solr/01GTM3XJXXFSMNMCW4MT9QFXZG?t=Infer|NULLPTR_DEREFERENCE" target="_blank">Visit the Lift Web Console</a> to find more details in your report.</p></div></details>
---
<details><summary>βΉοΈ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[β[π Not relevant](https://www.sonatype.com/lift-comment-rating?comment=421839576&lift_comment_rating=1)β] - [β[π Won't fix](https://www.sonatype.com/lift-comment-rating?comment=421839576&lift_comment_rating=2)β] - [β[π Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=421839576&lift_comment_rating=3)β] - [β[π Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=421839576&lift_comment_rating=4)β] - [β[π Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=421839576&lift_comment_rating=5)β]
--
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1145069656
##########
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:
Sorry it seems like I forgot to answer this. The reason why we need http 1.1 here is because actually we are not talking to Solr here, but the repository server. I am not really sure at this point if we should use a Solr client for this communication at all, seems quite silly.
Although I have tried an implementation where I was using a plain jetty client here, instead of Solr but it came out rather ugly and a bit complicated with all the random parameters I had to set a default for.
On the other hand I tried changing the test (PackageManagerCLITest) to create an http 2 compatible web server for the repository server [here](https://github.com/apache/solr/blob/11253f05cfb31f9fb945c831d8889b3db1e607f1/solr/core/src/test/org/apache/solr/cloud/PackageManagerCLITest.java#L239-L256) but then I realised that probably repository servers in reality are not prepared to be compatible with http2 so just changing the test did not seem a viable option.
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1507358525
I'm going to run teh full test set on my laptop while I go to the dentist today... If it passes, I'm planning on merging ;-).
--
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
[GitHub] [solr] bszabo97 commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1504797021
Thank you @epugh for looking into this!
I have reviewed the test failures and uploaded a fix for the solr core creation.
The failures connected to restart were the consequences of a user error, I assumed with a plain `restart` command Solr would restart in the same mode as in which it was started in the first place, but turns out I stopped a Solr cloud instance but started a standalone one. Adding `-c` after the restart command solved all the problems.
So as of now I understand that all the failures found by manual testing is either resolved or not related to this PR.
--
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
[GitHub] [solr] bszabo97 commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1497321076
Thank you @epugh for having a look at this!
I was just about to do some manual testing and rebase this PR to the current master, unfortunately I had lots of other stuff on my plate recently and was not able to proceed with this. I hope I can do it tomorrow and also look into you recent review.
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1328326575
@risdenk @dsmiley Would moving to jetty http2 client for the CLI be blocked by the same issue that is preventing that shift in the API code? I'd love to see all of this good work make it in sooner versus later ;-)
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1481190065
I think we should try to merge this PR before #1476 since that moves a lot of files around ;-)......
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1476645596
I wonder if the DistribPackageStore should wait for another time/PR?
--
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
[GitHub] [solr] bszabo97 commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1477557782
Do you recon I should revert the changes already made to the `DistribPackageStore` class? Initially I changed the client to jetty because the test was failing with json type `writeRaw` function, but now that I added back the filestream version it succeeds with the apache client as well.
--
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
[GitHub] [solr] epugh commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1162638083
##########
solr/core/src/test/org/apache/solr/util/ApiToolTest.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.util;
+
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.Arrays;
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.junit.Test;
+
+/** Unit test for SolrCLI's ApiTool */
+public class ApiToolTest extends SolrTestCase {
Review Comment:
I'd like unit tests for all the tools, for all the reasons that you use unit tests in the first place ;-).
--
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
[GitHub] [solr] epugh commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1158359743
##########
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:
@dsmiley does this respnose make sense to you? Do we need to change things up more? Or is adding a comment sufficient?
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1502024753
I ran `./gradlew integrationTests` on both `main` and this branch. They all pass on main. on this branch I get these two failures:
```
not ok 30 SQL Module
not ok 34 start solr with ssl
```
The others all pass...
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1478337934
Let me know when you are ready for some testing ;-) I think this probably warrants some manual testing.
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1509747726
Tests passed!
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1509748027
argh, forgot changes.txt. Will add that when jenkins main test pass...
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1166827702
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -1504,26 +1260,25 @@ public static boolean safeCheckCollectionExists(String url, String collection) {
return exists;
}
- public static boolean safeCheckCoreExists(String coreStatusUrl, String coreName) {
+ @SuppressWarnings("unchecked")
+ public static boolean safeCheckCoreExists(String solrUrl, String coreName) {
boolean exists = false;
- try {
+ try (var solrClient = getSolrClient(solrUrl)) {
boolean wait = false;
final long startWaitAt = System.nanoTime();
do {
if (wait) {
final int clamPeriodForStatusPollMs = 1000;
Thread.sleep(clamPeriodForStatusPollMs);
}
- Map<String, Object> existsCheckResult = getJson(coreStatusUrl);
- @SuppressWarnings("unchecked")
- Map<String, Object> status = (Map<String, Object>) existsCheckResult.get("status");
- @SuppressWarnings("unchecked")
- Map<String, Object> coreStatus = (Map<String, Object>) status.get(coreName);
- @SuppressWarnings("unchecked")
+ NamedList<Object> existsCheckResult =
+ CoreAdminRequest.getStatus(coreName, solrClient).getResponse();
+ NamedList<Object> status = (NamedList) existsCheckResult.get("status");
+ NamedList<Object> coreStatus = (NamedList) status.get(coreName);
Map<String, Object> failureStatus =
(Map<String, Object>) existsCheckResult.get("initFailures");
String errorMsg = (String) failureStatus.get(coreName);
Review Comment:
<picture><img alt="14% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/14/display.svg"></picture>
<b>*NULL_DEREFERENCE:</b>* object `failureStatus` last assigned on line 1279 could be null and is dereferenced at line 1280.
---
<details><summary>βΉοΈ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[β[π Not relevant](https://www.sonatype.com/lift-comment-rating?comment=491234077&lift_comment_rating=1)β] - [β[π Won't fix](https://www.sonatype.com/lift-comment-rating?comment=491234077&lift_comment_rating=2)β] - [β[π Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=491234077&lift_comment_rating=3)β] - [β[π Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=491234077&lift_comment_rating=4)β] - [β[π Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=491234077&lift_comment_rating=5)β]
--
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
[GitHub] [solr] dsmiley commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1319115125
Quick comment -- a variable named `http2SolrClient` is overly specific. I recommend simply `solr` or `client` or at most `solrClient`. That it's HTTP, and possibly 2.2 protocol version is entirely irrelevant.
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1331888076
@bszabo97 when you are ready on this PR, ping me and I can do some of that manual testing with BASIC auth etc...
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1033005921
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -968,38 +978,39 @@ public Map<String, Object> getStatus(String solrUrl) throws Exception {
if (!solrUrl.endsWith("/")) solrUrl += "/";
- String systemInfoUrl = solrUrl + "admin/info/system";
- CloseableHttpClient httpClient = getHttpClient();
- try {
- // hit Solr to get system info
- Map<String, Object> systemInfo = getJson(httpClient, systemInfoUrl, 2, true);
+ try (var solrClient = getHttpSolrClient(solrUrl)) {
+ NamedList<Object> systemInfo =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ CommonParams.SYSTEM_INFO_PATH,
+ new ModifiableSolrParams()));
// convert raw JSON into user-friendly output
- status = reportStatus(solrUrl, systemInfo, httpClient);
- } finally {
- closeHttpClient(httpClient);
+ status = reportStatus(systemInfo, solrClient);
}
return status;
}
- public Map<String, Object> reportStatus(
- String solrUrl, Map<String, Object> info, HttpClient httpClient) throws Exception {
+ public Map<String, Object> reportStatus(NamedList<Object> info, SolrClient solrClient)
+ throws Exception {
Map<String, Object> status = new LinkedHashMap<>();
String solrHome = (String) info.get("solr_home");
status.put("solr_home", solrHome != null ? solrHome : "?");
- status.put("version", asString("/lucene/solr-impl-version", info));
- status.put("startTime", asString("/jvm/jmx/startTime", info));
- status.put("uptime", uptime(asLong("/jvm/jmx/upTimeMS", info)));
+ status.put("version", ((NamedList) info.get("lucene")).get("solr-impl-version"));
- String usedMemory = asString("/jvm/memory/used", info);
- String totalMemory = asString("/jvm/memory/total", info);
+ @SuppressWarnings("unchecked")
+ NamedList<Object> jvm = (NamedList<Object>) info.get("jvm");
+ status.put("startTime", ((NamedList) jvm.get("jmx")).get("startTime"));
+ status.put("uptime", uptime((Long) ((NamedList) jvm.get("jmx")).get("upTimeMS")));
Review Comment:
π¬ 9 similar findings have been found in this PR
---
*NULL_DEREFERENCE:* object returned by `jvm.get("jmx").get("upTimeMS")` could be null and is dereferenced at line 1006.
---
<details><summary><b>π Expand here to view all instances of this finding</b></summary><br/>
<div align="center">
| **File Path** | **Line Number** |
| ------------- | ------------- |
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1005](https://github.com/bszabo97/solr/blob/9467bdc7e12619a1c4f4505fef056c39a2304f01/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1005)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1007](https://github.com/bszabo97/solr/blob/9467bdc7e12619a1c4f4505fef056c39a2304f01/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1007)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1033](https://github.com/bszabo97/solr/blob/9467bdc7e12619a1c4f4505fef056c39a2304f01/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1033)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1008](https://github.com/bszabo97/solr/blob/9467bdc7e12619a1c4f4505fef056c39a2304f01/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1008)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1032](https://github.com/bszabo97/solr/blob/9467bdc7e12619a1c4f4505fef056c39a2304f01/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1032)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1035](https://github.com/bszabo97/solr/blob/9467bdc7e12619a1c4f4505fef056c39a2304f01/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1035)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1005](https://github.com/bszabo97/solr/blob/9467bdc7e12619a1c4f4505fef056c39a2304f01/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1005)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1001](https://github.com/bszabo97/solr/blob/9467bdc7e12619a1c4f4505fef056c39a2304f01/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1001)|
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [1006](https://github.com/bszabo97/solr/blob/9467bdc7e12619a1c4f4505fef056c39a2304f01/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L1006)|
<p><a href="https://lift.sonatype.com/results/github.com/apache/solr/01GJXA8DNEM900MB9GAZATFZNB?t=Infer|NULL_DEREFERENCE" target="_blank">Visit the Lift Web Console</a> to find more details in your report.</p></div></details>
---
<details><summary><b>βΉοΈ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [π Not relevant](https://www.sonatype.com/lift-comment-rating?comment=355782706&lift_comment_rating=1) ] - [ [π Won't fix](https://www.sonatype.com/lift-comment-rating?comment=355782706&lift_comment_rating=2) ] - [ [π Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=355782706&lift_comment_rating=3) ] - [ [π Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=355782706&lift_comment_rating=4) ] - [ [π Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=355782706&lift_comment_rating=5) ]
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1328326295
@bszabo97 I hope it's okay, I pushed up some changes... I was curious if we could use `SolrCloudClient` or `SolrClient` in more places than we had, and it appears yes....
--
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1145069656
##########
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:
Sorry it seems like I forgot to answer this. The reason why we need http 1.1 here is because actually we are not talking to Solr here, but the repository server. I am not really sure at this point if we should use a Solr client for this communication at all, seems quite silly.
Although I have tried an implementation where I was using a plain jetty client here, instead of a Solr client but it came out rather ugly and a bit complicated with all the "random" parameters I had to set a default for - I stole these default Http2SolrClient, where we initialise the jetty client so in the end I got mostly the same jetty client.
On the other hand I tried changing the test (PackageManagerCLITest) to create an http 2 compatible web server for the repository server [here](https://github.com/apache/solr/blob/11253f05cfb31f9fb945c831d8889b3db1e607f1/solr/core/src/test/org/apache/solr/cloud/PackageManagerCLITest.java#L239-L256) but then I realised that probably repository servers in reality are not prepared to be compatible with http2 so just changing the test did not seem a viable option.
--
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1126325809
##########
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:
Without the correct encoding specified here I got an exception when requesting the manifest.json file, since the SHA did not match the expected.
I tried to set the content type for the response to contain the encoding but when I dug deeper it turned out the we get if from the BinaryResponseParser and I was more afraid to change it there since it would affect much more then just the manifest.json so I added this if here.
The other reason behind this was the we make use of this `FALLBACK_CHARSET` in many places in this class if we do not have an encoding specified, so I figured it would not mess up things if I add it here as well.
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1143539842
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/JsonMapResponseParser.java:
##########
@@ -0,0 +1,53 @@
+package org.apache.solr.client.solrj.impl;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.io.Reader;
+import java.util.Map;
+import org.apache.solr.client.solrj.ResponseParser;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.util.NamedList;
+import org.noggit.JSONParser;
+import org.noggit.ObjectBuilder;
+
+/** ResponseParser for JsonMaps. */
+public class JsonMapResponseParser extends ResponseParser {
+ @Override
+ public String getWriterType() {
+ return "json";
+ }
+
+ @Override
+ @SuppressWarnings({"unchecked"})
+ public NamedList<Object> processResponse(InputStream body, String encoding) {
+ @SuppressWarnings({"rawtypes"})
+ Map map = null;
+ try {
+ ObjectBuilder builder =
+ new ObjectBuilder(
+ new JSONParser(new InputStreamReader(body, encoding == null ? "UTF-8" : encoding)));
+ map = (Map) builder.getObject();
Review Comment:
<picture><img alt="11% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/11/display.svg"></picture>
<b>*RESOURCE_LEAK:</b>* resource of type `java.io.InputStreamReader` acquired by call to `InputStreamReader(...)` at line 29 is not released after line 30.
---
<details><summary>βΉοΈ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[β[π Not relevant](https://www.sonatype.com/lift-comment-rating?comment=445848149&lift_comment_rating=1)β] - [β[π Won't fix](https://www.sonatype.com/lift-comment-rating?comment=445848149&lift_comment_rating=2)β] - [β[π Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=445848149&lift_comment_rating=3)β] - [β[π Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=445848149&lift_comment_rating=4)β] - [β[π Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=445848149&lift_comment_rating=5)β]
--
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
[GitHub] [solr] dsmiley commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1476674958
> I wonder if the DistribPackageStore should wait for another time/PR?
+1
Those scope of this issue is just CLI tools so definitely keep this already big PR focused on that please.
--
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
[GitHub] [solr] dsmiley commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1159085529
##########
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:
(pending)
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1502102973
@bszabo97 I hope it's okay, I pushed up a fix, though I don't know that how I fixed it was the best way.. Would your thoughts on if there is a cleaner way ;-)
--
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
[GitHub] [solr] dsmiley commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1159203930
##########
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:
Seems you fixed it or something -- latest state is clean.
--
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
[GitHub] [solr] bszabo97 commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1510826469
Thank you @epugh for looking after this PR, it was a lot of help! Also thanks to @dsmiley and @risdenk for the reviews!
--
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
[GitHub] [solr] HoustonPutman commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1499209989
@bszabo97 in the future, its much better to merge `main` into your branch then to do a rebase. That way we keep the history of all comments.
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1503828534
The BATS test `test_ssl.bats` passes if I update this branch to the latest in `main`, so the fact that it fails on this PR is just fine:
```
> Task :solr:packaging:integrationTests
1..1
ok 1 start solr with ssl
```
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1504053171
> @bszabo97 I was able to run export just fine:
>
> ```
> (base) β solr-10.0.0-SNAPSHOT git:(SOLR-16367) β ./bin/solr export -url http://localhost:8983/solr/films
> NO: of shards : 1
> .
> Total Docs exported: 100. Time taken: 0secs
> ```
>
> However, with a larger number output, I get a different count:
>
> ```
> (base) β solr-10.0.0-SNAPSHOT git:(SOLR-16367) β ./bin/solr export -url http://localhost:8983/solr/films -limit -1
> NO: of shards : 1
> ..
> Export complete for : films_shard1_replica_n1, docs : 1,100
>
> Total Docs exported: 1099. Time taken: 0secs
> ```
Okay, the miscount of 1099 also shows up in `main` so it's not related to this PR ;-)
--
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
[GitHub] [solr] dsmiley commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1159085529
##########
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:
(pending)
--
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
[GitHub] [solr] epugh merged pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh merged PR #1182:
URL: https://github.com/apache/solr/pull/1182
--
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
[GitHub] [solr] bszabo97 commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1500206592
I have exported my notes about the testing that I have executed, in the beginning there are the failed tests which I will have to look into, later there are all the tests with the command and the output of the command.
The tests that failed are:
* Export prints 0 documents were exported although there was 1. The exported file is correct, though.
* Restart command messes up something around the zookeeper connection in cloud mode. Lots of functionalities are not working correctly. Stop + Start is working fine interestingly.
* Create core in standalone mode return with error, core is visible but not functional.
[Manual testing for SolrCLI.pdf](https://github.com/apache/solr/files/11178548/Manual.testing.for.SolrCLI.pdf)
--
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1127688733
##########
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:
I may be mistaken but I think the `getQuery` method returns the decoded version of the query. This is what I found in the javadoc:
```
Returns the decoded query component of this URI.
```
Is this what you meant should be decoded or did I misunderstand something?
--
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
[GitHub] [solr] bszabo97 commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
bszabo97 commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1354572598
I have added some more commits where I was trying to migrate from apache client to the jetty client, but right now I am a bit stuck and I would need a little guidance.
With the current version (at least) one tests is failing, which can be reproduced with
```
./gradlew :solr:core:test --tests "org.apache.solr.cloud.PackageManagerCLITest.testPackageManager" -Ptests.jvms=6 "-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=120m" -Ptests.seed=6DCD7855C98F8100 -Ptests.file.encoding=UTF-8
```
I get the following error in this test:
```
org.apache.solr.common.SolrException: org.apache.solr.client.solrj.impl.BaseHttpSolrClient$RemoteSolrException: Error from server at https://127.0.0.1:60536: Invalid version (expected 2, but 123) or the data in not in
'javabin' format
2> at org.apache.solr.packagemanager.PackageManager.fetchInstalledPackageInstances(PackageManager.java:223)
2> at org.apache.solr.util.PackageTool.runImpl(PackageTool.java:108)
2> at org.apache.solr.util.SolrCLI$ToolBase.runTool(SolrCLI.java:174)
2> at org.apache.solr.cloud.PackageManagerCLITest.run(PackageManagerCLITest.java:219)
2> at org.apache.solr.cloud.PackageManagerCLITest.testPackageManager(PackageManagerCLITest.java:103)
...
```
Looking at the logs of this test on the main branch I can see that the request with which we get the manifest file differs in the parameters. On the main branch it looks like this:
```
5384 INFO (qtp747046555-89) [n:127.0.0.1:61067_solr] o.a.s.s.HttpSolrCall [admin] webapp=null path=/node/files/package/question-answer/1.0.0/manifest.json params={} status=0 QTime=1
```
On my branch it is like this:
```
5209 INFO (qtp1404573452-50) [n:127.0.0.1:60536_solr] o.a.s.s.HttpSolrCall [admin] webapp=null path=/node/files/package/question-answer/1.0.0/manifest.json params={wt=javabin&version=2} status=0 QTime=0
```
I was not able to get rid of these parameters since I think they are there by default in every solr request(?)
I also tried implementing this with the V2Request but that way I get a different error which is:
```
2> org.apache.solr.client.solrj.impl.BaseHttpSolrClient$RemoteSolrException: Error from server at https://127.0.0.1:65494: Expected mime type in [application/octet-stream, application/vnd.apache.solr.javabin] but got text/html. <html>
2> <head>
2> <meta http-equiv="Content-Type" content="text/html;charset=ISO-8859-1"/>
2> <title>Error 404 Not Found</title>
2> </head>
2> <body><h2>HTTP ERROR 404 Not Found</h2>
2> <table>
2> <tr><th>URI:</th><td>/____v2/api/cluster/package</td></tr>
2> <tr><th>STATUS:</th><td>404</td></tr>
2> <tr><th>MESSAGE:</th><td>Not Found</td></tr>
2> <tr><th>SERVLET:</th><td>-</td></tr>
2> </table>
2> <hr/><a href="https://eclipse.org/jetty">Powered by Jetty:// 9.4.48.v20220622</a><hr/>
2>
2> </body>
2> </html>
2>
2> at org.apache.solr.client.solrj.impl.Http2SolrClient.processErrorsAndResponse(Http2SolrClient.java:796)
2> at org.apache.solr.client.solrj.impl.Http2SolrClient.processErrorsAndResponse(Http2SolrClient.java:509)
2> at org.apache.solr.client.solrj.impl.Http2SolrClient.request(Http2SolrClient.java:468)
2> at org.apache.solr.client.solrj.SolrRequest.process(SolrRequest.java:234)
2> at org.apache.solr.client.solrj.SolrRequest.process(SolrRequest.java:249)
```
(Actually I get a similar message with every V2Request in this class so I had to change quite a lot of them to be GenericSolrRequests)
Can somebody take a look at this problem and provide some guidance for me? Thank you in advance and sorry for the long comment.
--
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1035705543
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -968,38 +978,39 @@ public Map<String, Object> getStatus(String solrUrl) throws Exception {
if (!solrUrl.endsWith("/")) solrUrl += "/";
- String systemInfoUrl = solrUrl + "admin/info/system";
- CloseableHttpClient httpClient = getHttpClient();
- try {
- // hit Solr to get system info
- Map<String, Object> systemInfo = getJson(httpClient, systemInfoUrl, 2, true);
+ try (var solrClient = getHttpSolrClient(solrUrl)) {
+ NamedList<Object> systemInfo =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ CommonParams.SYSTEM_INFO_PATH,
+ new ModifiableSolrParams()));
// convert raw JSON into user-friendly output
- status = reportStatus(solrUrl, systemInfo, httpClient);
- } finally {
- closeHttpClient(httpClient);
+ status = reportStatus(systemInfo, solrClient);
}
return status;
}
- public Map<String, Object> reportStatus(
- String solrUrl, Map<String, Object> info, HttpClient httpClient) throws Exception {
+ public Map<String, Object> reportStatus(NamedList<Object> info, SolrClient solrClient)
+ throws Exception {
Map<String, Object> status = new LinkedHashMap<>();
String solrHome = (String) info.get("solr_home");
status.put("solr_home", solrHome != null ? solrHome : "?");
- status.put("version", asString("/lucene/solr-impl-version", info));
- status.put("startTime", asString("/jvm/jmx/startTime", info));
- status.put("uptime", uptime(asLong("/jvm/jmx/upTimeMS", info)));
+ status.put("version", ((NamedList) info.get("lucene")).get("solr-impl-version"));
- String usedMemory = asString("/jvm/memory/used", info);
- String totalMemory = asString("/jvm/memory/total", info);
+ @SuppressWarnings("unchecked")
+ NamedList<Object> jvm = (NamedList<Object>) info.get("jvm");
+ status.put("startTime", ((NamedList) jvm.get("jmx")).get("startTime"));
+ status.put("uptime", uptime((Long) ((NamedList) jvm.get("jmx")).get("upTimeMS")));
+ String usedMemory = (String) ((NamedList) jvm.get("memory")).get("used");
+ String totalMemory = (String) ((NamedList) jvm.get("memory")).get("total");
status.put("memory", usedMemory + " of " + totalMemory);
- // if this is a Solr in solrcloud mode, gather some basic cluster info
Review Comment:
I was not aware I deleted this comment. Looking at it, it is kind of obvious, maybe a bit redundant, but if you think it has a purpose we can put it back.
--
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
[GitHub] [solr] dsmiley commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1035468060
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -618,6 +623,10 @@ private static boolean exceptionIsAuthRelated(Exception exc) {
&& Arrays.asList(UNAUTHORIZED.code, FORBIDDEN.code).contains(((SolrException) exc).code()));
}
+ public static SolrClient getHttpSolrClient(String baseUrl) {
+ return new Http2SolrClient.Builder(baseUrl).maxConnectionsPerHost(32).build();
Review Comment:
why set this to 32?
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -968,38 +978,39 @@ public Map<String, Object> getStatus(String solrUrl) throws Exception {
if (!solrUrl.endsWith("/")) solrUrl += "/";
- String systemInfoUrl = solrUrl + "admin/info/system";
- CloseableHttpClient httpClient = getHttpClient();
- try {
- // hit Solr to get system info
- Map<String, Object> systemInfo = getJson(httpClient, systemInfoUrl, 2, true);
+ try (var solrClient = getHttpSolrClient(solrUrl)) {
+ NamedList<Object> systemInfo =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ CommonParams.SYSTEM_INFO_PATH,
+ new ModifiableSolrParams()));
// convert raw JSON into user-friendly output
- status = reportStatus(solrUrl, systemInfo, httpClient);
- } finally {
- closeHttpClient(httpClient);
+ status = reportStatus(systemInfo, solrClient);
}
return status;
}
- public Map<String, Object> reportStatus(
- String solrUrl, Map<String, Object> info, HttpClient httpClient) throws Exception {
+ public Map<String, Object> reportStatus(NamedList<Object> info, SolrClient solrClient)
+ throws Exception {
Map<String, Object> status = new LinkedHashMap<>();
String solrHome = (String) info.get("solr_home");
status.put("solr_home", solrHome != null ? solrHome : "?");
- status.put("version", asString("/lucene/solr-impl-version", info));
- status.put("startTime", asString("/jvm/jmx/startTime", info));
- status.put("uptime", uptime(asLong("/jvm/jmx/upTimeMS", info)));
+ status.put("version", ((NamedList) info.get("lucene")).get("solr-impl-version"));
- String usedMemory = asString("/jvm/memory/used", info);
- String totalMemory = asString("/jvm/memory/total", info);
+ @SuppressWarnings("unchecked")
+ NamedList<Object> jvm = (NamedList<Object>) info.get("jvm");
+ status.put("startTime", ((NamedList) jvm.get("jmx")).get("startTime"));
+ status.put("uptime", uptime((Long) ((NamedList) jvm.get("jmx")).get("upTimeMS")));
Review Comment:
I don't see why this method was modified regarding extraction from "info".
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -968,38 +978,39 @@ public Map<String, Object> getStatus(String solrUrl) throws Exception {
if (!solrUrl.endsWith("/")) solrUrl += "/";
- String systemInfoUrl = solrUrl + "admin/info/system";
- CloseableHttpClient httpClient = getHttpClient();
- try {
- // hit Solr to get system info
- Map<String, Object> systemInfo = getJson(httpClient, systemInfoUrl, 2, true);
+ try (var solrClient = getHttpSolrClient(solrUrl)) {
+ NamedList<Object> systemInfo =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ CommonParams.SYSTEM_INFO_PATH,
+ new ModifiableSolrParams()));
Review Comment:
Very glad to see we are simply using SolrJ rather than lower level HTTP requests
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -968,38 +978,39 @@ public Map<String, Object> getStatus(String solrUrl) throws Exception {
if (!solrUrl.endsWith("/")) solrUrl += "/";
- String systemInfoUrl = solrUrl + "admin/info/system";
- CloseableHttpClient httpClient = getHttpClient();
- try {
- // hit Solr to get system info
- Map<String, Object> systemInfo = getJson(httpClient, systemInfoUrl, 2, true);
+ try (var solrClient = getHttpSolrClient(solrUrl)) {
+ NamedList<Object> systemInfo =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ CommonParams.SYSTEM_INFO_PATH,
+ new ModifiableSolrParams()));
// convert raw JSON into user-friendly output
- status = reportStatus(solrUrl, systemInfo, httpClient);
- } finally {
- closeHttpClient(httpClient);
+ status = reportStatus(systemInfo, solrClient);
}
return status;
}
- public Map<String, Object> reportStatus(
- String solrUrl, Map<String, Object> info, HttpClient httpClient) throws Exception {
+ public Map<String, Object> reportStatus(NamedList<Object> info, SolrClient solrClient)
+ throws Exception {
Map<String, Object> status = new LinkedHashMap<>();
String solrHome = (String) info.get("solr_home");
status.put("solr_home", solrHome != null ? solrHome : "?");
- status.put("version", asString("/lucene/solr-impl-version", info));
- status.put("startTime", asString("/jvm/jmx/startTime", info));
- status.put("uptime", uptime(asLong("/jvm/jmx/upTimeMS", info)));
+ status.put("version", ((NamedList) info.get("lucene")).get("solr-impl-version"));
- String usedMemory = asString("/jvm/memory/used", info);
- String totalMemory = asString("/jvm/memory/total", info);
+ @SuppressWarnings("unchecked")
+ NamedList<Object> jvm = (NamedList<Object>) info.get("jvm");
+ status.put("startTime", ((NamedList) jvm.get("jmx")).get("startTime"));
+ status.put("uptime", uptime((Long) ((NamedList) jvm.get("jmx")).get("upTimeMS")));
+ String usedMemory = (String) ((NamedList) jvm.get("memory")).get("used");
+ String totalMemory = (String) ((NamedList) jvm.get("memory")).get("total");
status.put("memory", usedMemory + " of " + totalMemory);
- // if this is a Solr in solrcloud mode, gather some basic cluster info
Review Comment:
I guess you deleted because, why state the obvious?
--
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
[GitHub] [solr] bszabo97 commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1508375380
Woow somehow having a broken CreateCoreTool did not cause this test to fail, but since the safeCheckCoreExists was also broken when the CreateCoreTool got fixed it broke the test.
Nice catch @epugh thank you! I uploaded a fix, now this test is successful for me and I am running all the tests just now
--
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
[GitHub] [solr] epugh commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1166745952
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -1271,14 +1271,13 @@ public static boolean safeCheckCoreExists(String solrUrl, String coreName) {
final int clamPeriodForStatusPollMs = 1000;
Thread.sleep(clamPeriodForStatusPollMs);
}
- NamedList<Object> existsCheckResult =
- CoreAdminRequest.getStatus(coreName, solrClient).getCoreStatus(coreName);
- Map<String, Object> status = ((NamedList) existsCheckResult.get("status")).asMap();
- Map<String, Object> coreStatus = ((NamedList) status.get(coreName)).asMap();
- Map<String, Object> failureStatus =
- ((NamedList) existsCheckResult.get("initFailures")).asMap();
- String errorMsg = (String) failureStatus.get(coreName);
- final boolean hasName = coreStatus != null && coreStatus.containsKey(NAME);
+ NamedList<Object> existsCheckResult = CoreAdminRequest.getStatus(coreName, solrClient).getResponse();
Review Comment:
I hate where we have all these .asMaps and NamedLists etc.. it feels like things are just so brittle and easy to break ;-)
--
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
[GitHub] [solr] epugh commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1505106633
Okay, I'm going to ping @risdenk and @dsmiley for another review, now that the tests are all passing, and the manual testing has happened (and is now all passing). I'd like to merge this in the next day or so ;-).
--
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
[GitHub] [solr] epugh commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1158360488
##########
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:
@bszabo97 there are other places where we create a straigh up httpclient and do a get.. Maybe we should just do that here?
--
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
[GitHub] [solr] dsmiley commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1498411973
Tests pass.
--
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
[GitHub] [solr] epugh commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1059415280
##########
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"});
+ NamedList<Object> result =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ PackageUtils.getCollectionParamsPath(collection) + "/PKG_VERSIONS",
+ new ModifiableSolrParams(paramsMap)));
packages =
(Map<String, String>)
result._get("/response/params/PKG_VERSIONS", Collections.emptyMap());
- } catch (PathNotFoundException ex) {
- // Don't worry if PKG_VERSION wasn't found. It just means this collection was never touched by
- // the package manager.
Review Comment:
Do we need to keep this comment? Did we lose the fact that it's okay to not have a PKG_VERSIONS?
--
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1028346326
##########
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:
Great suggestion, I was not familiar with this class! I have changed the implementation to use URI, if you could have a check if it looks better now I would be grateful (I still had to use some "string magic" for which I could not figure a nicer solution).
--
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1028344331
##########
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:
Thank you for the suggestion, I have changed to this model wherever I used it like here.
--
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
[GitHub] [solr] epugh commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1025532208
##########
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:
agreed... @risdenk I wonder if there is a way to add a rule that would ensure we all follow the try/with/resources model?
--
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by GitBox <gi...@apache.org>.
bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1035704235
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -968,38 +978,39 @@ public Map<String, Object> getStatus(String solrUrl) throws Exception {
if (!solrUrl.endsWith("/")) solrUrl += "/";
- String systemInfoUrl = solrUrl + "admin/info/system";
- CloseableHttpClient httpClient = getHttpClient();
- try {
- // hit Solr to get system info
- Map<String, Object> systemInfo = getJson(httpClient, systemInfoUrl, 2, true);
+ try (var solrClient = getHttpSolrClient(solrUrl)) {
+ NamedList<Object> systemInfo =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ CommonParams.SYSTEM_INFO_PATH,
+ new ModifiableSolrParams()));
// convert raw JSON into user-friendly output
- status = reportStatus(solrUrl, systemInfo, httpClient);
- } finally {
- closeHttpClient(httpClient);
+ status = reportStatus(systemInfo, solrClient);
}
return status;
}
- public Map<String, Object> reportStatus(
- String solrUrl, Map<String, Object> info, HttpClient httpClient) throws Exception {
+ public Map<String, Object> reportStatus(NamedList<Object> info, SolrClient solrClient)
+ throws Exception {
Map<String, Object> status = new LinkedHashMap<>();
String solrHome = (String) info.get("solr_home");
status.put("solr_home", solrHome != null ? solrHome : "?");
- status.put("version", asString("/lucene/solr-impl-version", info));
- status.put("startTime", asString("/jvm/jmx/startTime", info));
- status.put("uptime", uptime(asLong("/jvm/jmx/upTimeMS", info)));
+ status.put("version", ((NamedList) info.get("lucene")).get("solr-impl-version"));
- String usedMemory = asString("/jvm/memory/used", info);
- String totalMemory = asString("/jvm/memory/total", info);
+ @SuppressWarnings("unchecked")
+ NamedList<Object> jvm = (NamedList<Object>) info.get("jvm");
+ status.put("startTime", ((NamedList) jvm.get("jmx")).get("startTime"));
+ status.put("uptime", uptime((Long) ((NamedList) jvm.get("jmx")).get("upTimeMS")));
Review Comment:
The original method got a Map as a parameter and these functions (asString, asLong, etc.) work well with Maps, but since we now get a NamedList I thought that getting the information from it as we usually get info from a NamedList would be the best approach.
Other than that simply leaving it as it was, just adding NamedList instead of a Map as a parameter was not working, so the methods would need some modifications in order to make them work with NamedLists as well.
--
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
[GitHub] [solr] HoustonPutman commented on pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #1182:
URL: https://github.com/apache/solr/pull/1182#issuecomment-1430027333
> @bszabo97 when you are ready on this PR, ping me and I can do some of that manual testing with BASIC auth etc...
Could we add an integration test for this?
--
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1126287118
##########
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:
When not specifying the http1.1 as protocol I got an `java.io.IOException: frame_size_error/invalid_frame_length` exception which I figured is because the response's format does not support http2. Using http1 works well.
Do you think making a change in the response so that we can use http2 here would be a neater solution?
--
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
[GitHub] [solr] epugh commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1127911487
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java:
##########
@@ -230,15 +229,12 @@ public List<SolrPackageInstance> fetchInstalledPackageInstances() throws SolrExc
public Map<String, SolrPackageInstance> getPackagesDeployed(String collection) {
Map<String, String> packages = null;
try {
- Map<String, String[]> paramsMap = new LinkedHashMap<>();
- paramsMap.put("omitHeader", new String[] {"true"});
- paramsMap.put("wt", new String[] {"javabin"});
NamedList<Object> result =
solrClient.request(
new GenericSolrRequest(
SolrRequest.METHOD.GET,
PackageUtils.getCollectionParamsPath(collection) + "/PKG_VERSIONS",
- new ModifiableSolrParams(paramsMap)));
+ new ModifiableSolrParams().add("omitHeader", "true").add("wt", "javabin")));
Review Comment:
Nicer!
--
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
[GitHub] [solr] bszabo97 commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
Posted by "bszabo97 (via GitHub)" <gi...@apache.org>.
bszabo97 commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1145083082
##########
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"});
+ NamedList<Object> result =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ PackageUtils.getCollectionParamsPath(collection) + "/PKG_VERSIONS",
+ new ModifiableSolrParams(paramsMap)));
packages =
(Map<String, String>)
result._get("/response/params/PKG_VERSIONS", Collections.emptyMap());
- } catch (PathNotFoundException ex) {
- // Don't worry if PKG_VERSION wasn't found. It just means this collection was never touched by
- // the package manager.
Review Comment:
Valid point, I added it back, 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