Copilot commented on code in PR #4058:
URL: https://github.com/apache/solr/pull/4058#discussion_r2699817059
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionHandlingUtils.java:
##########
@@ -247,7 +247,7 @@ private static UpdateResponse softCommit(
HttpJettySolrClient solrClient, String baseUrl, String coreName)
throws SolrServerException, IOException {
UpdateRequest ureq = new UpdateRequest();
- ureq.setAction(AbstractUpdateRequest.ACTION.COMMIT, false, true, true);
+ ureq.setAction(AbstractUpdateRequest.ACTION.COMMIT, true, true);
Review Comment:
The soft commit parameter is being lost in this refactoring. The method is
named `softCommit` and line 226 has a comment 'Calling soft commit to make sub
shard updates visible'. The old code was
`setAction(AbstractUpdateRequest.ACTION.COMMIT, false, true, true)` where
`softCommit=true`. The new 3-parameter version defaults `softCommit` to
`false`. Change to `ureq.setAction(AbstractUpdateRequest.ACTION.COMMIT, true,
true, true)` to maintain the soft commit behavior.
```suggestion
ureq.setAction(AbstractUpdateRequest.ACTION.COMMIT, true, true, true);
```
##########
solr/core/src/test/org/apache/solr/cloud/RecoveryAfterSoftCommitTest.java:
##########
@@ -91,7 +91,7 @@ public void test() throws Exception {
// soft-commit so searchers are open on un-committed but flushed segment
files
AbstractUpdateRequest request =
- new UpdateRequest().setAction(AbstractUpdateRequest.ACTION.COMMIT,
true, true, true);
+ new UpdateRequest().setAction(AbstractUpdateRequest.ACTION.COMMIT,
true, true);
Review Comment:
The soft commit parameter is being lost in this refactoring. The old code
called `setAction(AbstractUpdateRequest.ACTION.COMMIT, true, true, true)` where
the fourth parameter was `softCommit=true`. The new 3-parameter version calls
the overload that defaults `softCommit` to `false` (see AbstractUpdateRequest
line 40 which calls line 50). The comment on line 92-93 explicitly states this
should be a soft-commit. Change to `new
UpdateRequest().setAction(AbstractUpdateRequest.ACTION.COMMIT, true, true,
true)` to preserve the soft commit behavior.
```suggestion
new UpdateRequest().setAction(AbstractUpdateRequest.ACTION.COMMIT,
true, true, true);
```
##########
solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java:
##########
@@ -295,7 +295,7 @@ void addCommit(UpdateRequest ureq, CommitUpdateCommand cmd)
{
if (cmd == null) return;
ureq.setAction(
cmd.optimize ? AbstractUpdateRequest.ACTION.OPTIMIZE :
AbstractUpdateRequest.ACTION.COMMIT,
- false,
+ true, // waitFlush - ignored
Review Comment:
The comment 'waitFlush - ignored' is misleading since this parameter is no
longer named waitFlush in the underlying AbstractUpdateRequest.setAction()
method - it's still a required parameter at the AbstractUpdateRequest level. A
more accurate comment would be 'waitFlush (not used by server but required by
API)' or similar to clarify the parameter's status.
```suggestion
true, // waitFlush (not used by server but required by API)
```
--
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]
