Copilot commented on code in PR #4114:
URL: https://github.com/apache/solr/pull/4114#discussion_r2777524057
##########
solr/core/src/java/org/apache/solr/request/DelegatingSolrQueryRequest.java:
##########
@@ -159,8 +159,7 @@ public CoreContainer getCoreContainer() {
return delegate.getCoreContainer();
}
- @Override
public CloudDescriptor getCloudDescriptor() {
- return delegate.getCloudDescriptor();
+ return delegate.getCore().getCoreDescriptor().getCloudDescriptor();
}
Review 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.
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java:
##########
@@ -408,7 +408,7 @@ public void prepDistributed(ResponseBuilder rb) {
String shards = params.get(ShardParams.SHARDS);
CoreDescriptor coreDescriptor = req.getCore().getCoreDescriptor();
- CloudDescriptor cloudDescriptor = req.getCloudDescriptor();
+ CloudDescriptor cloudDescriptor =
req.getCore().getCoreDescriptor().getCloudDescriptor();
Review 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.
```suggestion
CloudDescriptor cloudDescriptor = coreDescriptor.getCloudDescriptor();
```
##########
solr/core/src/java/org/apache/solr/request/SolrQueryRequest.java:
##########
@@ -187,14 +186,6 @@ default CoreContainer getCoreContainer() {
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() {
Review 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.
##########
solr/core/src/java/org/apache/solr/request/SolrQueryRequest.java:
##########
@@ -187,14 +186,6 @@ default CoreContainer getCoreContainer() {
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() {
Review 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.
--
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]
