-
Notifications
You must be signed in to change notification settings - Fork 658
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SOLR-16503: Replace default USH apache client with Http2SolrClient #2741
base: main
Are you sure you want to change the base?
Conversation
SplitShardWithNodeRoleTest.testSolrClusterWithNodeRoleWithPull failed! Even in the past this one failed. If I look at the test, It seem simple, However the complexity that split operation involves is rather too much. This is how it runs.
I am not sure If increasing Timeout will help us. But we can try it! In the logs, one can see that just before the assertion failed, the IndexFetcher was running. May be the PULL replicas were downloading the index, and the time finished and the sub-shard never really recovered. Question: Do both NRT and PULL replicas need to be active for sub-shards to be active? Note :- I found a JIRA ticked related to this one https://issues.apache.org/jira/browse/SOLR-16753. |
solr/core/src/java/org/apache/solr/util/stats/InstrumentedHttpListenerFactory.java
Outdated
Show resolved
Hide resolved
solr/core/src/test/org/apache/solr/core/TestHttpSolrClientProvider.java
Outdated
Show resolved
Hide resolved
solr/core/src/test/org/apache/solr/core/TestHttpSolrClientProvider.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java
Outdated
Show resolved
Hide resolved
.withSocketTimeout(30000, TimeUnit.MILLISECONDS) | ||
.withConnectionTimeout(15000, TimeUnit.MILLISECONDS) | ||
.withHttpClient(updateShardHandler.getDefaultHttpClient()) | ||
.withZkClientTimeout(30000, TimeUnit.MILLISECONDS) | ||
.withZkConnectTimeout(15000, TimeUnit.MILLISECONDS) | ||
.withHttpClient(getCoreContainer().getDefaultHttpSolrClient()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
withSocketTimeout was on the HTTP connection at the LBHttpSolrClient layer not ZK one. The Http2/Jetty side doesn't quite work the same way. It creates an LBHttp2SolrClient without such customizations. Maybe they could bet set at the HttpClient (Jetty) layer, albeit this means you're not passing in AFAICT, there's no direct substitute for our Http2 builder; instead you'd have to create the Jetty HttpClient with those settings and then pass it in.
Aaaaanyway.... I don't think it's worth retaining such particulars here. I don't know why these specific timeouts were put here; they were added by @sigram in relation to ReindexCollection. I recommend we simply use the new getCoreContainer().getDefaultHttpSolrClient().
solr/core/src/java/org/apache/solr/cloud/api/collections/ReindexCollectionCmd.java
Show resolved
Hide resolved
.withZkConnectTimeout(15000, TimeUnit.MILLISECONDS) | ||
.withHttpClient(getCoreContainer().getDefaultHttpSolrClient()) | ||
.build()) { | ||
try (var solrClient = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this client is embedded into the Cloud one, I think you should name this var maybe internalClient or something and don't refer to it in the block of code for the try-finally except for passing into the CloudSolrClient.
BTW; it really is painful to customize these particular timeouts as we're forced to do it at an inner layer. Ugh. @epugh not sure if you worked on some of the SolrClient building methods and saw this issue before. Maybe we should add the same methods to the CloudHttp2SolrClient.Builder
var response = solrClient.requestWithBaseUrl(baseUrl, client -> client.request(request)); | ||
var response = solrClient.requestWithBaseUrl(baseUrl, request::process).getResponse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is rather nice. @gerlowskija, I think we can do away with the lambda-less requestWithBaseUrl I was suggesting. WDYT?
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java
Outdated
Show resolved
Hide resolved
|
||
InputStream is = null; | ||
var solrClient = coreContainer.getDefaultHttpSolrClient(); | ||
|
||
try { | ||
GenericSolrRequest request = new GenericSolrRequest(GET, "/node/files" + getMetaPath()); | ||
request.setResponseParser(new InputStreamResponseParser(null)); | ||
var response = solrClient.requestWithBaseUrl(baseUrl, request::process).getResponse(); | ||
is = (InputStream) response.get("stream"); | ||
metadata = | ||
Utils.executeGET( | ||
coreContainer.getUpdateShardHandler().getDefaultHttpClient(), | ||
baseUrl + "/node/files" + getMetaPath(), | ||
Utils.newBytesConsumer((int) MAX_PKG_SIZE)); | ||
Utils.newBytesConsumer((int) MAX_PKG_SIZE).accept((InputStream) response.get("stream")); | ||
m = (Map<?, ?>) Utils.fromJSON(metadata.array(), metadata.arrayOffset(), metadata.limit()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace needless use of the special InputStreamResponseParser with a standard JsonMapResponseParser. Perhaps don't even do that; we don't have to use JSON, I think; Solr will by default negotiate the format and get you a NamedList. (Map like thing).
var solrClient = coreContainer.getDefaultHttpSolrClient(); | ||
var resp = solrClient.requestWithBaseUrl(baseUrl, request::process).getResponse(); | ||
|
||
if (Utils.getObjectByPath(resp, false, Arrays.asList("files", path)) != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are convenience methods on NamedList avoiding the need to refer to this awkward utility method. Try findRecursive.
https://issues.apache.org/jira/browse/SOLR-16503
Replaced the default apache Http client provided by
UpdateShardHandler
with the Http2SolrClient provided by CoreContainer#getDefaultHttpSolrClient which will be introduced in #2689. As we are still deciding on what is the best way moving forward to set the url. Here, for now, almost everywhere a new Http2SolrClient is being re-created.Of course, If we decided to go with #2714 then setting up url and closing the instance would be replaced with a new abstraction.
Checklist
Please review the following and check all that apply:
main
branch../gradlew check
.