jdyer1 commented on code in PR #3774:
URL: https://github.com/apache/solr/pull/3774#discussion_r2437377138
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -73,15 +78,22 @@ protected CloudHttp2SolrClient(Builder builder) {
// locks.
this.locks = objectList(builder.parallelCacheRefreshesLocks);
- this.lbClient = new
LBHttp2SolrClient.Builder<Http2SolrClient>(myClient).build();
+ this.lbClient = new LBHttp2SolrClient.Builder<>(myClient).build();
}
- private Http2SolrClient createOrGetHttpClientFromBuilder(Builder builder) {
+ private HttpSolrClientBase createOrGetHttpClientFromBuilder(Builder builder)
{
if (builder.httpClient != null) {
return builder.httpClient;
} else if (builder.internalClientBuilder != null) {
return builder.internalClientBuilder.build();
} else {
+ try {
Review Comment:
great idea.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java:
##########
@@ -379,24 +380,31 @@ protected boolean maybeTryHeadRequest(String url) {
if (forceHttp11 || url == null ||
url.toLowerCase(Locale.ROOT).startsWith("https://";)) {
return true;
}
- return maybeTryHeadRequestSync(url);
- }
-
- protected volatile boolean headRequested; // must be threadsafe
- private boolean headSucceeded; // must be threadsafe
-
- private synchronized boolean maybeTryHeadRequestSync(String url) {
- if (headRequested) {
- return headSucceeded;
- }
-
URI uriNoQueryParams;
try {
- uriNoQueryParams = new URI(url);
+ var uriWithParams = new URI(url);
+ var uriNoPQueryParamsStr =
+ uriWithParams.getScheme()
+ + "://"
+ + uriWithParams.getAuthority()
+ + uriWithParams.getPath();
+ uriNoQueryParams = new URI(uriNoPQueryParamsStr);
} catch (URISyntaxException e) {
// If the url is invalid, let a subsequent request try again.
return false;
}
+ return maybeTryHeadRequestSync(uriNoQueryParams);
+ }
+
+ protected final Map<URI, Boolean> headSucceededByBaseUri =
Review Comment:
`CloudHttp2SolrClientTest` fails without this. It is not that one shard is
http/1 and the other is http/2, it is that it seems the JDK code also is
tracking which hosts it knows for sure using http/2. So we were sending the
HEAD request to the first shard and the JDK knew that shard was http/2, but for
the second shard, it would still be sending a jetty-incompatible upgrade
request. This change sends the HEAD request once to every shard and then all
the components are happy.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/SolrClientNodeStateProvider.java:
##########
@@ -59,6 +59,10 @@ public class SolrClientNodeStateProvider implements
NodeStateProvider, MapWriter
private Map<String, Map> nodeVsTags = new HashMap<>();
public SolrClientNodeStateProvider(CloudHttp2SolrClient solrClient) {
+ if (!(solrClient.getHttpClient() instanceof Http2SolrClient)) {
Review Comment:
these call the very special "requestwithbaseUrl" method on the jetty client.
I was unable to quickly create a common api for this with both clients so I
decided for now we just need to enforce the jetty client. In practice, I would
think these are only used internally so nothing is going to pass the jdk client
here.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -73,15 +78,22 @@ protected CloudHttp2SolrClient(Builder builder) {
// locks.
this.locks = objectList(builder.parallelCacheRefreshesLocks);
- this.lbClient = new
LBHttp2SolrClient.Builder<Http2SolrClient>(myClient).build();
+ this.lbClient = new LBHttp2SolrClient.Builder<>(myClient).build();
}
- private Http2SolrClient createOrGetHttpClientFromBuilder(Builder builder) {
+ private HttpSolrClientBase createOrGetHttpClientFromBuilder(Builder builder)
{
if (builder.httpClient != null) {
return builder.httpClient;
} else if (builder.internalClientBuilder != null) {
return builder.internalClientBuilder.build();
} else {
+ try {
+ Class.forName("org.eclipse.jetty.client.HttpClient");
+ } catch (ClassNotFoundException e) {
Review Comment:
I manually tested that it correctly picks the jdk client when jetty is
absent. I also added a unit test for the case where jetty is present. I
wasn't brave enough to try and write the unit test with jetty absent.
--
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]
