dsmiley commented on code in PR #2751:
URL: https://github.com/apache/solr/pull/2751#discussion_r1809154494
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/SolrClientNodeStateProvider.java:
##########
@@ -290,15 +290,15 @@ public SimpleSolrResponse invoke(String solrNode, String
path, SolrParams params
String url =
zkClientClusterStateProvider.getZkStateReader().getBaseUrlForNodeName(solrNode);
GenericSolrRequest request = new
GenericSolrRequest(SolrRequest.METHOD.POST, path, params);
- try (var client =
- new HttpSolrClient.Builder()
- .withHttpClient(solrClient.getHttpClient())
- .withBaseSolrUrl(url)
- .withResponseParser(new BinaryResponseParser())
- .build()) {
- NamedList<Object> rsp = client.request(request);
+ request.setResponseParser(new BinaryResponseParser());
+
+ try {
+ var solrClient = cloudSolrClient.getHttpClient();
+ NamedList<Object> rsp = solrClient.requestWithBaseUrl(url,
request::process).getResponse();
request.response.setResponse(rsp);
return request.response;
+ } catch (SolrServerException | IOException e) {
+ throw new RuntimeException(e);
Review Comment:
We avoid directly throwing RuntimeException in Solr; see SolrException
instead
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/SolrClientNodeStateProvider.java:
##########
@@ -290,15 +290,15 @@ public SimpleSolrResponse invoke(String solrNode, String
path, SolrParams params
String url =
zkClientClusterStateProvider.getZkStateReader().getBaseUrlForNodeName(solrNode);
GenericSolrRequest request = new
GenericSolrRequest(SolrRequest.METHOD.POST, path, params);
- try (var client =
- new HttpSolrClient.Builder()
- .withHttpClient(solrClient.getHttpClient())
- .withBaseSolrUrl(url)
- .withResponseParser(new BinaryResponseParser())
- .build()) {
- NamedList<Object> rsp = client.request(request);
+ request.setResponseParser(new BinaryResponseParser());
+
+ try {
+ var solrClient = cloudSolrClient.getHttpClient();
Review Comment:
rename this var httpSolrClient please
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/SolrClientCloudManager.java:
##########
@@ -51,21 +37,17 @@
public class SolrClientCloudManager implements SolrCloudManager {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- protected final CloudLegacySolrClient solrClient;
+ private final CloudHttp2SolrClient cloudSolrClient;
private final ZkDistribStateManager stateManager;
private final ZkStateReader zkStateReader;
private final SolrZkClient zkClient;
private final ObjectCache objectCache;
private final boolean closeObjectCache;
private volatile boolean isClosed;
- public SolrClientCloudManager(CloudLegacySolrClient solrClient) {
- this(solrClient, null);
- }
-
- public SolrClientCloudManager(CloudLegacySolrClient solrClient, ObjectCache
objectCache) {
- this.solrClient = solrClient;
- this.zkStateReader = ZkStateReader.from(solrClient);
+ public SolrClientCloudManager(ObjectCache objectCache, CloudHttp2SolrClient
client) {
Review Comment:
why the argument order flip?
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -769,6 +773,7 @@ public void close() {
sysPropsCacher.close();
customThreadPool.execute(() -> IOUtils.closeQuietly(cloudSolrClient));
+ customThreadPool.execute(() -> IOUtils.closeQuietly(http2SolrClient));
customThreadPool.execute(() -> IOUtils.closeQuietly(cloudManager));
Review Comment:
minor: order should flip -- close in reverse order as are created
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/SolrClientNodeStateProvider.java:
##########
@@ -290,15 +290,15 @@ public SimpleSolrResponse invoke(String solrNode, String
path, SolrParams params
String url =
zkClientClusterStateProvider.getZkStateReader().getBaseUrlForNodeName(solrNode);
GenericSolrRequest request = new
GenericSolrRequest(SolrRequest.METHOD.POST, path, params);
- try (var client =
- new HttpSolrClient.Builder()
- .withHttpClient(solrClient.getHttpClient())
- .withBaseSolrUrl(url)
- .withResponseParser(new BinaryResponseParser())
- .build()) {
- NamedList<Object> rsp = client.request(request);
+ request.setResponseParser(new BinaryResponseParser());
+
+ try {
+ var solrClient = cloudSolrClient.getHttpClient();
Review Comment:
or just inline it; no need to even declare it
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/SolrClientNodeStateProvider.java:
##########
@@ -54,15 +54,15 @@
public class SolrClientNodeStateProvider implements NodeStateProvider,
MapWriter {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- private final CloudLegacySolrClient solrClient;
+ private final CloudHttp2SolrClient cloudSolrClient;
Review Comment:
Do you think renaming the var is important? (I don't)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
