-
Notifications
You must be signed in to change notification settings - Fork 805
SOLR-18065: ConcurrentUpdateBaseSolrClient: support for HttpJdkSolrClient #4050
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
Conversation
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.
Pull request overview
This PR adds support for ConcurrentUpdateJdkSolrClient to enable concurrent update operations using the JDK HTTP client. The implementation follows the existing pattern established by ConcurrentUpdateJettySolrClient, but adapts it to work with the JDK client's asynchronous API.
Changes:
- Added
ConcurrentUpdateJdkSolrClientimplementation supporting concurrent updates with the JDK HTTP client - Refactored existing Jetty client tests into a shared
ConcurrentUpdateSolrClientTestBaseto support testing both Jetty and JDK client implementations - Modified
HttpJdkSolrClientto expose arequestInputStreamAsyncmethod for streaming responses - Updated
CustomBlockingQueueconstructor to remove unusedmaxConsumersparameter
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ConcurrentUpdateJdkSolrClient.java | New implementation of concurrent update client using JDK HTTP client |
| ConcurrentUpdateSolrClientTestBase.java | Extracted shared test logic to enable testing both Jetty and JDK client implementations |
| ConcurrentUpdateJdkSolrClientTest.java | Test implementation for JDK concurrent update client |
| ConcurrentUpdateJettySolrClientTest.java | Refactored to extend shared test base class |
| HttpJdkSolrClient.java | Added protected method to expose async input stream responses |
| ConcurrentUpdateBaseSolrClient.java | Removed unused maxConsumers parameter from CustomBlockingQueue |
| solr-tests.policy | Added URL permissions for JDK HTTP client test |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClientTest.java
Show resolved
Hide resolved
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClient.java
Show resolved
Hide resolved
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java
Outdated
Show resolved
Hide resolved
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClient.java
Show resolved
Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClientTestBase.java
Outdated
Show resolved
Hide resolved
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.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClient.java
Show resolved
Hide resolved
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClient.java
Show resolved
Hide resolved
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java
Show resolved
Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClientTestBase.java
Outdated
Show resolved
Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClientTestBase.java
Show resolved
Hide resolved
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClient.java
Outdated
Show resolved
Hide resolved
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClient.java
Outdated
Show resolved
Hide resolved
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClient.java
Show resolved
Hide resolved
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClient.java
Show resolved
Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClientTestBase.java
Show resolved
Hide resolved
dsmiley
left a comment
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.
+1 Looks good!
I checked the test refactorings in my IDE and saw things were retained as they were.
I think there's an adoc ref guide page to update and add this client to a list of other known clients.
janhoy
left a comment
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.
Thanks for this new client. It would be easier to review if all comments were resolved and precommit passing. But here are some initial comments.
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClientTestBase.java
Show resolved
Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClientTestBase.java
Show resolved
Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClientTestBase.java
Show resolved
Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClientTestBase.java
Show resolved
Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClientTestBase.java
Show resolved
Hide resolved
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClient.java
Outdated
Show resolved
Hide resolved
param order to be more consistent.
This adds
ConcurrentUpdateJdkSolrClient.javaand refactors the main unit tests to test both the Jetty and the JDK clients.Unlike with
LBAsyncSolrClientandCloudHttp2SolrClient, this new client requires anHttpJdkSolrClientand not any subclass ofHttpSolrClientBase. It did not seem possible to implement the abstract methodConcurrentUpdateBaseSolrClient#doSendUpdateStreamwithout requiring the concrete client.Unlike with
ConcurrentUpdateJettySolrClient, this implementation does not loop through the queue indoSendUpdateStream(the base class does this).I did not extend the tests
ConcurrentUpdateSolrClientBadInputTestorConcurrentUpdateSolrClientMultiCollectionTestto test this new client. These are testing the legacy client only.