-
Notifications
You must be signed in to change notification settings - Fork 805
SOLR-7003: Remove unused boolean waitFlush from SolrClients (VERSION 2) #4064
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
base: main
Are you sure you want to change the base?
Conversation
First pass to use a CommitOptions instead of a whole bunch of separate methods.
…ur of taking the commitOptions
|
Replaces #4058 |
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 removes the unused waitFlush parameter from Solr client commit and optimize methods by introducing a new CommitOptions class that provides a type-safe way to specify commit parameters. This replaces the previous approach of using multiple boolean parameters in method signatures.
Changes:
- Introduced
CommitOptionsclass to encapsulate commit/optimize settings (waitSearcher, openSearcher, softCommit, expungeDeletes, maxOptimizeSegments) - Deprecated existing commit/optimize methods that use multiple boolean parameters in favor of methods accepting
CommitOptions - Updated test code throughout the codebase to use the simplified API with sensible defaults
- Removed
waitFlushparameter fromCommitStreamand updated documentation accordingly
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
solr/solrj/src/java/org/apache/solr/client/solrj/request/CommitOptions.java |
New class providing type-safe commit/optimize options with factory methods and validation |
solr/solrj/src/test/org/apache/solr/client/solrj/request/CommitOptionsTest.java |
Comprehensive test coverage for CommitOptions including validation, factory methods, and integration |
solr/solrj/src/java/org/apache/solr/client/solrj/request/AbstractUpdateRequest.java |
Added new setAction method accepting CommitOptions; deprecated old methods with waitFlush |
solr/solrj/src/java/org/apache/solr/client/solrj/SolrClient.java |
Added new commit/optimize methods with CommitOptions; deprecated old methods; contains bug |
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/CommitStream.java |
Removed waitFlush parameter from constructor and field |
solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java |
Updated to use CommitOptions for building commit requests |
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java |
Updated commit calls to use CommitOptions; removed unused UpdateParams import |
| Various test files | Simplified commit/optimize calls to use defaults where appropriate |
solr/solr-ref-guide/modules/query-guide/pages/stream-decorator-reference.adoc |
Removed waitFlush from commit stream documentation |
changelog/unreleased/SOLR-7003.yml |
Added changelog entry for this refactoring |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
solr/solrj/src/java/org/apache/solr/client/solrj/SolrClient.java
Outdated
Show resolved
Hide resolved
solr/solrj/src/java/org/apache/solr/client/solrj/SolrClient.java
Outdated
Show resolved
Hide resolved
solr/solrj/src/java/org/apache/solr/client/solrj/request/CommitOptions.java
Show resolved
Hide resolved
|
This is ready for review @dsmiley. Thanks for your comments on the previous attempt, it helped me approach this much better. |
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.
This subject and approach strongly reminds me of ideas @bruno-roustant and I were once thinking about. Do you remember Bruno? Care to review?
| * | ||
| * @since Solr 10.0 | ||
| */ | ||
| public class CommitOptions { |
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.
why is this not a record?
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.
Mostly cause I don't really know what record are.... Do we use them in our code base? All I know about records is that IntelliJ periodically prompts me to change to one of them!
https://issues.apache.org/jira/browse/SOLR-7003
Description
Second take at removing the
waitFlushparameter.Solution
In this version, I introduced a
CommitOptionsobject that wraps up all the various settings. In my previous attempt, dealing iwth all the various booleans and ints and strings in the methods inSolrClientwas killing me....This lets us deprecate a bunch of methods in
SolrClientoptimizeandcommitmethods.I also found a whole bunch of places in our tests where we pass extra params into the
optimizeandcommitmethods that could be simplified to leverage existing defaults.Tests
Mostly reruning tests, and add a few small ones.