adityamparikh commented on PR #111:
URL: https://github.com/apache/solr-mcp/pull/111#issuecomment-5341802786
Rebased onto `main` (`a84033b`), with one fix to the PR's own content.
**Fixed:** the diff changed `catch (... | RuntimeException _)` to `catch
(... | SolrException e)` — naming a variable that is never used. `main`
deliberately uses the unnamed `_` there. Restored to `catch
(SolrServerException | IOException | SolrException _)` so the narrowing lands
without reintroducing an unused binding.
**One thing worth a reviewer's judgement.** `RemoteSolrException extends
SolrException extends RuntimeException`, so the 404 from a removed endpoint is
still caught and still degrades to `null` — that part is safe. But
`fetchMetrics` also does an unchecked `(NamedList<Object>)
response.get("metrics")`. Under the old broad `RuntimeException` catch, a
`ClassCastException` from an unexpected `/admin/metrics` response shape would
degrade to `null`; after this narrowing it propagates out of
`getCollectionStats`.
I think that is the right trade — swallowing `ClassCastException` hides a
genuine bug, and surfacing it is the stated point of the PR — but it is a real
behaviour change on any Solr whose metrics payload differs from 9.9, which is
what our default `solr.test.image` exercises. Happy to add `ClassCastException`
back to the catch list if the project would rather keep unconditional graceful
degradation there.
Verified: `./gradlew build` green on JDK 25.
--
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]
