You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "epugh (via GitHub)" <gi...@apache.org> on 2023/03/28 13:26:56 UTC
[GitHub] [solr] epugh opened a new pull request, #1499: SOLR-16604: Use the Builders directly in unit tests
epugh opened a new pull request, #1499:
URL: https://github.com/apache/solr/pull/1499
https://issues.apache.org/jira/browse/SOLR-16604
# Description
Swap to using Builder directly.
# Solution
Using the non deprecated version of the Builders where possible, so as not to just add more use of the deprecate SolrClients.
# Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
# 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] epugh commented on pull request #1499: SOLR-16604: Use the Builders directly in unit tests
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1499:
URL: https://github.com/apache/solr/pull/1499#issuecomment-1486887898
@dsmiley starting the migration ;-)
--
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 #1499: SOLR-16604: Use the Builders directly in unit tests
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1499:
URL: https://github.com/apache/solr/pull/1499#issuecomment-1488352844
> For simple URL -> SolrClient (no further customization), I admit the STCJ4 method was nice. Can we keep these and just focus here on anything else?
Sure! Do you think we keep this name?
```
collection1 = getHttpSolrClient(url + "/collection1");
```
or maybe change it to
```
collection1 = getSolrClient(url + "/collection1");
```
?
I guess I am wondering if the `Http` part matters... Thinking that if Http matters, then you should use 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 pull request #1499: SOLR-16604: Use the Builders directly in unit tests
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1499:
URL: https://github.com/apache/solr/pull/1499#issuecomment-1488915814
> > For simple URL -> SolrClient (no further customization), I admit the STCJ4 method was nice. Can we keep these and just focus here on anything else?
>
> Sure! Do you think we keep this name?
>
> ```
> collection1 = getHttpSolrClient(url + "/collection1");
> ```
>
> or maybe change it to
>
> ```
> collection1 = getSolrClient(url + "/collection1");
> ```
>
> ?
>
> I guess I am wondering if the `Http` part matters... Thinking that if Http matters, then you should use a Builder...
@dsmiley checkout out 47d7190b71eb68b6857506053ce91e296d3b77d0 where I went ahead and kept the use of Http2SolrClient, but used the helper method on SolrTestCaseJ4.
--
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 #1499: SOLR-16604: Use the Builders directly in unit tests
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1499:
URL: https://github.com/apache/solr/pull/1499#discussion_r1153320060
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractUnloadDistributedZkTestBase.java:
##########
@@ -285,7 +287,11 @@ private void testCoreUnloadAndLeaders() throws Exception {
// collectionClient.commit();
// unload the leader
- try (SolrClient collectionClient = getHttpSolrClient(leaderProps.getBaseUrl(), 15000, 30000)) {
+ try (SolrClient collectionClient =
+ new HttpSolrClient.Builder(leaderProps.getBaseUrl())
+ .withConnectionTimeout(15000, TimeUnit.MILLISECONDS)
+ .withSocketTimeout(30000, TimeUnit.MILLISECONDS)
+ .build()) {
Review Comment:
This test class seems to always want a client with specific timeouts. Then create a utility method _in this test_ for it.
##########
solr/core/src/test/org/apache/solr/cloud/LeaderVoteWaitTimeoutTest.java:
##########
@@ -343,6 +344,6 @@ private NamedList<Object> realTimeGetDocId(SolrClient solr, String docId)
protected SolrClient getSolrClient(Replica replica, String coll) {
ZkCoreNodeProps zkProps = new ZkCoreNodeProps(replica);
String url = zkProps.getBaseUrl() + "/" + coll;
- return getHttpSolrClient(url);
+ return new Http2SolrClient.Builder(url).build();
Review Comment:
just a url; can use the STCJ4 method
##########
solr/core/src/test/org/apache/solr/cloud/HttpPartitionTest.java:
##########
@@ -543,7 +544,7 @@ protected void assertDocsExistInAllReplicas(
protected SolrClient getHttpSolrClient(Replica replica, String collection) {
ZkCoreNodeProps zkProps = new ZkCoreNodeProps(replica);
String url = zkProps.getBaseUrl() + "/" + collection;
- return getHttpSolrClient(url);
+ return new Http2SolrClient.Builder(url).build();
Review Comment:
just a url; can use the STCJ4 method
##########
solr/core/src/test/org/apache/solr/cloud/api/collections/ShardSplitTest.java:
##########
@@ -1268,7 +1268,11 @@ protected void splitShard(
((HttpSolrClient) shardToJetty.get(SHARD1).get(0).client.getSolrClient()).getBaseURL();
baseUrl = baseUrl.substring(0, baseUrl.length() - "collection1".length());
- try (SolrClient baseServer = getHttpSolrClient(baseUrl, 30000, 60000 * 5)) {
+ try (SolrClient baseServer =
+ new HttpSolrClient.Builder(baseUrl)
+ .withConnectionTimeout(30000, TimeUnit.MILLISECONDS)
+ .withSocketTimeout(60000 * 5, TimeUnit.MILLISECONDS)
Review Comment:
It's sad to see this... can we just say 5 minutes?
##########
solr/core/src/test/org/apache/solr/cloud/TestCloudConsistency.java:
##########
@@ -351,6 +352,6 @@ private NamedList<Object> realTimeGetDocId(SolrClient solr, String docId)
protected SolrClient getHttpSolrClient(Replica replica, String coll) {
ZkCoreNodeProps zkProps = new ZkCoreNodeProps(replica);
String url = zkProps.getBaseUrl() + "/" + coll;
- return getHttpSolrClient(url);
+ return new Http2SolrClient.Builder(url).build();
Review Comment:
just a url; can use the STCJ4 method
##########
solr/test-framework/src/test/org/apache/solr/embedded/TestJettySolrRunner.java:
##########
@@ -55,7 +56,8 @@ public void testPassSolrHomeToRunner() throws Exception {
try {
runner.start();
- try (SolrClient client = getHttpSolrClient(runner.getBaseUrl().toString())) {
+ try (SolrClient client =
+ new Http2SolrClient.Builder(runner.getBaseUrl().toString()).build()) {
Review Comment:
you were going to keep these as they were; no? (only URL arg)
--
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 #1499: SOLR-16604: Use the Builders directly in unit tests
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1499:
URL: https://github.com/apache/solr/pull/1499#issuecomment-1489340812
> CloudSolrClient getCloudSolrClient(String zkHost) --> 5 usages in 3 files
Let's lose it.
> CloudSolrClient getCloudSolrClient(MiniSolrCloudCluster cluster) --> 11 usages in 8 files
Begs for an instance method on MiniSolrCloudCluster then :-). Also, I'd much prefer "new" instead of "get" because it's a new object that must be closed by the caller. "get" implies zero responsibility on the caller's part as to the lifecycle. A best-practice, really. Yes there are other "get" methods violating this best-practice but leave for another day IMO.
--
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 #1499: SOLR-16604: Use the Builders directly in unit tests
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh merged PR #1499:
URL: https://github.com/apache/solr/pull/1499
--
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 #1499: SOLR-16604: Use the Builders directly in unit tests
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1499:
URL: https://github.com/apache/solr/pull/1499#issuecomment-1489132628
I suggest leaving `getHttpSolrClient` (its name & single arg) be for now. Just to limit the scope here and it's unclear what may become of this with further test infra improvements. It's extremely popular to call this. The "Http" part makes sense -- it *is* HTTP based.
--
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 #1499: SOLR-16604: Use the Builders directly in unit tests
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1499:
URL: https://github.com/apache/solr/pull/1499#issuecomment-1490387080
Tests ran for me @dsmiley... What do you think? Is this ready for merging? It's the last sub ticket under SOLR-8975!!!
--
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 #1499: SOLR-16604: Use the Builders directly in unit tests
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1499:
URL: https://github.com/apache/solr/pull/1499#issuecomment-1489198684
> I suggest leaving `getHttpSolrClient` (its name & single arg) be for now. Just to limit the scope here and it's unclear what may become of this with further test infra improvements. It's extremely popular to call this. The "Http" part makes sense -- it _is_ HTTP based.
Okay, 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 #1499: SOLR-16604: Use the Builders directly in unit tests
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1499:
URL: https://github.com/apache/solr/pull/1499#issuecomment-1489227999
Okay, at this point, we have two CloudSolrClient related methods remaining...
```
CloudSolrClient getCloudSolrClient(String zkHost) --> 5 usages in 3 files
CloudSolrClient getCloudSolrClient(MiniSolrCloudCluster cluster) --> 11 usages in 8 files
```
I think then this is probably 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