dsmiley commented on code in PR #4526:
URL: https://github.com/apache/solr/pull/4526#discussion_r3409022435
##########
solr/core/src/test/org/apache/solr/servlet/TestRequestRateLimiter.java:
##########
@@ -429,6 +432,14 @@ public SlotReservation allowSlotBorrowing() throws
InterruptedException {
}
}
+ private CloudSolrClient newHttp1SolrClient(String collection) {
+ return new CloudJettySolrClient.Builder(
+ new
ZkClientClusterStateProvider(cluster.getZkServer().getZkAddress()))
+ .withDefaultCollection(collection)
+ .withHttpClientBuilder(new
HttpJettySolrClient.Builder().useHttp1_1(true))
Review Comment:
Claude's root cause, and suggested this fix:
When 350 concurrent queries hit the rate limiter and get rejected as 429s,
the server sends rapid RST_STREAM frames on the multiplexed HTTP/2 connection.
This triggers HTTP/2's ENHANCE_YOUR_CALM protection, which tears down the
entire connection. Subsequent requests then fail with IOException, which the
CloudSolrClient sees as a communication error → "No live SolrServers."
The old Apache HTTP client used HTTP/1.1 (separate connections per request),
so this never happened. The Jetty client defaults to HTTP/2.
##########
solr/core/src/test/org/apache/solr/servlet/TestRequestRateLimiter.java:
##########
@@ -56,7 +59,7 @@
import org.junit.Test;
import org.mockito.Mockito;
[email protected](bugUrl =
"https://issues.apache.org/jira/browse/SOLR-17810";)
+//@LuceneTestCase.AwaitsFix(bugUrl =
"https://issues.apache.org/jira/browse/SOLR-17810";)
Review Comment:
will remove
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java:
##########
@@ -240,14 +241,11 @@ protected NamedList<Object> processErrorsAndResponse(
}
if (httpStatus != 200 && !isV2Api) {
if (error == null) {
- StringBuilder msg =
- new StringBuilder()
- .append(responseReason)
- .append("\n")
- .append("request: ")
- .append(responseMethod);
- String reason = URLDecoder.decode(msg.toString(), FALLBACK_CHARSET);
- throw new RemoteSolrException(urlExceptionMessage, httpStatus,
reason, null);
+ throw new RemoteSolrException(
+ urlExceptionMessage,
+ httpStatus,
+ "non ok status: " + httpStatus + ", message:" + responseReason,
+ null);
Review Comment:
@HoustonPutman , you added this in SOLR-17998 #1657 I didn't see much
sense in this particular string so I changed it to match the message string
above line 217
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java:
##########
@@ -218,15 +219,15 @@ protected NamedList<Object> processErrorsAndResponse(
}
}
+ checkContentType(processor, is, mimeType, encoding, httpStatus,
urlExceptionMessage);
Review Comment:
moved this above wantStream / InputStreamResponseParser because I think it
makes sense to check this fairly early. Someone could subclass ISRP and
specify the content types that should be checked.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java:
##########
@@ -209,7 +210,7 @@ protected NamedList<Object> processErrorsAndResponse(
}
break;
default:
- if (processor == null || mimeType == null) {
+ if (mimeType == null || mimeType.equals("text/html")) {
Review Comment:
the meat of the change.
--
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]
