You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "epugh (via GitHub)" <gi...@apache.org> on 2026/02/07 12:56:17 UTC
[PR] SOLR-18106: inline deprecated call [solr]
epugh opened a new pull request, #4114:
URL: https://github.com/apache/solr/pull/4114
https://issues.apache.org/jira/browse/SOLR-18106
# Description
Deprecated method
# Solution
Simple inline of code call.
# Tests
All pass
# Checklist
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: remove SolrQueryRequest.getCloudDescriptor() [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #4114:
URL: https://github.com/apache/solr/pull/4114#issuecomment-3894458734
For now, remove deprecation. Thanks for volunteering Eric!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: remove SolrQueryRequest.getCloudDescriptor() [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #4114:
URL: https://github.com/apache/solr/pull/4114#issuecomment-3902020570
Not doing this one.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: inline deprecated call [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley closed pull request #4114: SOLR-18106: inline deprecated call
URL: https://github.com/apache/solr/pull/4114
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: remove SolrQueryRequest.getCloudDescriptor() [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh closed pull request #4114: SOLR-18106: remove SolrQueryRequest.getCloudDescriptor()
URL: https://github.com/apache/solr/pull/4114
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: remove SolrQueryRequest.getCloudDescriptor() [solr]
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on PR #4114:
URL: https://github.com/apache/solr/pull/4114#issuecomment-3937762598
Im a bit surprised the test cases passes too. This is because of :
TL;DR - coll name from `CloudDescriptor` was not really used in the end 😓 . Due to `addCollectionParamIfNeeded(getCollectionsList())` in `HttpSolrCall` setting the `collection` param anyway
Details (generated from AI 🤖 )
```
When a request comes to the coordinator at /real_collection_name/select:
1. HttpSolrCall.init() (line 217): Parses origCorename = "real_collection_name" from the URL path
2. Line 220: Tries core = cores.getCore("real_collection_name") → returns null (coordinator doesn't have that core)
3. Line 242: Since core == null at this point:
String def = core != null ? core.getCoreDescriptor().getCollectionName() : origCorename;
// def = "real_collection_name" (the REAL name, not synthetic!)
4. Line 243-245: Sets collectionsList to ["real_collection_name"] (the real collection name)
5. Line 257: Calls getCoreByCollection("real_collection_name", ...) which is overridden by CoordinatorHttpSolrCall and returns the synthetic core
6. Line 299: Calls addCollectionParamIfNeeded(getCollectionsList()) which sets the collection request parameter to "real_collection_name" (line 803)
7. Later in HttpShardHandler.prepDistributed():
- Line 411: Gets cloudDescriptor from core descriptor → has synthetic collection name
- Line 442: Passes cloudDescriptor.getCollectionName() to builder → synthetic name
8. But in CloudReplicaSource.withClusterState() (line 73-89):
String collections = params.get("collection"); // Gets the "collection" parameter!
if (collections != null) {
// Uses the collection parameter (REAL name) - line 74-86
for (String collectionName : collectionList) {
addSlices(sliceMap, clusterState, params, collectionName, ...);
}
} else {
// Would use builder.collection (SYNTHETIC name) - line 89
// BUT THIS PATH IS NEVER TAKEN!
}
```
I have added a test case in [here](https://github.com/cowpaths/fullstory-solr/commit/a975288219d2bf025ae777f1a0bb91bd447797bc) to verify `req.getCore().getCoreDescriptor().getCloudDescriptor()` would give correct collection name. Such unit test case would fail right now 😓
I also looked at the idea of more "wrapping" as proposed in this [comment](https://github.com/apache/solr/pull/4114#issuecomment-3886297566). However, I backed off at the end as:
1. w/o major refactoring of existing core code (`SolrCore` etc), wrapping `SolrCore` will be very clumsy
2. Even I tried to go with the "clumsy" approach, im a bit worried about performance impact, since we are really creating whole bunch of "wrapper cores" which does run through the init routines in `SolrCore`.
Sorry I really couldn't contribute much to a solution at the end 😿 .
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: inline deprecated call [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4114:
URL: https://github.com/apache/solr/pull/4114#discussion_r2777755639
##########
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:
thanks, I followed up on this.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: inline deprecated call [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4114:
URL: https://github.com/apache/solr/pull/4114#discussion_r2777756428
##########
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:
I think I sorted this!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: remove SolrQueryRequest.getCloudDescriptor() [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #4114:
URL: https://github.com/apache/solr/pull/4114#issuecomment-3894392546
What would be the specific changes to move this one forward? I remain flummoxed that no tests fail with this change and yet we can't move it forward.
Since my goal is really to eliminate a deprecated method... Are we good with undeprecating it? I can do that.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: remove SolrQueryRequest.getCloudDescriptor() [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #4114:
URL: https://github.com/apache/solr/pull/4114#issuecomment-3886351131
> TL;DR: Yes. We probably need some additional change in order to have `CoordinatorHttpSolrCall` to work with this change
>
> Details: By debugging, it's shown this change right now would cause some issues in `HttpShardHandler#prepDistributed` as suspected.
>
> This is because `CoordinatorHttpSolrCall` only wraps the request by providing an overriding `getCloudDescriptor` with info of the actual core (with collection name, replica type and core shard ID replaced with the actual core info, instead of the synthetic core). Which means with this current change, getting `CloudDescriptor` from the `CoreDescroptor` would have returned the synthetic core with incorrect collection name (of the Synthetic core/collection)
>
> Perhaps in `CoordinatorHttpSolrCall`, we will need to instead override `getCore` to return a wrapped `SolrCore` and `CoreDescriptor` that its `getCloudDescriptor` would return the modified cloud description from the coordinator code?
>
> Not sure if I missed out any obvious details here 😓
Can we make sure to add a test that fails with this change? I worry as I do refactoring, that without David's sharp eyes, something like this would be merged based on the tests passing ;-).
@patsonluk I hope you have some time to get the changes made, feel free to push to this branch. I'm really pushing hard to remove deprecated code from Solr, and this is one of them!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: remove SolrQueryRequest.getCloudDescriptor() [solr]
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on PR #4114:
URL: https://github.com/apache/solr/pull/4114#issuecomment-3881730455
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 `CloudDescriptor` would still work properly for coordinator node setup( I believe the core in the req for coordinator node would not have the correct info, but i don't quite remember all the details 😓 )
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: inline deprecated call [solr]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: remove SolrQueryRequest.getCloudDescriptor() [solr]
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on PR #4114:
URL: https://github.com/apache/solr/pull/4114#issuecomment-3886297566
TL;DR:
Yes. We probably need some additional change in order to have `CoordinatorHttpSolrCall` to work with this change
Details:
By debugging, it's shown this change right now would cause some issues in `HttpShardHandler#prepDistributed` as suspected.
This is because `CoordinatorHttpSolrCall` only wraps the request by providing an overriding `getCloudDescriptor` with info of the actual core (with collection name, replica type and core shard ID replaced with the actual core info, instead of the synthetic core).
Perhaps, we will need to override `getCore` to returned a wrapped `SolrCore` and `CoreDescriptor` that its `getCloudDescriptor` would return the modified cloud description from the cooridnator code?
Not sure if I missed out any obvious details here 😓
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: remove SolrQueryRequest.getCloudDescriptor() [solr]
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on PR #4114:
URL: https://github.com/apache/solr/pull/4114#issuecomment-3936648473
Im going to take a quick look for test case even if this is closed. Sorry about the late reply!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: inline deprecated call [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #4114:
URL: https://github.com/apache/solr/pull/4114#issuecomment-3864608605
See JIRA issue
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: inline deprecated call [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #4114:
URL: https://github.com/apache/solr/pull/4114#issuecomment-3864794877
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...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: remove SolrQueryRequest.getCloudDescriptor() [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #4114:
URL: https://github.com/apache/solr/pull/4114#issuecomment-3886436001
+100 to the test.
And I confess (again) that this deprecation of mine may be premature... there's no clear substitute right now. Your idea of returning a getCore with a custom core descriptor is a plausible solution!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: remove SolrQueryRequest.getCloudDescriptor() [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #4114:
URL: https://github.com/apache/solr/pull/4114#issuecomment-3880620077
@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?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18106: remove SolrQueryRequest.getCloudDescriptor() [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #4114:
URL: https://github.com/apache/solr/pull/4114#issuecomment-3880632195
@patsonluk for your review please
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org