Skip to content

fix: narrow catch clauses in fetchCacheMetrics/fetchHandlerMetrics - #111

Open
adityamparikh wants to merge 2 commits into
apache:mainfrom
adityamparikh:fix/narrow-metrics-catch-clauses
Open

fix: narrow catch clauses in fetchCacheMetrics/fetchHandlerMetrics#111
adityamparikh wants to merge 2 commits into
apache:mainfrom
adityamparikh:fix/narrow-metrics-catch-clauses

Conversation

@adityamparikh

@adityamparikh adityamparikh commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Narrow catch from RuntimeException to SolrException in fetchCacheMetrics() and fetchHandlerMetrics()
  • Still catches Solr 10 RemoteSolrException (subclass of SolrException) for graceful degradation when /admin/mbeans is unavailable
  • Programming bugs (NPE, ClassCastException, etc.) now propagate instead of being silently swallowed

Test plan

  • ./gradlew build passes
  • ./gradlew nativeTest -Pnative passes (119/119 tests)
  • No regressions

🤖 Generated with Claude Code

@adityamparikh
adityamparikh force-pushed the fix/narrow-metrics-catch-clauses branch from 02a0b4a to 521eb2f Compare May 2, 2026 17:04
@adityamparikh adityamparikh mentioned this pull request Jul 16, 2026
Merged
epugh pushed a commit that referenced this pull request Aug 18, 2026
@adityamparikh @claude
docs(collection): explain null cache/handler stats on Solr 10 (#163)
c011b0e 
On Solr 10 the /admin/mbeans endpoint was removed, so get-collection-stats
degrades gracefully and returns null cacheStats/handlerStats. Nothing on
the tool surface said so, leaving MCP clients to treat the nulls as a
failure and retry or misreport. State the behavior in the tool
description, which every client reads.

Kept to the description only to avoid colliding with PR #111, which
touches the fetchCacheMetrics/fetchHandlerMetrics implementations.

Signed-off-by: adityamparikh <aditya.m.parikh@gmail.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@adityamparikh @claude
fix: narrow catch clauses in fetchCacheMetrics/fetchHandlerMetrics
3df481e 
Change catch from RuntimeException to SolrException in
fetchCacheMetrics() and fetchHandlerMetrics(). This still catches
Solr 10's RemoteSolrException (subclass of SolrException) for graceful
degradation when /admin/mbeans is unavailable, but no longer swallows
programming bugs like NPE or ClassCastException.

Closes #4

Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: adityamparikh <aditya.m.parikh@gmail.com>
@adityamparikh
adityamparikh force-pushed the fix/narrow-metrics-catch-clauses branch from 521eb2f to 3df481e Compare August 19, 2026 11:58
@adityamparikh

adityamparikh commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

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.

@adityamparikh adityamparikh mentioned this pull request Aug 19, 2026
Open
adityamparikh added a commit to adityamparikh/solr-mcp that referenced this pull request Aug 20, 2026
@adityamparikh
refactor(collection): narrow the metrics catch clauses to SolrException
506f2bb 
Adopts the narrowing from apache#111 so the two PRs compose instead of colliding.

Both PRs rewrite the same two catch clauses in fetchCacheMetrics and
fetchHandlerMetrics. apache#111 narrows RuntimeException to SolrException; this PR
was binding the exception for logging while leaving RuntimeException in place.
Whichever merged second would either conflict or silently revert the other's
intent — so this branch now carries the narrowed form too, and the end state is
the same in either merge order.

RemoteSolrException extends SolrException (verified against solrj 10.0.0), so
the Solr 10 path where /admin/mbeans is gone still degrades to null rather than
propagating. What no longer gets swallowed is unrelated RuntimeExceptions --
which is the point of apache#111, and is what the new debug logging is there to
surface.

Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
@adityamparikh
docs: correct the Solr 10 metrics catch described in AGENTS.md
ab1aa24 
The Solr 10 Compatibility section said getCacheMetrics()/getHandlerMetrics()
"catch RuntimeException (which covers RemoteSolrException)". This PR is what
makes that untrue, so the correction belongs here rather than trailing behind
in a follow-up.

Also records why the narrower catch is the right one: RemoteSolrException
extends SolrException, so Solr 10's missing /admin/mbeans still degrades to
null, while an unrelated runtime failure in metrics parsing now surfaces as a
bug instead of masquerading as "metrics unavailable".

Note CLAUDE.md is a symlink to AGENTS.md, so this is the single source.

Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
@adityamparikh

adityamparikh commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Two updates.

Added the doc correction this PR invalidates (ab1aa24). The Solr 10 Compatibility section said
getCacheMetrics()/getHandlerMetrics() "catch RuntimeException (which covers
RemoteSolrException)" — untrue the moment this merges, so the fix belongs here rather than
trailing behind. It now also records why the narrower catch is correct, which the old wording
didn't. (CLAUDE.md is a symlink to AGENTS.md, so that's the single source.)

Resolved a collision with #110. Both PRs rewrite the same two catch clauses — this one narrows
RuntimeException to SolrException, #110 was binding the exception for logging while leaving
RuntimeException in place. Whichever merged second would have conflicted or silently reverted
the other. #110 now carries the narrowed form too (506f2bb), so the end state is identical in
either merge order and neither blocks the other.

For the record, the narrowing is safe: RemoteSolrException extends SolrException in solrj
10.0.0, verified against the jar rather than from memory, so Solr 10's missing /admin/mbeans
still degrades to null. What stops being swallowed is unrelated runtime failures — which is the
point.

@adityamparikh adityamparikh mentioned this pull request Aug 20, 2026
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@adityamparikh