janhoy commented on PR #4899:
URL: https://github.com/apache/solr/pull/4899#issuecomment-5822703480
I asked Claude to review your PR. Pasting in the review text below
----
I like the direction here — 200-with-`status: ERROR` is a bad shape and we
should get rid of it. A few things to sort out first though.
### `TestReplicationHandler.testFileListShouldReportErrorsWhenTheyOccur`
fails
It asserts exactly the behaviour being removed:
```java
assertEquals("ERROR", resp.get("status"));
assertEquals("invalid index generation", resp.get("message"));
```
Running it on this branch:
```
2> 2246 INFO (qtp441963249-48-null-1) [ x:collection1 t:null-1]
o.a.s.c.S.Request
path=/replication params={generation=-2&wt=javabin&command=filelist}
status=404 QTime=12
org.apache.solr.client.solrj.RemoteSolrException: Error from server at
http://127.0.0.1:.../solr/collection1/replication?wt=javabin&command=filelist&generation=-2:
org.apache.solr.common.SolrException: invalid index generation
at
org.apache.solr.handler.TestReplicationHandler.testFileListShouldReportErrorsWhenTheyOccur(TestReplicationHandler.java:1493)
```
CI is green because `TestReplicationHandler` is `@Nightly`, so it never ran
here:
```
./gradlew :solr:core:test -Ptests.nightly=true \
--tests
"org.apache.solr.handler.TestReplicationHandler.testFileListShouldReportErrorsWhenTheyOccur"
```
The good news: `testFollowerRestartsWhenCommitExpiresBeforeFileDownload`
(SOLR-18406) still passes.
### This isn't only a v2 change, and these APIs have shipped
The description says these "aren't used yet by any existing code, and
haven't been released in Solr". That holds for `SnapshotBackupAPI`, but not for
the filelist path:
- `ReplicationHandler:296-300` routes v1 `command=filelist` straight into
`CoreReplication.fetchFileList(...)` and squashes the result into the v1
response, so the v1 command's wire format changes too — the run above shows
`/replication?command=filelist` now answering **404**.
- `IndexFetcher.fetchFileList` (`IndexFetcher:369`) calls that v1 command on
every leader/follower replication cycle.
- Both v2 endpoints shipped in 10.0.0 (`git tag --contains` on 244a29b39e3
and 10795898433).
The follower-side effect: today an expired generation comes back 200 with no
`filelist` key, `fetchFileList` sets `filesToDownload = List.of()`, and
`fetchLatestIndex` returns the specific
`IndexFetchResult.PEER_INDEX_COMMIT_DELETED` (`IndexFetcher:572-575`). With a
404, the `RemoteSolrException` escapes `fetchFileList` (it's a
`RuntimeException`, so the `catch (SolrServerException)` doesn't catch it),
gets rethrown by `catch (SolrException e) { throw e; }` at `IndexFetcher:779`,
and lands in `ReplicationHandler.doFetch`'s `catch (Exception)` as a generic
`FAILED_BY_EXCEPTION`. Not fatal, but we lose a distinct diagnostic outcome and
start logging an expected condition at error level.
Worth a changelog `type: changed` rather than `fixed`, I think, and possibly
an upgrade note.
### 404 vs 409
SOLR-18406 (0cc72b8e326) already models this exact condition — "the
generation you asked for is gone" — as **409 CONFLICT**, in
`DirectoryFileStream.initWrite()`, and `IndexFetcher` maps a 409 to
`InvalidIndexGenerationException` and restarts replication
(`IndexFetcher:1832`, `:1869`). Using 404 for the same event in the filelist
half means the two halves of the same replication conversation report it
differently. Suggest `ErrorCode.CONFLICT` with the generation in the message,
matching `"invalid index generation: " + indexGen` — and then teaching
`fetchFileList` to route it into the same restart path instead of a generic
failure.
### Smaller stuff (optional)
- `FileListResponse.message` / `.exception` and
`ReplicationBackupResponse.message` / `.exception` have no writer left after
this change. `public Exception exception` in a JSON response model isn't a
great shape anyway — worth either removing them here or saying why they stay.
- `getFileList` only sets `status = OK_STATUS` inside the conf-files branch
(`ReplicationAPIBase:229`); the common SolrCloud / no-conf-files path returns
early at `:219-220` with `status == null`. If `ERROR` is going away, `OK`
probably should too.
- v1 `command=backup` keeps its own `reportErrorOnResponse` in
`ReplicationHandler:649-673`, so v1 backup still answers 200 + ERROR while v2
now answers 500. Fine to leave out of scope, but worth a note on the JIRA.
- `"Error encountered while creating a snapshot: " + e.getMessage()` with
`e` also passed as the cause duplicates the message.
- Branch is ~45 commits behind main.
--
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]
