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/10/30 06:08:22 UTC
[GitHub] [solr] joshgog opened a new pull request, #1149: SOLR-16498: Tests to retrieve url from SolrClient
joshgog opened a new pull request, #1149:
URL: https://github.com/apache/solr/pull/1149
https://issues.apache.org/jira/browse/SOLR-16498
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Solr:
* https://issues.apache.org/jira/projects/SOLR
For something minor (i.e. that wouldn't be worth putting in release notes), you can skip JIRA.
To create a Jira issue, you will need to create an account there first.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# Description
Some tests are either getting the URL from legacy/deprecated SolrClient classes or Apache HttpClient instance out of the client. Tests need to get the URL from the SolrClient in a more general way..
# Solution
This is still a work in progress. Created a utility method that interrogates the passed SolrClient and returns the base url.
# Tests
Tests are not needed since this PR refactors existing tests.
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1149:
URL: https://github.com/apache/solr/pull/1149#discussion_r1033740581
##########
solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java:
##########
@@ -261,23 +259,22 @@ private NamedList<Object> getIndexVersion(SolrClient s) throws Exception {
return res;
}
- private NamedList<Object> reloadCore(SolrClient s, String core) throws Exception {
+ private void reloadCore(JettySolrRunner jettySolrRunner, String core) throws Exception {
Review Comment:
I wonder if we need a JIRA to "Go through and migrate Tests that don't require HTTP to not use Jetty"?
--
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 #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1149:
URL: https://github.com/apache/solr/pull/1149#discussion_r1034143179
##########
solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java:
##########
@@ -261,23 +259,22 @@ private NamedList<Object> getIndexVersion(SolrClient s) throws Exception {
return res;
}
- private NamedList<Object> reloadCore(SolrClient s, String core) throws Exception {
+ private void reloadCore(JettySolrRunner jettySolrRunner, String core) throws Exception {
ModifiableSolrParams params = new ModifiableSolrParams();
params.set("action", "reload");
params.set("core", core);
params.set("qt", "/admin/cores");
QueryRequest req = new QueryRequest(params);
- try (SolrClient adminClient = adminClient(s)) {
+ try (SolrClient adminClient = adminClient(jettySolrRunner)) {
NamedList<Object> res = adminClient.request(req);
assertNotNull("null response from server", res);
- return res;
}
}
- private SolrClient adminClient(SolrClient client) {
- String adminUrl = ((HttpSolrClient) client).getBaseURL().replace("/collection1", "");
+ private SolrClient adminClient(JettySolrRunner client) {
+ String adminUrl = client.getBaseUrl().toString().replace("/collection1", "");
return getHttpSolrClient(adminUrl);
}
Review Comment:
https://issues.apache.org/jira/browse/SOLR-16563
--
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 #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1149:
URL: https://github.com/apache/solr/pull/1149#issuecomment-1329433986
@joshgog I'm going to mark this as Ready for Review ;-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] joshgog commented on a diff in pull request #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
joshgog commented on code in PR #1149:
URL: https://github.com/apache/solr/pull/1149#discussion_r1009450598
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -3235,6 +3236,14 @@ private static boolean isChildDoc(Object o) {
return o instanceof SolrInputDocument;
}
+ public static String getUrlFrom(SolrClient solrClient) {
+ if (solrClient instanceof HttpSolrClient) {
+ return ((HttpSolrClient) solrClient).getBaseURL();
+ } else {
+ return null;
Review Comment:
> I _guess_ maybe if you had random types of `SolrClient` being created you would need an else?
If its ok to add `getBaseURL()` method to other clients, I intend to use else if statements to check `instanceOf `other clients and the final `else `statement to raise an exception
--
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 #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1149:
URL: https://github.com/apache/solr/pull/1149#issuecomment-1328360108
@joshgog I got excited about eliminating lots more `HttpSolrClient` references, and got a lot more done on this branch. I'm going to wait till I can run a full `gradle test` before I check in my commits. I basically kept up the pattern that you started on this, so thanks very much for starting this! Will hopefully commit tomorrow the work I've 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 #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1149:
URL: https://github.com/apache/solr/pull/1149#issuecomment-1297173107
For fun, I applied the pattern to another unit test, `TestReplicationHandler` to see how it looked... I like that we hid the `HttpSolrClient` away! Thoughts @dsmiley ? Are we on the right track?
--
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 #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1149:
URL: https://github.com/apache/solr/pull/1149#discussion_r1033801926
##########
solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesDistrib.java:
##########
@@ -1489,6 +1489,12 @@ private String getBaseUrl(String id) {
return slice.getLeader().getCoreUrl();
}
+ protected String getBaseUrl(HttpSolrClient client) {
+ return client
+ .getBaseURL()
+ .substring(0, client.getBaseURL().length() - DEFAULT_COLLECTION.length() - 1);
+ }
Review Comment:
adding a comment.. and someday, when we eliminate HttpSolrClient, will have to properly figure out how to get the underlying Jetty server...
--
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 #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1149:
URL: https://github.com/apache/solr/pull/1149#discussion_r1033692635
##########
solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java:
##########
@@ -261,23 +259,22 @@ private NamedList<Object> getIndexVersion(SolrClient s) throws Exception {
return res;
}
- private NamedList<Object> reloadCore(SolrClient s, String core) throws Exception {
+ private void reloadCore(JettySolrRunner jettySolrRunner, String core) throws Exception {
ModifiableSolrParams params = new ModifiableSolrParams();
params.set("action", "reload");
params.set("core", core);
params.set("qt", "/admin/cores");
QueryRequest req = new QueryRequest(params);
- try (SolrClient adminClient = adminClient(s)) {
+ try (SolrClient adminClient = adminClient(jettySolrRunner)) {
NamedList<Object> res = adminClient.request(req);
assertNotNull("null response from server", res);
- return res;
}
}
- private SolrClient adminClient(SolrClient client) {
- String adminUrl = ((HttpSolrClient) client).getBaseURL().replace("/collection1", "");
+ private SolrClient adminClient(JettySolrRunner client) {
+ String adminUrl = client.getBaseUrl().toString().replace("/collection1", "");
return getHttpSolrClient(adminUrl);
}
Review Comment:
Do you want to create a JIRA and associate it with the right other jira's to tackle this in the future?
--
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 #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1149:
URL: https://github.com/apache/solr/pull/1149#discussion_r1033551745
##########
solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java:
##########
@@ -92,7 +90,7 @@
*
* @since 1.4
*/
-@Nightly
+// @Nightly
Review Comment:
Not on purpose?
##########
solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java:
##########
@@ -261,23 +259,22 @@ private NamedList<Object> getIndexVersion(SolrClient s) throws Exception {
return res;
}
- private NamedList<Object> reloadCore(SolrClient s, String core) throws Exception {
+ private void reloadCore(JettySolrRunner jettySolrRunner, String core) throws Exception {
ModifiableSolrParams params = new ModifiableSolrParams();
params.set("action", "reload");
params.set("core", core);
params.set("qt", "/admin/cores");
QueryRequest req = new QueryRequest(params);
- try (SolrClient adminClient = adminClient(s)) {
+ try (SolrClient adminClient = adminClient(jettySolrRunner)) {
NamedList<Object> res = adminClient.request(req);
assertNotNull("null response from server", res);
- return res;
}
}
- private SolrClient adminClient(SolrClient client) {
- String adminUrl = ((HttpSolrClient) client).getBaseURL().replace("/collection1", "");
+ private SolrClient adminClient(JettySolrRunner client) {
+ String adminUrl = client.getBaseUrl().toString().replace("/collection1", "");
return getHttpSolrClient(adminUrl);
}
Review Comment:
This method demonstrates a problem that has bothered me with Solr's Http clients for a long time. The user can construct an HttpSolrClient with a URL that includes the collection, but HttpSolrClient doesn't "know" this. This has consequences, such as the existence of this method and issues elsewhere. Ideally, the "base URL" would be provided and optionally a default collection/core name. Then, here, we wouldn't need another SolrClient instance simply because we want to issue admin commands. In a test setting, we could even avoid Jetty/HTTP. Granted, addressing that is definitely out of scope.
For now, without trying to fix all imperfections at once, the change here is fine.
##########
solr/core/src/test/org/apache/solr/servlet/CacheHeaderTest.java:
##########
@@ -59,7 +59,7 @@ public void testCacheVetoHandler() throws Exception {
f.getCanonicalPath(),
CommonParams.STREAM_CONTENTTYPE,
"text/csv");
- HttpResponse response = getClient().execute(m);
+ HttpResponse response = getHttpClient().execute(m);
Review Comment:
normally I wouldn't like the direction of this -- generic to HTTP specific, but here, clearly the test is related to HTTP.
##########
solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java:
##########
@@ -261,23 +259,22 @@ private NamedList<Object> getIndexVersion(SolrClient s) throws Exception {
return res;
}
- private NamedList<Object> reloadCore(SolrClient s, String core) throws Exception {
+ private void reloadCore(JettySolrRunner jettySolrRunner, String core) throws Exception {
Review Comment:
Previously we needed a SolrClient (thus could even be EmbeddedSolrServer in theory) and now we require Jetty, an HTTP server. Since this test is private, and I'm merely observing from the diff, maybe this is moot since it's a test that presumably is already using Jetty so there is no practical difference. But I'd prefer we not insist on Jetty unless we actually need to work with HTTP for some reason.
##########
solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesDistrib.java:
##########
@@ -1489,6 +1489,12 @@ private String getBaseUrl(String id) {
return slice.getLeader().getCoreUrl();
}
+ protected String getBaseUrl(HttpSolrClient client) {
+ return client
+ .getBaseURL()
+ .substring(0, client.getBaseURL().length() - DEFAULT_COLLECTION.length() - 1);
+ }
Review Comment:
add a comment to say that we assume that client's URL ends with `/collection1`.
But you still have client.getBaseURL which we're trying to get rid of. So...
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractSyncSliceTestBase.java:
##########
@@ -102,9 +101,7 @@ public void test() throws Exception {
QueryRequest request = new QueryRequest(params);
request.setPath("/admin/collections");
- String baseUrl =
- ((HttpSolrClient) shardToJetty.get("shard1").get(2).client.solrClient).getBaseURL();
- baseUrl = baseUrl.substring(0, baseUrl.length() - "collection1".length());
+ String baseUrl = shardToJetty.get(SHARD1).get(2).jetty.getBaseUrl().toString();
Review Comment:
much nicer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] joshgog commented on a diff in pull request #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
joshgog commented on code in PR #1149:
URL: https://github.com/apache/solr/pull/1149#discussion_r1009442214
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -3235,6 +3236,14 @@ private static boolean isChildDoc(Object o) {
return o instanceof SolrInputDocument;
}
+ public static String getUrlFrom(SolrClient solrClient) {
+ if (solrClient instanceof HttpSolrClient) {
Review Comment:
That was is what I had in mind but then only HttpSolrClient has a baseUrl field including getter and setter methods. Will it be ok to add the same to other 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] epugh commented on pull request #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1149:
URL: https://github.com/apache/solr/pull/1149#issuecomment-1329716560
> So I see `DEFAULT_COLLECTION` used in a few places and `collection1` hardcoded in others. I think I prefer `DEFAULT_COLLECTION` if possible to avoid hard coding `collection1`?
so, `DEFAULT_COLLECTION` is defined twice in the code base, and as a private variable. How about we leave this as is, and if you create me a JIRA, I'll happily do anohter PR that focuses on cleaning up all the places we mix "collection1" and "DEFAULT_COLLECTION"??/
--
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 #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1149:
URL: https://github.com/apache/solr/pull/1149#discussion_r1009507911
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -3235,6 +3236,14 @@ private static boolean isChildDoc(Object o) {
return o instanceof SolrInputDocument;
}
+ public static String getUrlFrom(SolrClient solrClient) {
+ if (solrClient instanceof HttpSolrClient) {
Review Comment:
lets get some feedback from @dsmiley on our direction!
--
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 #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1149:
URL: https://github.com/apache/solr/pull/1149#discussion_r1009414092
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -3235,6 +3236,14 @@ private static boolean isChildDoc(Object o) {
return o instanceof SolrInputDocument;
}
+ public static String getUrlFrom(SolrClient solrClient) {
+ if (solrClient instanceof HttpSolrClient) {
+ return ((HttpSolrClient) solrClient).getBaseURL();
+ } else {
+ return null;
Review Comment:
I *think* that this should raise an exception, not a null... since it's test code, we wouldn't ever call this where a null would make sense? I *guess* maybe if you had random types of `SolrClient` being created you would need an else? Right now however that owuldn't happen.
--
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 pull request #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #1149:
URL: https://github.com/apache/solr/pull/1149#issuecomment-1329723507
My concern was along the lines of the newly added `collection1` stuff in this PR specifically.
--
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 #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1149:
URL: https://github.com/apache/solr/pull/1149#discussion_r1033741853
##########
solr/core/src/test/org/apache/solr/servlet/CacheHeaderTest.java:
##########
@@ -59,7 +59,7 @@ public void testCacheVetoHandler() throws Exception {
f.getCanonicalPath(),
CommonParams.STREAM_CONTENTTYPE,
"text/csv");
- HttpResponse response = getClient().execute(m);
+ HttpResponse response = getHttpClient().execute(m);
Review Comment:
Yeah, I renamed it because I got confused with HttpClient versus SolrClients ;-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh merged pull request #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
epugh merged PR #1149:
URL: https://github.com/apache/solr/pull/1149
--
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 #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1149:
URL: https://github.com/apache/solr/pull/1149#discussion_r1033591963
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -2841,8 +2836,9 @@ static CollectionAdminResponse getStatusResponse(String requestId, SolrClient cl
}
protected void setupRestTestHarnesses() {
- for (final SolrClient client : clients) {
- RestTestHarness harness = new RestTestHarness(() -> ((HttpSolrClient) client).getBaseURL());
+ for (final JettySolrRunner jetty : jettys) {
+ RestTestHarness harness =
Review Comment:
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `AbstractFullDistribZkTestBase.setupRestTestHarnesses()` 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=356018482&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=356018482&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=356018482&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=356018482&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=356018482&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 #1149: SOLR-16498: Tests to retrieve url from SolrClient
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1149:
URL: https://github.com/apache/solr/pull/1149#discussion_r1009412874
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -3235,6 +3236,14 @@ private static boolean isChildDoc(Object o) {
return o instanceof SolrInputDocument;
}
+ public static String getUrlFrom(SolrClient solrClient) {
+ if (solrClient instanceof HttpSolrClient) {
Review Comment:
I imagine we would want to check instaceof for other clients too?
--
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