Skip to content

SOLR-18439: v2 APIs use 200 on success and 500 with a error.msg key - #4899

Open
epugh wants to merge 5 commits into
apache:mainfrom
epugh:SOLR-18439
Open

epugh wants to merge 5 commits into
apache:mainfrom
epugh:SOLR-18439

Conversation

@epugh

@epugh epugh commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

https://issues.apache.org/jira/browse/SOLR-18439

Description

We have a standard on V2 apis that 200 is success and 500 is an error, with a message under error.msg attribute. Most of the API's have that, but some don't so bring them into compliance.

Solution

Update three apis. They aren't used yet by any existing code, and haven't been released in Solr, so I think we don't need a changelog.

@epugh epugh added this to the 10.x milestone Sep 10, 2026
@github-actions github-actions Bot added the tests label Sep 10, 2026
@epugh

epugh commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Okay, running the CI process and then will merge if the tests all pass.

@janhoy

janhoy commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

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:

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 244a29b and 1079589).

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 (0cc72b8) 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants