dsmiley commented on code in PR #1012:
URL: https://github.com/apache/solr/pull/1012#discussion_r992564452
##########
solr/core/src/test/org/apache/solr/filestore/TestDistribPackageStore.java:
##########
@@ -309,11 +309,10 @@ public static <T extends NavigableObject> T
assertResponseValues(
public static void uploadKey(byte[] bytes, String path, MiniSolrCloudCluster
cluster)
throws Exception {
JettySolrRunner jetty = cluster.getRandomJetty(random());
- try (HttpSolrClient client = (HttpSolrClient) jetty.newClient()) {
- PackageUtils.uploadKey(
- bytes, path, Paths.get(jetty.getCoreContainer().getSolrHome()),
client);
+ try (HttpSolrClient solrClient = (HttpSolrClient) jetty.newClient()) {
+ PackageUtils.uploadKey(bytes, path,
Paths.get(jetty.getCoreContainer().getSolrHome()));
String url = jetty.getBaseURLV2() + "/node/files" + path + "?sync=true";
- Object resp = Utils.executeGET(client.getHttpClient(), url, null);
+ Object resp = Utils.executeGET(solrClient.getHttpClient(), url, null);
log.info("sync resp: {} was {}", url, resp);
}
Review Comment:
This code doesn't need an HttpSolrClient at all; it only needs some HTTP
client. Maybe we'll punt on this for now but hopefully we'll come back. At
least a comment for now?
##########
solr/core/src/test/org/apache/solr/handler/admin/ZookeeperStatusHandlerFailureTest.java:
##########
@@ -51,12 +51,13 @@ public void monitorZookeeperAfterZkShutdown()
throws IOException, SolrServerException, InterruptedException,
ExecutionException,
TimeoutException {
URL baseUrl = cluster.getJettySolrRunner(0).getBaseUrl();
- HttpSolrClient solr = new
HttpSolrClient.Builder(baseUrl.toString()).build();
+ HttpSolrClient solrClient = new
HttpSolrClient.Builder(baseUrl.toString()).build();
Review Comment:
Why change this file at all if you didn't change the declared type?
##########
solr/core/src/test/org/apache/solr/handler/admin/ZookeeperReadAPITest.java:
##########
@@ -60,19 +60,20 @@ public void testZkread() throws Exception {
String basezk = baseUrl.toString().replace("/solr", "/api") +
"/cluster/zk/data";
String basezkls = baseUrl.toString().replace("/solr", "/api") +
"/cluster/zk/ls";
- try (HttpSolrClient client = new
HttpSolrClient.Builder(baseUrl.toString()).build()) {
+ try (HttpSolrClient solrClient = new
HttpSolrClient.Builder(baseUrl.toString()).build()) {
Review Comment:
This SolrClient isn't actually used as-such. Maybe a TODO to avoid this
later.
Or just reverse -- don't change this file; the declared type is fine. I'd
have preferred "var" but I failed to convince you.
##########
solr/core/src/test/org/apache/solr/security/BasicAuthStandaloneTest.java:
##########
@@ -88,33 +88,37 @@ public void testBasicAuth() throws Exception {
String authcPrefix = "/admin/authentication";
String authzPrefix = "/admin/authorization";
- HttpClient cl = null;
- HttpSolrClient httpSolrClient = null;
+ HttpClient httpClient = null;
Review Comment:
nice renames to clarify!
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/TopicStream.java:
##########
@@ -509,7 +509,7 @@ private void getPersistedCheckpoints() throws IOException {
for (Replica replica : replicas) {
if (replica.getState() == Replica.State.ACTIVE
&& liveNodes.contains(replica.getNodeName())) {
- HttpSolrClient httpClient =
+ SolrClient httpClient =
Review Comment:
rename variable please! misleading name
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java:
##########
@@ -100,10 +100,11 @@ public synchronized CloudSolrClient
getCloudSolrClient(String zkHost) {
}
@Deprecated(since = "9.0")
- public synchronized HttpSolrClient getHttpSolrClient(String host) {
- HttpSolrClient client;
+ // is this really deprecated? what if it's getSolrClient? It COULD be!
Review Comment:
The name is fine because the argument is in fact a URL. But I agree it
should not be deprecated.
##########
solr/core/src/test/org/apache/solr/handler/admin/ZookeeperStatusHandlerTest.java:
##########
@@ -76,12 +76,13 @@ public void monitorZookeeper()
throws IOException, SolrServerException, InterruptedException,
ExecutionException,
TimeoutException {
URL baseUrl = cluster.getJettySolrRunner(0).getBaseUrl();
- HttpSolrClient solr = new
HttpSolrClient.Builder(baseUrl.toString()).build();
+ HttpSolrClient solrClient = new
HttpSolrClient.Builder(baseUrl.toString()).build();
Review Comment:
Why change this file at all if you didn't change the declared type?
##########
solr/core/src/test/org/apache/solr/update/SolrCmdDistributorTest.java:
##########
@@ -376,9 +376,9 @@ public void newSearcher(
}
private void testDeletes(boolean dbq, boolean withFailures) throws Exception
{
- final HttpSolrClient solrclient = (HttpSolrClient) clients.get(0);
- solrclient.commit(true, true);
- long numFoundBefore = solrclient.query(new
SolrQuery("*:*")).getResults().getNumFound();
+ final HttpSolrClient solrClient = (HttpSolrClient) clients.get(0);
+ solrClient.commit(true, true);
+ long numFoundBefore = solrClient.query(new
SolrQuery("*:*")).getResults().getNumFound();
Review Comment:
Why change this file at all if you didn't change the declared type?
##########
solr/core/src/test/org/apache/solr/TestDistributedSearch.java:
##########
@@ -1711,7 +1710,7 @@ public void test() throws Exception {
tdate_b,
"stats.calcdistinct",
"true");
- } catch (BaseHttpSolrClient.RemoteSolrException e) {
+ } catch (SolrException e) {
Review Comment:
Why?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java:
##########
@@ -100,10 +100,11 @@ public synchronized CloudSolrClient
getCloudSolrClient(String zkHost) {
}
@Deprecated(since = "9.0")
- public synchronized HttpSolrClient getHttpSolrClient(String host) {
- HttpSolrClient client;
+ // is this really deprecated? what if it's getSolrClient? It COULD be!
Review Comment:
I'd appreciate docs on the argument if you have time. It's possibly kind of
wrong; appears to be baseUrl actually?
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
