SOLR-18106: remove SolrQueryRequest.getCloudDescriptor() #4114
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to eliminate usage of the deprecated SolrQueryRequest#getCloudDescriptor() method by inlining the underlying call chain.
Changes:
- Removes the deprecated
getCloudDescriptor()default method fromSolrQueryRequest. - Updates
DelegatingSolrQueryRequest#getCloudDescriptor()to inline the call chain instead of delegating. - Replaces
req.getCloudDescriptor()usage inHttpShardHandlerwith the inlined call chain.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
solr/core/src/java/org/apache/solr/request/SolrQueryRequest.java |
Deletes the deprecated getCloudDescriptor() method from the public request interface. |
solr/core/src/java/org/apache/solr/request/DelegatingSolrQueryRequest.java |
Stops overriding/delegating the removed interface method and inlines the call chain. |
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java |
Replaces deprecated req.getCloudDescriptor() call with an inlined req.getCore().getCoreDescriptor().getCloudDescriptor() chain. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| default CoreContainer getCoreContainer() { | ||
| SolrCore core = getCore(); | ||
| return core == null ? null : core.getCoreContainer(); | ||
| } | ||
|
|
||
| /** | ||
| * @deprecated use getCore().getCoreDescriptor().getCloudDescriptor() | ||
| */ | ||
| @Deprecated | ||
| default CloudDescriptor getCloudDescriptor() { | ||
| return getCore().getCoreDescriptor().getCloudDescriptor(); | ||
| } | ||
|
|
||
| /** The writer to use for this request, considering {@link CommonParams#WT}. Never null. */ | ||
| default QueryResponseWriter getResponseWriter() { |
There was a problem hiding this comment.
Removing SolrQueryRequest#getCloudDescriptor() is a source/binary incompatible API change on a public interface and also breaks current in-repo implementations that still override it (e.g., CoordinatorHttpSolrCall has an @OverRide getCloudDescriptor method). If the goal is only to eliminate deprecated call sites, keep the deprecated default method and update callers; if the goal is full removal, all remaining overrides/callers (including CoordinatorHttpSolrCall) must be updated in this PR.
There was a problem hiding this comment.
thanks, I followed up on this.
|
|
||
| @Override | ||
| public CloudDescriptor getCloudDescriptor() { | ||
| return delegate.getCloudDescriptor(); | ||
| return delegate.getCore().getCoreDescriptor().getCloudDescriptor(); | ||
| } |
There was a problem hiding this comment.
DelegatingSolrQueryRequest still declares getCloudDescriptor(), but SolrQueryRequest no longer defines this method. This leaves a surprising extra public API on a class described as delegating the SolrQueryRequest interface. Either remove this method (and its CloudDescriptor import) or formally keep it as part of SolrQueryRequest (deprecated) so the delegation contract remains consistent.
There was a problem hiding this comment.
I think I sorted this!
|
|
||
| CoreDescriptor coreDescriptor = req.getCore().getCoreDescriptor(); | ||
| CloudDescriptor cloudDescriptor = req.getCloudDescriptor(); | ||
| CloudDescriptor cloudDescriptor = req.getCore().getCoreDescriptor().getCloudDescriptor(); |
There was a problem hiding this comment.
This line duplicates the already-computed coreDescriptor and repeats req.getCore().getCoreDescriptor(). Prefer using the existing local variable (coreDescriptor.getCloudDescriptor()) to avoid redundant calls and keep the code consistent with the surrounding style.
| CloudDescriptor cloudDescriptor = req.getCore().getCoreDescriptor().getCloudDescriptor(); | |
| CloudDescriptor cloudDescriptor = coreDescriptor.getCloudDescriptor(); |
| default CoreContainer getCoreContainer() { | ||
| SolrCore core = getCore(); | ||
| return core == null ? null : core.getCoreContainer(); | ||
| } | ||
|
|
||
| /** | ||
| * @deprecated use getCore().getCoreDescriptor().getCloudDescriptor() | ||
| */ | ||
| @Deprecated | ||
| default CloudDescriptor getCloudDescriptor() { | ||
| return getCore().getCoreDescriptor().getCloudDescriptor(); | ||
| } | ||
|
|
||
| /** The writer to use for this request, considering {@link CommonParams#WT}. Never null. */ | ||
| default QueryResponseWriter getResponseWriter() { |
There was a problem hiding this comment.
PR description says this is a "simple inline" of a deprecated call, but this diff also removes the deprecated method from SolrQueryRequest entirely. If method removal is intended, the PR description (and any deprecation/removal policy docs/release notes) should reflect that broader API change.
|
See JIRA issue |
|
I'm going to reopen this so you can see the furthur commits, and then if we decide not to move forward then I'll remove the deprecation tag and close it... |
dsmiley
changed the title
|
@dsmiley thanks for the better title! Is there some sort of investigation that needs to be done before this can be merged? We can leave it open obviously for a few days, but is there anyone who we specifically need to chase to review this? |
|
@patsonluk for your review please |
|
Thanks @dsmiley ! I will take a look tomorrow. I see the concern you all discussed in https://issues.apache.org/jira/browse/SOLR-18106?focusedCommentId=18057106&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-18057106 Need to revisit to code and see whether the absence of modified |
3 participants
https://issues.apache.org/jira/browse/SOLR-18106
Description
Deprecated method
Solution
Simple inline of code call.
Tests
All pass
Checklist