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: [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]
