epugh commented on code in PR #2412:
URL: https://github.com/apache/solr/pull/2412#discussion_r1572623626
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -54,17 +54,12 @@ public class CloudHttp2SolrClient extends CloudSolrClient {
*/
protected CloudHttp2SolrClient(Builder builder) {
super(builder.shardLeadersOnly, builder.parallelUpdates,
builder.directUpdatesToLeadersOnly);
- if (builder.httpClient == null) {
- this.clientIsInternal = true;
- if (builder.internalClientBuilder == null) {
- this.myClient = new Http2SolrClient.Builder().build();
- } else {
- this.myClient = builder.internalClientBuilder.build();
- }
- } else {
- this.clientIsInternal = false;
- this.myClient = builder.httpClient;
- }
+ this.clientIsInternal = builder.httpClient == null;
+ this.myClient = createOrGetHttpClientFromBuilder(builder);
+ this.stateProvider =
Review Comment:
the two methods here seem much clearer on the logic paths.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -85,14 +79,62 @@ protected CloudHttp2SolrClient(Builder builder) {
this.lbClient = new LBHttp2SolrClient.Builder(myClient).build();
}
+ private Http2SolrClient createOrGetHttpClientFromBuilder(Builder builder) {
+ return builder.httpClient != null
Review Comment:
this logic feels a bit complex to follow? I'm sure it's right but it's a
lot of trinary logic? Is there anotherway to write this that would be deasier
to read?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientBuilderTest.java:
##########
@@ -172,4 +184,48 @@ public void
testDefaultCollectionPassedFromBuilderToClient() throws IOException
assertEquals("aCollection", createdClient.getDefaultCollection());
}
}
+
+ /**
+ * Tests the consistency between the HTTP client used by {@link
CloudHttp2SolrClient} and the one
+ * used by its associated {@link Http2ClusterStateProvider}. This method
ensures that whether a
+ * {@link CloudHttp2SolrClient} is created with a specific HTTP client, an
internal client
+ * builder, or with no specific HTTP client at all, the same HTTP client
instance is used both by
+ * the {@link CloudHttp2SolrClient} and by its {@link
Http2ClusterStateProvider}.
+ */
+ @Test
+ public void testHttpClientPreservedInHttp2ClusterStateProvider() throws
IOException {
+ List<String> solrUrls =
List.of(cluster.getJettySolrRunner(0).getBaseUrl().toString());
+
+ // No httClient - No internalClientBuilder
Review Comment:
in a few places ;-)
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -85,14 +79,62 @@ protected CloudHttp2SolrClient(Builder builder) {
this.lbClient = new LBHttp2SolrClient.Builder(myClient).build();
}
+ private Http2SolrClient createOrGetHttpClientFromBuilder(Builder builder) {
+ return builder.httpClient != null
+ ? builder.httpClient
+ : builder.internalClientBuilder != null
+ ? builder.internalClientBuilder.build()
+ : new Http2SolrClient.Builder().build();
+ }
+
+ private ClusterStateProvider createZkClusterStateProvider(Builder builder) {
+ try {
+ ClusterStateProvider stateProvider =
+ ClusterStateProvider.newZkClusterStateProvider(
+ builder.zkHosts, builder.zkChroot, builder.canUseZkACLs);
+ if (stateProvider instanceof SolrZkClientTimeoutAware) {
+ var timeoutAware = (SolrZkClientTimeoutAware) stateProvider;
+ timeoutAware.setZkClientTimeout(builder.zkClientTimeout);
+ timeoutAware.setZkConnectTimeout(builder.zkConnectTimeout);
+ }
+ return stateProvider;
+ } catch (Exception e) {
+ closeMyClientIfNeeded();
+ throw (e);
+ }
+ }
+
+ private ClusterStateProvider createHttp2ClusterStateProvider(
+ List<String> solrUrls, Http2SolrClient httpClient) {
+ try {
+ return new Http2ClusterStateProvider(solrUrls, httpClient);
+ } catch (Exception e) {
+ closeMyClientIfNeeded();
+ throw new RuntimeException(
+ "Couldn't initialize a HttpClusterStateProvider (is/are the "
+ + "Solr server(s), "
+ + solrUrls
+ + ", down?)",
+ e);
+ }
+ }
+
+ private void closeMyClientIfNeeded() {
+ try {
+ if (clientIsInternal && myClient != null) {
+ myClient.close();
+ }
+ } catch (Exception e) {
+ throw new RuntimeException("Exception on closing myClient", e);
+ }
+ }
+
@Override
public void close() throws IOException {
stateProvider.close();
lbClient.close();
- if (clientIsInternal && myClient != null) {
- myClient.close();
- }
+ closeMyClientIfNeeded();
Review Comment:
these type of conditional methods, is the pattern we use in Solr? I
personalily like the verbose ness, but I think normally it would just be
"closeMyClient())". Also, should we specify what client is "myClient"? We
hjave a lot of different clients.. is this a httpClient? a SolrClient?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientBuilderTest.java:
##########
@@ -172,4 +184,48 @@ public void
testDefaultCollectionPassedFromBuilderToClient() throws IOException
assertEquals("aCollection", createdClient.getDefaultCollection());
}
}
+
+ /**
+ * Tests the consistency between the HTTP client used by {@link
CloudHttp2SolrClient} and the one
+ * used by its associated {@link Http2ClusterStateProvider}. This method
ensures that whether a
+ * {@link CloudHttp2SolrClient} is created with a specific HTTP client, an
internal client
+ * builder, or with no specific HTTP client at all, the same HTTP client
instance is used both by
+ * the {@link CloudHttp2SolrClient} and by its {@link
Http2ClusterStateProvider}.
+ */
+ @Test
+ public void testHttpClientPreservedInHttp2ClusterStateProvider() throws
IOException {
+ List<String> solrUrls =
List.of(cluster.getJettySolrRunner(0).getBaseUrl().toString());
+
+ // No httClient - No internalClientBuilder
Review Comment:
typo.. `httpClient`
--
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
