SOLR-17516: LBHttpSolrClient: support HttpJdkSolrClient (Generic Version) #2828
Conversation
- Fix JDK Client to throw the correct exception when IOException occurs
| @@ -173,8 +173,8 @@ public NamedList<Object> request(SolrRequest<?> solrRequest, String collection) | |||
| "Timeout occurred while waiting response from server at: " + pReq.url, e); | |||
| } catch (SolrException se) { | |||
| throw se; | |||
| } catch (RuntimeException re) { | |||
| throw new SolrServerException(re); | |||
| } catch (IOException | RuntimeException e) { | |||
There was a problem hiding this comment.
The request method declares it throws IOException; shouldn't we propagate it instead of wrap?
There was a problem hiding this comment.
I found that LBHttpSolrClient needs a SolrServerException to detect a down node. Yet we still can throw an IOException here during prepareRequest. This roughly mimics the exception handling in Http2SolrClient.
There was a problem hiding this comment.
Okay; just needs a comment because it looks wrong
There was a problem hiding this comment.
Actually, maybe LBHttpSolrClient should be enhanced to catch the IOException to behave properly here? Otherwise, it seems wrong just looking at this SolrClient to throw/not-throw based on some wrapping/delegating client unless we spell out these things in the documented semantics of this method.
There was a problem hiding this comment.
Indeed, the doc-comment on the LBSolrClient request method says,
If a request fails due to an IOException, the server is moved to the dead pool
So I restored the previous behavior in HttpJdkSolrClient and made the modification in LBSolrClient. This new behavior is covered by LBHttp2SolrClientIntegrationTest.
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java
Outdated
Show resolved
Hide resolved
| private long aliveCheckIntervalMillis = | ||
| TimeUnit.MILLISECONDS.convert(60, TimeUnit.SECONDS); // 1 minute between checks | ||
| protected String defaultCollection; | ||
|
|
||
| public Builder(Http2SolrClient http2Client, Endpoint... endpoints) { | ||
| this.http2SolrClient = http2Client; | ||
| public Builder(C http2Client, LBSolrClient.Endpoint... endpoints) { |
There was a problem hiding this comment.
Can you add a static import for that Endpoint inner class so it looks simple as it was?
minor but can you please rename "http2Client" to incorporate the name "Solr" like just call it "solrClient" is fine. I like a SolrClient and HttpClient to not be confusable.
| try (Http2SolrClient http2SolrClient = | ||
| new Http2SolrClient.Builder(url).withTheseParamNamesInTheUrl(urlParamNames).build(); |
There was a problem hiding this comment.
perhaps some tests methods shouldn't care in theory what the httpSolrClient impl is exactly but it'd be nice to get one randomly or via parameterization even better. Not sure if that'd be too difficult.
There was a problem hiding this comment.
Actually, 4/5 test methods here are using a mock client. The withTheseParamNamesInTheUrl case is being tested with the clients' own unit tests. I did add a random choice to LBHttp2SolrClientIntegrationTest (renamed). I think this is pretty good coverage already.
| * Taken from: https://www.baeldung.com/java-httpclient-ssl sec 4.1, 2024/02/12. This is an | ||
| * all-trusting Trust Manager. Works with self-signed certificates. | ||
| */ | ||
| public class MockTrustManager extends X509ExtendedTrustManager { |
There was a problem hiding this comment.
nice to see this separated
There was a problem hiding this comment.
Thank you for taking the time to review. I'll plan to commit this (main only?; 9.8?) early next week.
Improve method signature hygene
…erverException - HttpJdkSolrClient to throw IOException (restore existing behavior)
| if (justFailed == null) justFailed = new HashMap<>(); | ||
| if (justFailed == null) { | ||
| justFailed = new HashMap<>(); | ||
| } |
There was a problem hiding this comment.
It's a shame to see duplication in getRootCause IOException detection with the dedicated clause. Maybe it'd be cleaner to have another try-catch just around calling request() above that detects the IOException and throws it? Just a thought; I leave it to you.
|
@gerlowskija @dsmiley This PR required significant changes to merge with the incoming changes from SOLR-17256. I would appreciate another round of review before merging. |
| }); | ||
| return future; | ||
| } catch (SolrServerException | IOException e) { | ||
| // Unreachable, since 'requestWithBaseUrl' above is running the request asynchronously |
There was a problem hiding this comment.
an "assert false" would be helpful.
I suppose a requestAsyncWithBaseUrl would be nice but not a big deal.
Similar to #2805, this is a minimal PR to add Load Balancing functionality for
HttpJdkSolrClient. Included is test refactoring, adding the *JdkClient as a random choice for the integration test.Whereas #2805 created a class hierarchy, this makes
LBHttp2SolrClientgeneric. Adding a generic parameter to this class will not break backwards-compatibility. This seems a better option that #2805.One downside is we retain the name
LBHttp2SolrClientalthough it is no longer strictly to be used withHttp2SolrClient. However, the legacy implementation already uses the preferred name:LBHttpSolrClient. Whenever the legacy implementation is removed, I recommend also renamingLBHttp2SolrClienttoLBHttpSolrClient.