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/01 18:57:49 UTC
[GitHub] [solr] epugh opened a new pull request, #1158: SOLR-16368: experiment with builder to simplify client creation
epugh opened a new pull request, #1158:
URL: https://github.com/apache/solr/pull/1158
https://issues.apache.org/jira/browse/SOLR-16368
# Description
Part of working on reducing the use of legacy `HttpSolrClient` in the tests everywhere is seeing if mutating the client can be reduced by embracing the Builder pattern.
# Solution
Use the Builder where possible.
I couldn't figure out how to untangle the logic in `TestRandomFlRTGCloud` and would love some eyes on that one!
# Tests
ran the tests
# Checklist
Please review the following and check all that apply:
- [ ] 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.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] 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)
- [ ] I have developed this patch against the `main` branch.
- [ ] 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] dsmiley commented on pull request #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1302466510
While you are changing these lines of code, why not use Http2 as well? Put differently, work on making Http2 Builder's more comprehensive... and may or may not get to the deprecated clients.
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1010832334
##########
solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java:
##########
@@ -156,8 +160,7 @@ public synchronized SolrClient getSolrClient() {
public SolrClient createNewSolrClient() {
try {
// setup the client...
- final String url = jetty.getBaseUrl().toString() + "/" + "collection1";
- final SolrClient client = getHttpSolrClient(url, DEFAULT_CONNECTION_TIMEOUT);
+ final SolrClient client = getHttpSolrClient(getServerUrl(), DEFAULT_CONNECTION_TIMEOUT);
Review Comment:
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `SolrJettyTestBase.createNewSolrClient()` indirectly mutates container `util.ObjectReleaseTracker.OBJECTS` via call to `Map.put(...)` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<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=349964066&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=349964066&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=349964066&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=349964066&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=349964066&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 a diff in pull request #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1011891480
##########
solr/core/src/test/org/apache/solr/search/TestSmileRequest.java:
##########
@@ -71,7 +71,7 @@ public void testDistribJsonRequest() throws Exception {
@Override
public void assertJQ(SolrClient client, SolrParams args, String... tests)
throws Exception {
- ((HttpSolrClient) client).setParser(SmileResponseParser.inst);
+ ((HttpSolrClient) client).setParser(new SmileResponseParser());
Review Comment:
I can't figure out how to move away from the `.setParser`, I want to somehow override how `SolrTestCaseHS` sets up it's `SolrClient` and kind of got burned out trying to slueth thorugh all the layers of indirection...
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1326673034
Uh oh, not sure the changes in f49e2a26a46b17c9a9429dde56865c000e1863d1 make sense. Primarily because if the `LBHttp2SolrClient` wraps a `Http2SolrClient`, then we shouldn't be mucking around with the wrapped client, that would violate immutablity? So thinking about backing that change out, and then committing the rest? Thoughts?
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1299465793
Can't figure out how to use builder pattern in `TestSmileRequest` ;-(
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1011881388
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClient.java:
##########
@@ -107,8 +107,8 @@ protected ConcurrentUpdateSolrClient(Builder builder) {
.withHttpClient(builder.httpClient)
.withConnectionTimeout(builder.connectionTimeoutMillis)
.withSocketTimeout(builder.socketTimeoutMillis)
+ .setFollowRedirects(false)
Review Comment:
thanks!
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1011844656
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientBuilder.java:
##########
@@ -46,6 +50,21 @@ public B withResponseParser(ResponseParser responseParser) {
return getThis();
}
+ public B withRequestWriter(RequestWriter requestWriter) {
+ this.requestWriter = requestWriter;
+ return getThis();
+ }
+
+ public B withUseMultiPartPost(Boolean useMultiPartPost) {
+ this.useMultiPartPost = useMultiPartPost;
+ return getThis();
+ }
+
+ public B setFollowRedirects(boolean withFollowRedirects) {
Review Comment:
`withFollowRedirects`?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/TestBatchUpdate.java:
##########
@@ -46,24 +46,28 @@ public static void beforeTest() throws Exception {
@Test
public void testWithXml() throws Exception {
- HttpSolrClient client = (HttpSolrClient) getSolrClient();
- client.setRequestWriter(new RequestWriter());
+ client =
Review Comment:
`client` must be closed in `SolrJettyTestBase`? would be clearer to do `try-with-resources` for each of these?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleTests.java:
##########
@@ -602,58 +602,60 @@ public void testUnicode() throws Exception {
SolrClient client = getSolrClient();
// save the old parser, so we can set it back.
- ResponseParser oldParser = null;
- if (client instanceof HttpSolrClient) {
- HttpSolrClient httpSolrClient = (HttpSolrClient) client;
- oldParser = httpSolrClient.getParser();
- }
-
- try {
- for (int iteration = 0; iteration < numIterations; iteration++) {
- // choose format
- if (client instanceof HttpSolrClient) {
- if (random.nextBoolean()) {
- ((HttpSolrClient) client).setParser(new BinaryResponseParser());
- } else {
- ((HttpSolrClient) client).setParser(new XMLResponseParser());
- }
- }
-
- int numDocs = TestUtil.nextInt(random(), 1, 10 * RANDOM_MULTIPLIER);
-
- // Empty the database...
- client.deleteByQuery("*:*"); // delete everything!
-
- List<SolrInputDocument> docs = new ArrayList<>();
- for (int i = 0; i < numDocs; i++) {
- // Now add something...
- SolrInputDocument doc = new SolrInputDocument();
- doc.addField("id", "" + i);
- doc.addField("unicode_s", randomTestString(30));
- docs.add(doc);
- }
+ // ResponseParser oldParser = null;
+ // if (client instanceof HttpSolrClient) {
+ // HttpSolrClient httpSolrClient = (HttpSolrClient) client;
+ // oldParser = httpSolrClient.getParser();
+ // }
Review Comment:
Guess still figuring out how this tests works/needs to be converted?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java:
##########
@@ -478,6 +484,8 @@ public void testUpdate() throws Exception {
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
// XML response and writer
+ // Have not been able to move this to the Builder pattern, the below check for application/xml
+ // always returns as application/javabin when I try this!
Review Comment:
Would be good to dig to why this happens
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java:
##########
@@ -341,7 +346,6 @@ public void testQuery() throws Exception {
assertEquals("keep-alive", DebugServlet.headers.get("Connection"));
// XML/POST
- client.setParser(new XMLResponseParser());
Review Comment:
Is there a side effect here about having a new `XMLResponseParser` to be concerned with?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClient.java:
##########
@@ -107,8 +107,8 @@ protected ConcurrentUpdateSolrClient(Builder builder) {
.withHttpClient(builder.httpClient)
.withConnectionTimeout(builder.connectionTimeoutMillis)
.withSocketTimeout(builder.socketTimeoutMillis)
+ .setFollowRedirects(false)
Review Comment:
`withFollowRedirects`?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java:
##########
@@ -118,72 +111,22 @@ public void testUtf8PerfDegradation() throws Exception {
doc.addField("id", "1");
doc.addField("b_is", IntStream.range(0, 30000).boxed().collect(Collectors.toList()));
- HttpSolrClient client = (HttpSolrClient) getSolrClient();
- client.add(doc);
- client.commit();
- long start = System.nanoTime();
- QueryResponse rsp = client.query(new SolrQuery("*:*"));
- System.out.println("time taken : " + ((System.nanoTime() - start)) / (1000 * 1000));
- assertEquals(1, rsp.getResults().getNumFound());
- }
-
- @Ignore
Review Comment:
when was this marked `@Ignore` would be good to find out why?
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1014385111
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -542,7 +557,32 @@ public void testUpdate() throws Exception {
}
@Test
- public void testRedirect() throws Exception {
+ public void testFollowRedirect() throws Exception {
+ final String clientUrl = jetty.getBaseUrl().toString() + "/redirect/foo";
+ try (Http2SolrClient client =
+ new Http2SolrClient.Builder(clientUrl).withFollowRedirects(true).build()) {
+ SolrQuery q = new SolrQuery("*:*");
+ client.query(q);
+ }
+ }
+
+ @Test
+ public void testDoNotFollowRedirect() throws Exception {
+ final String clientUrl = jetty.getBaseUrl().toString() + "/redirect/foo";
+ try (Http2SolrClient client =
+ new Http2SolrClient.Builder(clientUrl).withFollowRedirects(false).build()) {
+ SolrQuery q = new SolrQuery("*:*");
+ try {
+ client.query(q);
+ fail("Should have thrown an exception.");
Review Comment:
Done!
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -518,10 +527,16 @@ public void testUpdate() throws Exception {
assertEquals("application/xml; charset=UTF-8", DebugServlet.headers.get("content-type"));
assertEquals(1, DebugServlet.parameters.get("a").length);
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+ }
+ try (Http2SolrClient client =
+ new Http2SolrClient.Builder(url)
+ .withRequestWriter(new BinaryRequestWriter())
+ .withResponseParser(new BinaryResponseParser())
+ .build()) {
// javabin request
- client.setParser(new BinaryResponseParser());
- client.setRequestWriter(new BinaryRequestWriter());
+ // client.setParser(new BinaryResponseParser());
+ // client.setRequestWriter(new BinaryRequestWriter());
Review Comment:
Done!
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1304060117
@dsmiley @risdenk should I resolve comments as I follow up with them, or just leave them open till the end and maybe reply with a "Done" or "Dealt with"?? Whats the best process?
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1011860272
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java:
##########
@@ -813,6 +813,7 @@ public String getBaseURL() {
* In this case the client is more flexible and can be used to send requests to any cores. The
* cost of this is that the core must be specified on each request.
*/
+ @Deprecated
Review Comment:
Eventually probably want to have a `use this instead` part of the javadoc
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1310978720
Why is this not being split up? I mean... if we have a PR, we ultimately need a coherent commit message as to the scope of what it's doing. What is the scope 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 #1158: SOLR-16368: Use Builder Pattern with Solr Clients
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1032604734
##########
solr/core/src/test/org/apache/solr/schema/TestCloudManagedSchema.java:
##########
@@ -53,21 +54,22 @@ public void test() throws Exception {
QueryRequest request = new QueryRequest(params);
request.setPath("/admin/cores");
int which = r.nextInt(clients.size());
- HttpSolrClient client = (HttpSolrClient) clients.get(which);
- String previousBaseURL = client.getBaseURL();
- // Strip /collection1 step from baseURL - requests fail otherwise
- client.setBaseURL(previousBaseURL.substring(0, previousBaseURL.lastIndexOf("/")));
- NamedList<?> namedListResponse = client.request(request);
- client.setBaseURL(previousBaseURL); // Restore baseURL
- NamedList<?> status = (NamedList<?>) namedListResponse.get("status");
- NamedList<?> collectionStatus = (NamedList<?>) status.getVal(0);
- String collectionSchema = (String) collectionStatus.get(CoreAdminParams.SCHEMA);
- // Make sure the upgrade to managed schema happened
- assertEquals(
- "Schema resource name differs from expected name", "managed-schema.xml", collectionSchema);
- SolrZkClient zkClient = new SolrZkClient(zkServer.getZkHost(), 30000);
- try {
+ // create a client that does not have the /collection1 as part of the URL.
+ try (SolrClient rootClient =
+ new HttpSolrClient.Builder(buildUrl(jettys.get(which).getLocalPort())).build()) {
+ NamedList<?> namedListResponse = rootClient.request(request);
+ NamedList<?> status = (NamedList<?>) namedListResponse.get("status");
Review Comment:
I'd love a suggestion on this, or even a quick commit, as this whole area is a bit of dark art to me!
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1302480360
>
So I'm clear, you are suggesting that where we create a `HttpSolrClient` in tests that just need a `SolrClient`, look to move to Http2SolrClient? I can do that, it looks like `SolrJettyTestBase` is a candidate for that... I worry that will make this PR not mergable.. On the other hand, I'm happy to keep going!
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1013354025
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -2197,14 +2197,10 @@ protected SolrClient createNewSolrClient(
}
protected SolrClient createNewSolrClient(String collection, String baseUrl) {
- try {
// setup the server...
SolrClient client =
- getHttpSolrClient(baseUrl + "/" + collection, DEFAULT_CONNECTION_TIMEOUT, 60000);
+ getHttp2SolrClient(baseUrl + "/" + collection);
Review Comment:
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `AbstractFullDistribZkTestBase.createNewSolrClient(...)` indirectly mutates container `util.ObjectReleaseTracker.OBJECTS` via call to `Map.put(...)` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<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=350628553&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=350628553&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=350628553&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=350628553&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=350628553&lift_comment_rating=5) ]
##########
solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java:
##########
@@ -154,14 +158,9 @@ public synchronized SolrClient getSolrClient() {
* options.
*/
public SolrClient createNewSolrClient() {
- try {
- // setup the client...
- final String url = jetty.getBaseUrl().toString() + "/" + "collection1";
- final SolrClient client = getHttpSolrClient(url, DEFAULT_CONNECTION_TIMEOUT);
- return client;
- } catch (final Exception ex) {
- throw new RuntimeException(ex);
- }
+ // setup the client...
+ final SolrClient client = getHttp2SolrClient(getServerUrl());
Review Comment:
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `SolrJettyTestBase.createNewSolrClient()` indirectly mutates container `util.ObjectReleaseTracker.OBJECTS` via call to `Map.put(...)` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<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=350628730&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=350628730&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=350628730&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=350628730&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=350628730&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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1299363456
I'd love another set of eyes on eb13c64057c000326087915e5810c3eea5339adf @risdenk, I *think* I simplified the test which meant I could remove the `setParser` code... But maybe I lost what the test was all about...
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1011120330
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleStreamingBinaryTest.java:
##########
@@ -35,9 +34,15 @@ public class SolrExampleStreamingBinaryTest extends SolrExampleStreamingTest {
@Override
public SolrClient createNewSolrClient() {
- ConcurrentUpdateSolrClient client = (ConcurrentUpdateSolrClient) super.createNewSolrClient();
- client.setParser(new BinaryResponseParser());
- client.setRequestWriter(new BinaryRequestWriter());
+
+ SolrClient client =
+ new ErrorTrackingConcurrentUpdateSolrClient.Builder(getServerUrl())
+ .withQueueSize(2)
+ .withThreadCount(5)
Review Comment:
Queue size and thread count? They aren't in the original?
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1299471494
If this all looks in the right direction, then I can tackle `Http2SolrClient` and `ConcurrentUpdateHttp2SolrClient`, moving them to builder pattern.
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1011858823
##########
solr/core/src/test/org/apache/solr/search/TestSmileRequest.java:
##########
@@ -71,7 +71,7 @@ public void testDistribJsonRequest() throws Exception {
@Override
public void assertJQ(SolrClient client, SolrParams args, String... tests)
throws Exception {
- ((HttpSolrClient) client).setParser(SmileResponseParser.inst);
+ ((HttpSolrClient) client).setParser(new SmileResponseParser());
Review Comment:
What issues are you running into with this test?
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1011904785
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java:
##########
@@ -478,6 +484,8 @@ public void testUpdate() throws Exception {
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
// XML response and writer
+ // Have not been able to move this to the Builder pattern, the below check for application/xml
+ // always returns as application/javabin when I try this!
Review Comment:
yeah, I'd love a fresh set of eyes, I don't get why it doesn't work.. maybe somethign about the `DebugServlet` or some sort of something being reused...
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1014654220
##########
solr/core/src/test/org/apache/solr/search/TestSmileRequest.java:
##########
@@ -71,7 +71,7 @@ public void testDistribJsonRequest() throws Exception {
@Override
public void assertJQ(SolrClient client, SolrParams args, String... tests)
throws Exception {
- ((HttpSolrClient) client).setParser(SmileResponseParser.inst);
+ ((HttpSolrClient) client).setParser(new SmileResponseParser());
Review Comment:
Thanks for the tip, that worked 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 a diff in pull request #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1013166419
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java:
##########
@@ -341,7 +346,6 @@ public void testQuery() throws Exception {
assertEquals("keep-alive", DebugServlet.headers.get("Connection"));
// XML/POST
- client.setParser(new XMLResponseParser());
Review Comment:
so... Reworked the tests... still had to keep a `client.setParser()`, and never figured out why :-(
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1302395229
So... `LBHttp2SolrClient` doesn't have a Builder pattern... Should it? I'm worried about never getting this PR to a committable shape by adding more.... On the other hand, maybe to commit this, we need `LBHttp2SolrClient` to have a Builder?
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1031485769
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -499,10 +502,16 @@ public void testUpdate() throws Exception {
// parameter encoding
assertEquals(1, DebugServlet.parameters.get("a").length);
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+ }
+ try (Http2SolrClient client =
+ new Http2SolrClient.Builder(url)
+ .withRequestWriter(new RequestWriter())
+ .withResponseParser(new XMLResponseParser())
+ .build()) {
// XML response and writer
- client.setParser(new XMLResponseParser());
- client.setRequestWriter(new RequestWriter());
+ // client.setParser(new XMLResponseParser());
+ // client.setRequestWriter(new RequestWriter());
Review Comment:
and likewise, thanks for the eyes...
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1011894050
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java:
##########
@@ -813,6 +813,7 @@ public String getBaseURL() {
* In this case the client is more flexible and can be used to send requests to any cores. The
* cost of this is that the core must be specified on each request.
*/
+ @Deprecated
Review Comment:
Just scavenged some text from a patch file that @gerlowskija had written!
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1014392413
##########
solr/prometheus-exporter/src/test/org/apache/solr/prometheus/scraper/SolrStandaloneScraperTest.java:
##########
@@ -63,13 +63,14 @@ public static void setupBeforeClass() throws Exception {
configuration =
Helpers.loadConfiguration("conf/prometheus-solr-exporter-scraper-test-config.xml");
- solrClient = new Http2SolrClient.Builder(restTestHarness.getAdminURL()).build();
- solrScraper = new SolrStandaloneScraper(solrClient, executor, "test");
-
NoOpResponseParser responseParser = new NoOpResponseParser();
responseParser.setWriterType("json");
Review Comment:
done!~
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1304287579
> should I resolve comments as I follow up with them, or just leave them open till the end and maybe reply with a "Done" or "Dealt with"?? Whats the best process?
IMO simply saying "Done" is kind of noise. You can simply hit the "resolve" button. If you have something to say (other than "Done"), then say it, then maybe hit "resolve" if applicable. I'll get notifications with messages you type, even if subsequently resolved.
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1031485348
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -423,9 +422,12 @@ public void testDelete() throws Exception {
client.getParser().getVersion(), DebugServlet.parameters.get(CommonParams.VERSION)[0]);
// agent
assertEquals(EXPECTED_USER_AGENT, DebugServlet.headers.get("user-agent"));
+ }
+ // XML
+ try (Http2SolrClient client =
+ new Http2SolrClient.Builder(url).withResponseParser(new XMLResponseParser()).build()) {
- // XML
- client.setParser(new XMLResponseParser());
+ // client.setParser(new XMLResponseParser());
Review Comment:
thanks for the eyes!
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1031540437
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -2165,46 +2165,14 @@ protected SolrClient createNewSolrClient(int port) {
return createNewSolrClient(DEFAULT_COLLECTION, port);
}
- protected SolrClient createNewSolrClient(
- int port, int connectionTimeoutMillis, int socketTimeoutMillis) {
- return createNewSolrClient(
- DEFAULT_COLLECTION, port, connectionTimeoutMillis, socketTimeoutMillis);
- }
-
protected SolrClient createNewSolrClient(String coreName, int port) {
- try {
- // setup the server...
- String baseUrl = buildUrl(port);
- String url = baseUrl + (baseUrl.endsWith("/") ? "" : "/") + coreName;
- SolrClient client = getHttpSolrClient(url, DEFAULT_CONNECTION_TIMEOUT, 60000);
- return client;
- } catch (Exception ex) {
- throw new RuntimeException(ex);
- }
- }
-
- protected SolrClient createNewSolrClient(
- String coreName, int port, int connectionTimeoutMillis, int socketTimeoutMillis) {
- try {
- // setup the server...
- String baseUrl = buildUrl(port);
- String url = baseUrl + (baseUrl.endsWith("/") ? "" : "/") + coreName;
- SolrClient client = getHttpSolrClient(url, connectionTimeoutMillis, socketTimeoutMillis);
- return client;
- } catch (Exception ex) {
- throw new RuntimeException(ex);
- }
+ String baseUrl = buildUrl(port);
+ String url = baseUrl + (baseUrl.endsWith("/") ? "" : "/") + coreName;
+ return getHttpSolrClient(url, DEFAULT_CONNECTION_TIMEOUT, 60000);
Review Comment:
đŦ 2 similar findings have been found in this PR
---
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `AbstractFullDistribZkTestBase.createNewSolrClient(...)` indirectly mutates container `util.ObjectReleaseTracker.OBJECTS` via call to `Map.put(...)` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<details><summary><b>đ Expand here to view all instances of this finding</b></summary><br/>
<div align="center">
| **File Path** | **Line Number** |
| ------------- | ------------- |
| solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java | [2175](https://github.com/epugh/solr/blob/acde86bfb2a43624ffc2df4ec791cd7ad494449b/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java#L2175)|
| solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java | [161](https://github.com/epugh/solr/blob/acde86bfb2a43624ffc2df4ec791cd7ad494449b/solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java#L161)|
<p><a href="https://lift.sonatype.com/results/github.com/apache/solr/01GJMW794KN7GJMJC17P5CM4ET?t=Infer|THREAD_SAFETY_VIOLATION" 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=355304250&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=355304250&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=355304250&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=355304250&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=355304250&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] madrob commented on a diff in pull request #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
madrob commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1031797318
##########
solr/core/src/test/org/apache/solr/schema/TestCloudManagedSchema.java:
##########
@@ -53,21 +54,22 @@ public void test() throws Exception {
QueryRequest request = new QueryRequest(params);
request.setPath("/admin/cores");
int which = r.nextInt(clients.size());
- HttpSolrClient client = (HttpSolrClient) clients.get(which);
- String previousBaseURL = client.getBaseURL();
- // Strip /collection1 step from baseURL - requests fail otherwise
- client.setBaseURL(previousBaseURL.substring(0, previousBaseURL.lastIndexOf("/")));
- NamedList<?> namedListResponse = client.request(request);
- client.setBaseURL(previousBaseURL); // Restore baseURL
- NamedList<?> status = (NamedList<?>) namedListResponse.get("status");
- NamedList<?> collectionStatus = (NamedList<?>) status.getVal(0);
- String collectionSchema = (String) collectionStatus.get(CoreAdminParams.SCHEMA);
- // Make sure the upgrade to managed schema happened
- assertEquals(
- "Schema resource name differs from expected name", "managed-schema.xml", collectionSchema);
- SolrZkClient zkClient = new SolrZkClient(zkServer.getZkHost(), 30000);
- try {
+ // create a client that does not have the /collection1 as part of the URL.
+ try (SolrClient rootClient =
+ new HttpSolrClient.Builder(buildUrl(jettys.get(which).getLocalPort())).build()) {
+ NamedList<?> namedListResponse = rootClient.request(request);
+ NamedList<?> status = (NamedList<?>) namedListResponse.get("status");
Review Comment:
There's a multi argument get or something on named list that lets you drill in. Don't remember what it's called exactly, I'm on my phone 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 pull request #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1311003183
During the discussion, there was a desire to use the same pattern across all the SolrClients.. And yeah, I think we are breaking this PR up...
--
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 #1158: SOLR-16368: Use Builder Pattern with Solr Clients
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1033664796
##########
solr/core/src/test/org/apache/solr/search/TestSmileRequest.java:
##########
@@ -91,7 +90,6 @@ public void assertJQ(SolrClient client, SolrParams args, String... tests)
// adding this to core adds the dependency on a few extra jars to our distribution.
// So this is not added there
public static class SmileResponseParser extends BinaryResponseParser {
- public static final SmileResponseParser inst = new SmileResponseParser();
Review Comment:
you can probably revert this line change and set the query response parser to `SmileResponseParser.inst`
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java:
##########
@@ -476,9 +484,20 @@ public void testUpdate() throws Exception {
// parameter encoding
assertEquals(1, DebugServlet.parameters.get("a").length);
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+ }
+ DebugServlet.clear();
+ // XML response and writer
+ try (HttpSolrClient client =
+ new HttpSolrClient.Builder(url)
+ .withRequestWriter(new RequestWriter())
+ .withResponseParser(new XMLResponseParser())
+ .build()) {
+ UpdateRequest req = new UpdateRequest();
+ req.add(new SolrInputDocument());
+ req.setParam("a", "\u1234");
- // XML response and writer
- client.setParser(new XMLResponseParser());
+ // Have not been able to move this to the Builder pattern, the below check for application/xml
+ // always returns as application/javabin when .setRequestWriter() is commented out.
Review Comment:
Was this ever looked at again?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -379,7 +378,7 @@ public void testQuery() throws Exception {
assertEquals(EXPECTED_USER_AGENT, DebugServlet.headers.get("user-agent"));
assertEquals("application/x-www-form-urlencoded", DebugServlet.headers.get("content-type"));
- client.setParser(new XMLResponseParser());
+ // client.setParser(new XMLResponseParser());
Review Comment:
looks like this wasn't removed yet
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -360,7 +359,7 @@ public void testQuery() throws Exception {
assertEquals(EXPECTED_USER_AGENT, DebugServlet.headers.get("user-agent"));
// XML/POST
- client.setParser(new XMLResponseParser());
+ // client.setParser(new XMLResponseParser());
Review Comment:
looks like this wasn't removed yet
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1013203543
##########
solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java:
##########
@@ -154,14 +158,9 @@ public synchronized SolrClient getSolrClient() {
* options.
*/
public SolrClient createNewSolrClient() {
- try {
- // setup the client...
- final String url = jetty.getBaseUrl().toString() + "/" + "collection1";
- final SolrClient client = getHttpSolrClient(url, DEFAULT_CONNECTION_TIMEOUT);
- return client;
- } catch (final Exception ex) {
- throw new RuntimeException(ex);
- }
+ // setup the client...
+ final SolrClient client = getHttpSolrClient(getServerUrl(), DEFAULT_CONNECTION_TIMEOUT);
Review Comment:
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `SolrJettyTestBase.createNewSolrClient()` indirectly mutates container `util.ObjectReleaseTracker.OBJECTS` via call to `Map.put(...)` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<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=350588764&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=350588764&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=350588764&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=350588764&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=350588764&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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1014351333
##########
solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java:
##########
@@ -156,8 +160,7 @@ public synchronized SolrClient getSolrClient() {
public SolrClient createNewSolrClient() {
try {
// setup the client...
- final String url = jetty.getBaseUrl().toString() + "/" + "collection1";
- final SolrClient client = getHttpSolrClient(url, DEFAULT_CONNECTION_TIMEOUT);
+ final SolrClient client = getHttpSolrClient(getServerUrl(), DEFAULT_CONNECTION_TIMEOUT);
Review Comment:
@sonatype-lift exclude solr/solrj/src/java/org/apache/solr/common/util/ObjectReleaseTracker.java
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1014396292
##########
solr/core/src/test/org/apache/solr/schema/TestCloudManagedSchema.java:
##########
@@ -53,12 +54,11 @@ public void test() throws Exception {
QueryRequest request = new QueryRequest(params);
request.setPath("/admin/cores");
int which = r.nextInt(clients.size());
- HttpSolrClient client = (HttpSolrClient) clients.get(which);
- String previousBaseURL = client.getBaseURL();
- // Strip /collection1 step from baseURL - requests fail otherwise
- client.setBaseURL(previousBaseURL.substring(0, previousBaseURL.lastIndexOf("/")));
- NamedList<?> namedListResponse = client.request(request);
- client.setBaseURL(previousBaseURL); // Restore baseURL
+
+ // create a client that does not have the /collection1 as part of the URL.
+ SolrClient rootClient =
+ new HttpSolrClient.Builder(buildUrl(jettys.get(which).getLocalPort())).build();
+ NamedList<?> namedListResponse = rootClient.request(request);
Review Comment:
Done, and did the same with the `zkClient`
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1014379583
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java:
##########
@@ -72,15 +65,15 @@ public void testBadSetup() {
@Test
public void testArbitraryJsonIndexing() throws Exception {
- HttpSolrClient client = (HttpSolrClient) getSolrClient();
+ SolrClient client = getSolrClient();
Review Comment:
ah ok nvm then. guess this just returns a reference to a solr client.
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1014303007
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java:
##########
@@ -476,9 +484,20 @@ public void testUpdate() throws Exception {
// parameter encoding
assertEquals(1, DebugServlet.parameters.get("a").length);
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+ }
+ DebugServlet.clear();
+ // XML response and writer
+ try (HttpSolrClient client =
+ new HttpSolrClient.Builder(url)
+ .withRequestWriter(new RequestWriter())
+ .withResponseParser(new XMLResponseParser())
+ .build()) {
+ UpdateRequest req = new UpdateRequest();
+ req.add(new SolrInputDocument());
+ req.setParam("a", "\u1234");
- // XML response and writer
- client.setParser(new XMLResponseParser());
+ // Have not been able to move this to the Builder pattern, the below check for application/xml
+ // always returns as application/javabin when .setRequestWriter() is commented out.
Review Comment:
Needs to be looked at again.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -360,7 +359,7 @@ public void testQuery() throws Exception {
assertEquals(EXPECTED_USER_AGENT, DebugServlet.headers.get("user-agent"));
// XML/POST
- client.setParser(new XMLResponseParser());
+ // client.setParser(new XMLResponseParser());
Review Comment:
This can be removed?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -379,7 +378,7 @@ public void testQuery() throws Exception {
assertEquals(EXPECTED_USER_AGENT, DebugServlet.headers.get("user-agent"));
assertEquals("application/x-www-form-urlencoded", DebugServlet.headers.get("content-type"));
- client.setParser(new XMLResponseParser());
+ // client.setParser(new XMLResponseParser());
Review Comment:
This can be removed?
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2562,20 +2562,20 @@ public static Object skewed(Object likely, Object unlikely) {
* A variant of {@link org.apache.solr.client.solrj.impl.CloudHttp2SolrClient.Builder} that will
* randomize some internal settings.
*/
- public static class CloudHttp2SolrClientBuilder extends CloudHttp2SolrClient.Builder {
+ public static class RandomizingCloudHttp2SolrClientBuilder extends CloudHttp2SolrClient.Builder {
Review Comment:
Since this name change didn't actually affect any other files - is this class even used???
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -518,10 +527,16 @@ public void testUpdate() throws Exception {
assertEquals("application/xml; charset=UTF-8", DebugServlet.headers.get("content-type"));
assertEquals(1, DebugServlet.parameters.get("a").length);
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+ }
+ try (Http2SolrClient client =
+ new Http2SolrClient.Builder(url)
+ .withRequestWriter(new BinaryRequestWriter())
+ .withResponseParser(new BinaryResponseParser())
+ .build()) {
// javabin request
- client.setParser(new BinaryResponseParser());
- client.setRequestWriter(new BinaryRequestWriter());
+ // client.setParser(new BinaryResponseParser());
+ // client.setRequestWriter(new BinaryRequestWriter());
Review Comment:
This can be removed?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -499,10 +502,16 @@ public void testUpdate() throws Exception {
// parameter encoding
assertEquals(1, DebugServlet.parameters.get("a").length);
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+ }
+ try (Http2SolrClient client =
+ new Http2SolrClient.Builder(url)
+ .withRequestWriter(new RequestWriter())
+ .withResponseParser(new XMLResponseParser())
+ .build()) {
// XML response and writer
- client.setParser(new XMLResponseParser());
- client.setRequestWriter(new RequestWriter());
+ // client.setParser(new XMLResponseParser());
+ // client.setRequestWriter(new RequestWriter());
Review Comment:
This can be removed?
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2872,9 +2872,16 @@ public static LBHttpSolrClient getLBHttpSolrClient(String... solrUrls)
}
/**
- * This method <i>may</i> randomize unspecified aspects of the resulting SolrClient. Tests that do
- * not wish to have any randomized behavior should use the {@link
Review Comment:
Hmm I wonder if there should be more randomization here? I think the keyword is `may` but not yet? If we had tests use these methods we could randomize stuff easier?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -542,7 +557,32 @@ public void testUpdate() throws Exception {
}
@Test
- public void testRedirect() throws Exception {
+ public void testFollowRedirect() throws Exception {
+ final String clientUrl = jetty.getBaseUrl().toString() + "/redirect/foo";
+ try (Http2SolrClient client =
+ new Http2SolrClient.Builder(clientUrl).withFollowRedirects(true).build()) {
+ SolrQuery q = new SolrQuery("*:*");
+ client.query(q);
+ }
+ }
+
+ @Test
+ public void testDoNotFollowRedirect() throws Exception {
+ final String clientUrl = jetty.getBaseUrl().toString() + "/redirect/foo";
+ try (Http2SolrClient client =
+ new Http2SolrClient.Builder(clientUrl).withFollowRedirects(false).build()) {
+ SolrQuery q = new SolrQuery("*:*");
+ try {
+ client.query(q);
+ fail("Should have thrown an exception.");
Review Comment:
Better to use `assertThrows` if possible.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleTests.java:
##########
@@ -602,58 +602,60 @@ public void testUnicode() throws Exception {
SolrClient client = getSolrClient();
// save the old parser, so we can set it back.
- ResponseParser oldParser = null;
- if (client instanceof HttpSolrClient) {
- HttpSolrClient httpSolrClient = (HttpSolrClient) client;
- oldParser = httpSolrClient.getParser();
- }
-
- try {
- for (int iteration = 0; iteration < numIterations; iteration++) {
- // choose format
- if (client instanceof HttpSolrClient) {
- if (random.nextBoolean()) {
- ((HttpSolrClient) client).setParser(new BinaryResponseParser());
- } else {
- ((HttpSolrClient) client).setParser(new XMLResponseParser());
- }
- }
-
- int numDocs = TestUtil.nextInt(random(), 1, 10 * RANDOM_MULTIPLIER);
-
- // Empty the database...
- client.deleteByQuery("*:*"); // delete everything!
-
- List<SolrInputDocument> docs = new ArrayList<>();
- for (int i = 0; i < numDocs; i++) {
- // Now add something...
- SolrInputDocument doc = new SolrInputDocument();
- doc.addField("id", "" + i);
- doc.addField("unicode_s", randomTestString(30));
- docs.add(doc);
- }
+ // ResponseParser oldParser = null;
+ // if (client instanceof HttpSolrClient) {
+ // HttpSolrClient httpSolrClient = (HttpSolrClient) client;
+ // oldParser = httpSolrClient.getParser();
+ // }
Review Comment:
Not sure where my other comment went :) I had one typed up about this but looks like it went poof.
Since the test is called `testUnicode` - I wonder if `SolrExampleBinaryTest` or `SolrExampleXMLTest` test unicode? maybe this is specific to unicode and thats why the parser is randomized?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/TestSolrJErrorHandling.java:
##########
@@ -102,16 +102,19 @@ public void showExceptions() throws Exception {
@Test
public void testWithXml() throws Exception {
- HttpSolrClient client = (HttpSolrClient) getSolrClient();
- client.setRequestWriter(new RequestWriter());
+ client =
+ new HttpSolrClient.Builder(getServerUrl()).withRequestWriter(new RequestWriter()).build();
+
Review Comment:
`try-with-resources` since we create new clients here.
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2562,20 +2562,20 @@ public static Object skewed(Object likely, Object unlikely) {
* A variant of {@link org.apache.solr.client.solrj.impl.CloudHttp2SolrClient.Builder} that will
* randomize some internal settings.
*/
- public static class CloudHttp2SolrClientBuilder extends CloudHttp2SolrClient.Builder {
+ public static class RandomizingCloudHttp2SolrClientBuilder extends CloudHttp2SolrClient.Builder {
Review Comment:
Name change makes sense.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java:
##########
@@ -72,15 +65,15 @@ public void testBadSetup() {
@Test
public void testArbitraryJsonIndexing() throws Exception {
- HttpSolrClient client = (HttpSolrClient) getSolrClient();
+ SolrClient client = getSolrClient();
Review Comment:
should be closed eventually?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/TestBatchUpdate.java:
##########
@@ -46,24 +46,28 @@ public static void beforeTest() throws Exception {
@Test
public void testWithXml() throws Exception {
- HttpSolrClient client = (HttpSolrClient) getSolrClient();
- client.setRequestWriter(new RequestWriter());
+ client =
Review Comment:
I'm of the opinion - we should close the Solr client where we create the Solr client. Since this change is making new Solr clients for each test - they should be `try-with-resources`.
##########
solr/prometheus-exporter/src/test/org/apache/solr/prometheus/scraper/SolrStandaloneScraperTest.java:
##########
@@ -63,13 +63,14 @@ public static void setupBeforeClass() throws Exception {
configuration =
Helpers.loadConfiguration("conf/prometheus-solr-exporter-scraper-test-config.xml");
- solrClient = new Http2SolrClient.Builder(restTestHarness.getAdminURL()).build();
- solrScraper = new SolrStandaloneScraper(solrClient, executor, "test");
-
NoOpResponseParser responseParser = new NoOpResponseParser();
responseParser.setWriterType("json");
Review Comment:
I think this is redundant now?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -423,9 +422,12 @@ public void testDelete() throws Exception {
client.getParser().getVersion(), DebugServlet.parameters.get(CommonParams.VERSION)[0]);
// agent
assertEquals(EXPECTED_USER_AGENT, DebugServlet.headers.get("user-agent"));
+ }
+ // XML
+ try (Http2SolrClient client =
+ new Http2SolrClient.Builder(url).withResponseParser(new XMLResponseParser()).build()) {
- // XML
- client.setParser(new XMLResponseParser());
+ // client.setParser(new XMLResponseParser());
Review Comment:
This can be 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] epugh closed pull request #1158: SOLR-16368: Use Builder Pattern with Solr Clients
Posted by GitBox <gi...@apache.org>.
epugh closed pull request #1158: SOLR-16368: Use Builder Pattern with Solr Clients
URL: https://github.com/apache/solr/pull/1158
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1010870504
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleStreamingBinaryTest.java:
##########
@@ -35,9 +34,15 @@ public class SolrExampleStreamingBinaryTest extends SolrExampleStreamingTest {
@Override
public SolrClient createNewSolrClient() {
- ConcurrentUpdateSolrClient client = (ConcurrentUpdateSolrClient) super.createNewSolrClient();
- client.setParser(new BinaryResponseParser());
- client.setRequestWriter(new BinaryRequestWriter());
+
+ SolrClient client =
+ new ErrorTrackingConcurrentUpdateSolrClient.Builder(getServerUrl())
+ .withQueueSize(2)
+ .withThreadCount(5)
Review Comment:
can you elaborate on what is not needed? Defining the client here versus just returning from the .build()?
--
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 #1158: SOLR-16368: Use Builder Pattern with Solr Clients
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1327719483
Pinged mailing list about getting this reviewed and merged next week....
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1326484924
Hi all, so in #acde86bfb2a43624ffc2df4ec791cd7ad494449b I backed out the changes to the tests to move from Http1 to Http2, pending both the bug fixed version of Jetty 10 showing up plus the community deciding to commit to http2 as the default choice. I'm going to finish adding a Builder pattern to `LBHttp2SolrClient`, and then I think this PR will be ready for final review and 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] epugh commented on a diff in pull request #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1014373636
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2562,20 +2562,20 @@ public static Object skewed(Object likely, Object unlikely) {
* A variant of {@link org.apache.solr.client.solrj.impl.CloudHttp2SolrClient.Builder} that will
* randomize some internal settings.
*/
- public static class CloudHttp2SolrClientBuilder extends CloudHttp2SolrClient.Builder {
+ public static class RandomizingCloudHttp2SolrClientBuilder extends CloudHttp2SolrClient.Builder {
Review Comment:
So, it is called from `SolrTestCaseJ4.getCloudHttp2SolrClient()`, who does `return new RandomizingCloudHttp2SolrClientBuilder(cluster).build();`
`getCloudHttp2SolrClient()` is only ever used by one method `CloudHttp2SolrClientBadInputTest.testDeleteByIdReportsInvalidIdLists`, who I don't actually think cares that it might have randomization.
It appears to be a "good idea" that never went anywhere... We could move it to that one class, or maybe look to use it in more places? Dunno.
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1014378171
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java:
##########
@@ -72,15 +65,15 @@ public void testBadSetup() {
@Test
public void testArbitraryJsonIndexing() throws Exception {
- HttpSolrClient client = (HttpSolrClient) getSolrClient();
+ SolrClient client = getSolrClient();
Review Comment:
these get closed by the base test case... However, happy to wrap it in a try with.... Maybe we should remove the magic closing in the base test case?
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1014374236
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2872,9 +2872,16 @@ public static LBHttpSolrClient getLBHttpSolrClient(String... solrUrls)
}
/**
- * This method <i>may</i> randomize unspecified aspects of the resulting SolrClient. Tests that do
- * not wish to have any randomized behavior should use the {@link
Review Comment:
Yeah, I thought it better to fix the docs to match the current code!
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1010874223
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientBuilder.java:
##########
@@ -46,6 +49,16 @@ public B withResponseParser(ResponseParser responseParser) {
return getThis();
}
+ public B withRequestWriter(RequestWriter requestWriter) {
Review Comment:
yeah, I think if folks agree with moving to immutable SolrClient then 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] epugh commented on a diff in pull request #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1011647489
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleStreamingBinaryTest.java:
##########
@@ -35,9 +34,15 @@ public class SolrExampleStreamingBinaryTest extends SolrExampleStreamingTest {
@Override
public SolrClient createNewSolrClient() {
- ConcurrentUpdateSolrClient client = (ConcurrentUpdateSolrClient) super.createNewSolrClient();
- client.setParser(new BinaryResponseParser());
- client.setRequestWriter(new BinaryRequestWriter());
+
+ SolrClient client =
+ new ErrorTrackingConcurrentUpdateSolrClient.Builder(getServerUrl())
+ .withQueueSize(2)
+ .withThreadCount(5)
Review Comment:
looking into it now... i might have goofed! I remember thinking "this is different"...
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1011902917
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java:
##########
@@ -341,7 +346,6 @@ public void testQuery() throws Exception {
assertEquals("keep-alive", DebugServlet.headers.get("Connection"));
// XML/POST
- client.setParser(new XMLResponseParser());
Review Comment:
i don't think so, as we generate a new client on line 236 with a fresh `XMLResponseParser`, and the test pasts. However, the method `testUpdate` is giving me problems....
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1011897088
##########
solr/solrj/src/test/org/apache/solr/client/solrj/TestBatchUpdate.java:
##########
@@ -46,24 +46,28 @@ public static void beforeTest() throws Exception {
@Test
public void testWithXml() throws Exception {
- HttpSolrClient client = (HttpSolrClient) getSolrClient();
- client.setRequestWriter(new RequestWriter());
+ client =
Review Comment:
there is a
```
@After
public synchronized void afterClass() throws Exception {
if (client != null) client.close();
client = null;
}
```
There doesn't appear to be any one pattern...
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1013364124
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -2172,39 +2172,26 @@ protected SolrClient createNewSolrClient(
}
protected SolrClient createNewSolrClient(String coreName, int port) {
- try {
- // setup the server...
- String baseUrl = buildUrl(port);
- String url = baseUrl + (baseUrl.endsWith("/") ? "" : "/") + coreName;
- SolrClient client = getHttpSolrClient(url, DEFAULT_CONNECTION_TIMEOUT, 60000);
- return client;
- } catch (Exception ex) {
- throw new RuntimeException(ex);
- }
+ // setup the server...
+ String baseUrl = buildUrl(port);
+ String url = baseUrl + (baseUrl.endsWith("/") ? "" : "/") + coreName;
+ SolrClient client = getHttpSolrClient(url, DEFAULT_CONNECTION_TIMEOUT, 60000);
+ return client;
}
protected SolrClient createNewSolrClient(
String coreName, int port, int connectionTimeoutMillis, int socketTimeoutMillis) {
- try {
- // setup the server...
- String baseUrl = buildUrl(port);
- String url = baseUrl + (baseUrl.endsWith("/") ? "" : "/") + coreName;
- SolrClient client = getHttpSolrClient(url, connectionTimeoutMillis, socketTimeoutMillis);
- return client;
- } catch (Exception ex) {
- throw new RuntimeException(ex);
- }
+ // setup the server...
+ String baseUrl = buildUrl(port);
+ String url = baseUrl + (baseUrl.endsWith("/") ? "" : "/") + coreName;
+ SolrClient client = getHttpSolrClient(url, connectionTimeoutMillis, socketTimeoutMillis);
Review Comment:
đŦ 2 similar findings have been found in this PR
---
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `AbstractFullDistribZkTestBase.createNewSolrClient(...)` indirectly mutates container `util.ObjectReleaseTracker.OBJECTS` via call to `Map.put(...)` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<details><summary><b>đ Expand here to view all instances of this finding</b></summary><br/>
<div align="center">
| **File Path** | **Line Number** |
| ------------- | ------------- |
| solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java | [2178](https://github.com/epugh/solr/blob/1589aa97d7f96ac7c489b9e14c63a2d91ed0d5a1/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java#L2178)|
| solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java | [2193](https://github.com/epugh/solr/blob/1589aa97d7f96ac7c489b9e14c63a2d91ed0d5a1/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java#L2193)|
<p><a href="https://lift.sonatype.com/results/github.com/apache/solr/01GGZG44HAXB4CWCS53NJCEE3G?t=Infer|THREAD_SAFETY_VIOLATION" 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=350637967&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=350637967&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=350637967&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=350637967&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=350637967&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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1013976253
##########
solr/core/src/test/org/apache/solr/schema/TestCloudManagedSchema.java:
##########
@@ -53,12 +54,11 @@ public void test() throws Exception {
QueryRequest request = new QueryRequest(params);
request.setPath("/admin/cores");
int which = r.nextInt(clients.size());
- HttpSolrClient client = (HttpSolrClient) clients.get(which);
- String previousBaseURL = client.getBaseURL();
- // Strip /collection1 step from baseURL - requests fail otherwise
- client.setBaseURL(previousBaseURL.substring(0, previousBaseURL.lastIndexOf("/")));
- NamedList<?> namedListResponse = client.request(request);
- client.setBaseURL(previousBaseURL); // Restore baseURL
+
+ // create a client that does not have the /collection1 as part of the URL.
+ SolrClient rootClient =
+ new HttpSolrClient.Builder(buildUrl(jettys.get(which).getLocalPort())).build();
+ NamedList<?> namedListResponse = rootClient.request(request);
Review Comment:
Your change will probably leave an un-closed SolrClient hanging around, which will fail.
##########
solr/core/src/test/org/apache/solr/search/TestSmileRequest.java:
##########
@@ -71,7 +71,7 @@ public void testDistribJsonRequest() throws Exception {
@Override
public void assertJQ(SolrClient client, SolrParams args, String... tests)
throws Exception {
- ((HttpSolrClient) client).setParser(SmileResponseParser.inst);
+ ((HttpSolrClient) client).setParser(new SmileResponseParser());
Review Comment:
Okay but surely moving away from a global instance to creating a new instance (of this parser thing) will have zero effect on solving these complications around SolrClient lifecycle/mutability.
Looking at this specific scenario, it seems to me we need/want to be able to set the parser for a specific request, which will not run afoul of our immutability plans for SolrClient. At the SolrClient level, setting this parser is more global (across requests). SolrQueryRequest has a setResponseParser; try that.
--
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 #1158: SOLR-16368: Use Builder Pattern with Solr Clients
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1038648987
##########
solr/core/src/test/org/apache/solr/search/TestSmileRequest.java:
##########
@@ -91,7 +90,6 @@ public void assertJQ(SolrClient client, SolrParams args, String... tests)
// adding this to core adds the dependency on a few extra jars to our distribution.
// So this is not added there
public static class SmileResponseParser extends BinaryResponseParser {
- public static final SmileResponseParser inst = new SmileResponseParser();
Review Comment:
i liked how `query.setResponseParser(new SmileResponseParser());` looked better, everywhere else we create a new parser....
I wonder if this should be called "FakeSmileResponseParser" so you don't think it's real...
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1310803128
Would it work if I create a NEW PR, based on this PR, where I make the http1 to http2 changes, and back out those changes from this PR, so that this one is committable... ? Plus add the Builder pattern to the LBHttp2SolrClient? Then we can get this merged, and then once we are on the right version of Jetty, merge the http1 to http2 changes?
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1310980060
> Plus add the Builder pattern to the LBHttp2SolrClient?
I said I don't think it's necessary as its internal; not sure if you/someone disagreed or you've seen evidence as to its public-ness.
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1010801052
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrSchemalessExampleTest.java:
##########
@@ -130,20 +130,18 @@ public void testFieldMutating() throws Exception {
@Override
public SolrClient createNewSolrClient() {
- try {
- // setup the server...
- String url = jetty.getBaseUrl().toString() + "/collection1";
- HttpSolrClient client = getHttpSolrClient(url, DEFAULT_CONNECTION_TIMEOUT);
- client.setUseMultiPartPost(random().nextBoolean());
-
- if (random().nextBoolean()) {
- client.setParser(new BinaryResponseParser());
- client.setRequestWriter(new BinaryRequestWriter());
- }
-
- return client;
- } catch (Exception ex) {
- throw new RuntimeException(ex);
+ // setup the server...
+ String url = jetty.getBaseUrl().toString() + "/collection1";
+ HttpSolrClient.Builder httpSolrClientBuilder = new HttpSolrClient.Builder(url);
+ if (random().nextBoolean()) {
+ httpSolrClientBuilder
+ .withRequestWriter(new BinaryRequestWriter())
+ .withResponseParser(new BinaryResponseParser());
+ }
+ if (random().nextBoolean()) {
+ httpSolrClientBuilder.withUseMultiPartPost(true);
}
Review Comment:
don't need to the if statement
```suggestion
httpSolrClientBuilder.withUseMultiPartPost(random().nextBoolean());
```
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrSchemalessExampleTest.java:
##########
@@ -130,20 +130,18 @@ public void testFieldMutating() throws Exception {
@Override
public SolrClient createNewSolrClient() {
- try {
- // setup the server...
- String url = jetty.getBaseUrl().toString() + "/collection1";
- HttpSolrClient client = getHttpSolrClient(url, DEFAULT_CONNECTION_TIMEOUT);
- client.setUseMultiPartPost(random().nextBoolean());
-
- if (random().nextBoolean()) {
- client.setParser(new BinaryResponseParser());
- client.setRequestWriter(new BinaryRequestWriter());
- }
-
- return client;
- } catch (Exception ex) {
- throw new RuntimeException(ex);
+ // setup the server...
+ String url = jetty.getBaseUrl().toString() + "/collection1";
Review Comment:
should be `getServerUrl()`?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleStreamingBinaryTest.java:
##########
@@ -35,9 +34,15 @@ public class SolrExampleStreamingBinaryTest extends SolrExampleStreamingTest {
@Override
public SolrClient createNewSolrClient() {
- ConcurrentUpdateSolrClient client = (ConcurrentUpdateSolrClient) super.createNewSolrClient();
- client.setParser(new BinaryResponseParser());
- client.setRequestWriter(new BinaryRequestWriter());
+
+ SolrClient client =
+ new ErrorTrackingConcurrentUpdateSolrClient.Builder(getServerUrl())
+ .withQueueSize(2)
+ .withThreadCount(5)
Review Comment:
not needed
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientBuilder.java:
##########
@@ -46,6 +49,16 @@ public B withResponseParser(ResponseParser responseParser) {
return getThis();
}
+ public B withRequestWriter(RequestWriter requestWriter) {
+ this.requestWriter = requestWriter;
+ return getThis();
+ }
+
+ public B withUseMultiPartPost(Boolean useMultiPartPost) {
Review Comment:
should deprecate `setUseMultiPartPost`?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientBuilder.java:
##########
@@ -46,6 +49,16 @@ public B withResponseParser(ResponseParser responseParser) {
return getThis();
}
+ public B withRequestWriter(RequestWriter requestWriter) {
Review Comment:
should deprecate `setRequestWriter`?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleBinaryTest.java:
##########
@@ -32,19 +32,15 @@ public static void beforeTest() throws Exception {
@Override
public SolrClient createNewSolrClient() {
- try {
- // setup the server...
- String url = jetty.getBaseUrl().toString() + "/collection1";
- HttpSolrClient client = getHttpSolrClient(url, DEFAULT_CONNECTION_TIMEOUT);
- client.setUseMultiPartPost(random().nextBoolean());
+ // setup the server...
+ String url = jetty.getBaseUrl().toString() + "/collection1";
Review Comment:
should be `getServerUrl()`?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleXMLTest.java:
##########
@@ -32,15 +32,14 @@ public static void beforeTest() throws Exception {
@Override
public SolrClient createNewSolrClient() {
- try {
- String url = jetty.getBaseUrl().toString() + "/collection1";
- HttpSolrClient client = getHttpSolrClient(url, DEFAULT_CONNECTION_TIMEOUT);
- client.setUseMultiPartPost(random().nextBoolean());
- client.setParser(new XMLResponseParser());
- client.setRequestWriter(new RequestWriter());
- return client;
- } catch (Exception ex) {
- throw new RuntimeException(ex);
- }
+
+ String url = jetty.getBaseUrl().toString() + "/collection1";
Review Comment:
should be `getServerUrl()`
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1299286090
@risdenk I'd love some input on the way I did deprecation. Since the whole class is kind of deprecated, I didn't provide a bunch of `@deprecation` in the javadocs, or add the `since` tag... Do you think I should?
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1011888554
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java:
##########
@@ -118,72 +111,22 @@ public void testUtf8PerfDegradation() throws Exception {
doc.addField("id", "1");
doc.addField("b_is", IntStream.range(0, 30000).boxed().collect(Collectors.toList()));
- HttpSolrClient client = (HttpSolrClient) getSolrClient();
- client.add(doc);
- client.commit();
- long start = System.nanoTime();
- QueryResponse rsp = client.query(new SolrQuery("*:*"));
- System.out.println("time taken : " + ((System.nanoTime() - start)) / (1000 * 1000));
- assertEquals(1, rsp.getResults().getNumFound());
- }
-
- @Ignore
Review Comment:
The test was added with the `@Ignore` tag four years ago, so I suspect it wasn't really a test but was to figure some code out.... https://github.com/apache/solr/blame/26195c82493422cb9d6d4bdf9d4452046e7b3f67/solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java#L130.
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1302471764
The "LB" variants, I think of as being rather internal. Thus true immutability is not very important if we can see that within Solr's codebase, the setters are only ever invoked immediately after construction and not at arbitrary points afterwards.
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1011906493
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleTests.java:
##########
@@ -602,58 +602,60 @@ public void testUnicode() throws Exception {
SolrClient client = getSolrClient();
// save the old parser, so we can set it back.
- ResponseParser oldParser = null;
- if (client instanceof HttpSolrClient) {
- HttpSolrClient httpSolrClient = (HttpSolrClient) client;
- oldParser = httpSolrClient.getParser();
- }
-
- try {
- for (int iteration = 0; iteration < numIterations; iteration++) {
- // choose format
- if (client instanceof HttpSolrClient) {
- if (random.nextBoolean()) {
- ((HttpSolrClient) client).setParser(new BinaryResponseParser());
- } else {
- ((HttpSolrClient) client).setParser(new XMLResponseParser());
- }
- }
-
- int numDocs = TestUtil.nextInt(random(), 1, 10 * RANDOM_MULTIPLIER);
-
- // Empty the database...
- client.deleteByQuery("*:*"); // delete everything!
-
- List<SolrInputDocument> docs = new ArrayList<>();
- for (int i = 0; i < numDocs; i++) {
- // Now add something...
- SolrInputDocument doc = new SolrInputDocument();
- doc.addField("id", "" + i);
- doc.addField("unicode_s", randomTestString(30));
- docs.add(doc);
- }
+ // ResponseParser oldParser = null;
+ // if (client instanceof HttpSolrClient) {
+ // HttpSolrClient httpSolrClient = (HttpSolrClient) client;
+ // oldParser = httpSolrClient.getParser();
+ // }
Review Comment:
Actually hoping someone else can confirm that the changes I made still keep the test working "as designed", and that this commented out code can just be deleted....
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1011650146
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleStreamingBinaryTest.java:
##########
@@ -35,9 +34,15 @@ public class SolrExampleStreamingBinaryTest extends SolrExampleStreamingTest {
@Override
public SolrClient createNewSolrClient() {
- ConcurrentUpdateSolrClient client = (ConcurrentUpdateSolrClient) super.createNewSolrClient();
- client.setParser(new BinaryResponseParser());
- client.setRequestWriter(new BinaryRequestWriter());
+
+ SolrClient client =
+ new ErrorTrackingConcurrentUpdateSolrClient.Builder(getServerUrl())
+ .withQueueSize(2)
+ .withThreadCount(5)
Review Comment:
okay, now I remember.. I eliminate the `super.createNewSolrClient` partly because it was a pattern I had never seen used elsewhere, and in `SolrExampleStreamingTest` it looks like:
```
return new ErrorTrackingConcurrentUpdateSolrClient.Builder(getServerUrl())
.withQueueSize(2)
.withThreadCount(5)
.withResponseParser(new XMLResponseParser())
.withRequestWriter(new RequestWriter())
.build();
```
--
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 #1158: SOLR-16368: experiment with builder to simplify client creation
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1014386128
##########
solr/solrj/src/test/org/apache/solr/client/solrj/TestSolrJErrorHandling.java:
##########
@@ -102,16 +102,19 @@ public void showExceptions() throws Exception {
@Test
public void testWithXml() throws Exception {
- HttpSolrClient client = (HttpSolrClient) getSolrClient();
- client.setRequestWriter(new RequestWriter());
+ client =
+ new HttpSolrClient.Builder(getServerUrl()).withRequestWriter(new RequestWriter()).build();
+
Review Comment:
Done
--
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 #1158: SOLR-16368: Use Builder Pattern with Solr Clients
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1329890210
Can you please summarize the changes into a proposed commit message? I wish GitHub built this into the PR so that we could review it as it's rather important. If you start listing lots of extraneous things then it's a sign we should have more PRs (a subjective judgement call for 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] dsmiley commented on pull request #1158: SOLR-16368: Use Builder Pattern with Solr Clients
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1158:
URL: https://github.com/apache/solr/pull/1158#issuecomment-1329890511
Note that SOLR-16368 is marked "Resolved" already.
--
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