Repository navigation
Fix v2 API 404 when a /config request is proxied to another node - #5178
Conversation
Solr forwards a v2 request to the node that actually hosts the target
collection using the short "/c/{collection}/..." path alias (see
HttpSolrCall#sendRemoteProxy), but Constants.INDEX_PATH_PREFIX's JAX-RS
@path regex only recognized "cores" or "collections", not "c". Any
JAX-RS resource built on that prefix therefore 404'd with "Cannot find
API for the path" whenever a request needed to be proxied cross-node.
Reproduced with a new 2-node test in GetConfigSolrCloudTest that
deliberately sends a GET /config/overlay request to the node that does
NOT host the collection's only replica, then fixed by accepting "c" as
an alias for "collections" in the path regex.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RzzUgiebQ8ZhhgE2E7ZZU5
|
@jaykay12 this is the actual issue you were bumping into with package tool test!!! |
|
@gerlowskija would love your review. Also should we just drop the /c/ thing??? |
|
This was discovered while reviewing/debugging #5168 (SOLR-16396: migrate `GET /config/requestHandler` to JAX-RS) — its `PackageToolTest` was intermittently failing with `Cannot find API for the path: /config/requestHandlers` whenever the test's 2-node cluster routed the HTTP request to the node that didn't host the test collection's replica. Traced it down to this proxy-routing gap, confirmed it's general (not specific to that migration) by reproducing it against the already-merged `GET /config/overlay` endpoint here, and split the fix out into its own PR since it's independent of that migration. |
|
/no-changelog |
CI builds with -Werror, and errorprone flags comparing JettySolrRunner instances with != instead of .equals(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzzUgiebQ8ZhhgE2E7ZZU5
| // "c" is accepted alongside "collections" because Solr internally forwards a v2 request to the | ||
| // node actually hosting a collection using the short "/c/{collection}/..." alias (see | ||
| // HttpSolrCall#sendRemoteProxy and V2HttpCall's "c"/"collections" handling); without it, any | ||
| // JAX-RS resource built on this prefix 404s with "Cannot find API for the path" whenever the | ||
| // request has to be proxied to another node. |
There was a problem hiding this comment.
I wonder do we really need this justification. It doesn't look strange for 'c' to be here (to me); I'm not going to wonder what would break if one of these items was missing.
There was a problem hiding this comment.
I suppose silence means implied disagreement or lack of interest.
I find an increasing part of my code reviewing job in the age of AI coding development is to get Claude to shut up when it's saying too much :-)
There was a problem hiding this comment.
humm.... I think I missed your comment.... I will say, I 've though about going through various modules and seeing if we could put our comments on a diet... kind of like when we used to go through looking for stylistic warnnings from intellji!
There was a problem hiding this comment.
It's not a codebase-wide issue (yet), it's on many AI-driven PRs like this one.
The method name already says what the test does; per dsmiley's review comment, the javadoc was redundant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzzUgiebQ8ZhhgE2E7ZZU5
|
Looks good to me. Claude Opus 5.5 has this to say: Real but minor effects:
Nits:
Result: authorization, proxy dispatch and generated clients are unaffected, and the |
jaykay12
left a comment
jaykay12
left a comment
There was a problem hiding this comment.
@jaykay12 this is the actual issue you were bumping into with package tool test!!!
thanks @epugh for taking this up & fixing it.
I confirm that locally my tests are now passing on the branch with these changes on top. Will be able to confirm crave runs once workflows are triggered on that branch again.
lgtm functionality wise.
|
okay, so going to leave this open for another day. What I really want is someone to say |
|
going to merge to unblock #5168 |
4 participants
Summary
/c/{collection}/...path alias (seeHttpSolrCall#sendRemoteProxy), butConstants.INDEX_PATH_PREFIX's JAX-RS@Pathregex only recognizedcoresorcollections, notc. Any JAX-RS resource built on this prefix therefore 404'd withCannot find API for the pathwhenever a request had to be proxied cross-node.cas an alias forcollections.GET /config/requestHandlerto JAX-RS (SOLR-16396):PackageToolTestuses a 2-node cluster and intermittently hit this exact 404 whenever its HTTP client landed on the node that didn't host the test collection's replica. Confirmed the bug is general (not specific to that migration) by reproducing it against an already-merged, unrelated endpoint (GET /config/overlay).Test plan
GetConfigSolrCloudTest.testGetOverlayFromCollectionViaNonHostingNode, which bumps the test cluster to 2 nodes and deliberately sends a request to the node that does not host the collection's replica, forcing an internal proxy. Verified it fails withCannot find API for the path: /config/overlaybefore the fix, and passes after.GetConfigSolrCloudTestsuite passes.org.apache.solr.handler.admin.api.*,org.apache.solr.handler.admin.*,org.apache.solr.api.*,org.apache.solr.jersey.*(436 tests) pass.org.apache.solr.cloud.api.collections.*,org.apache.solr.handler.*V2*,org.apache.solr.servlet.*pass.spotlessApply— no formatting changes needed.🤖 Generated with Claude Code
https://claude.ai/code/session_01RzzUgiebQ8ZhhgE2E7ZZU5