Re: [PR] Coordinator: remove SolrQueryRequest.getCloudDescriptor [solr]
patsonluk commented on PR #2913: URL: https://github.com/apache/solr/pull/2913#issuecomment-2830160309 @dsmiley Ah thanks for pointing that out, I have totally missed that! I totally hear your concern, the "special knowledge" makes it error prone for other dev (and i don't even remember that given that I did work with Coordinator logic on and off 😓 ). In theory we could override the `getCore` method in the anonymous `DelegatingSolrQueryRequest` within `wrappedReq` instead of `getCloudDescriptor`, but that would mean a new "wrapped" `SolrCore` with `getCloudDescriptor` overridden. However, that a wrapped SolrCore could mean a lot of boilerplate code (and some extra maintenance). Not sure which option is better 🤷🏼 We could improve the documentation as you suggested 😊 , it's not super intuitive tho (most dev probably don't even know what a synthetic/coordinator core is for) -- 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]
Re: [PR] Coordinator: remove SolrQueryRequest.getCloudDescriptor [solr]
dsmiley commented on PR #2913: URL: https://github.com/apache/solr/pull/2913#issuecomment-2827800625 The canShortCircuit method simply wouldn't return true for the CloudDescriptor of a coordinator core because, for one, the collection name is different. And the shard id is null for another reason. But I've become more sympathetic to this getCloudDescriptor's existence. If it were to be removed, how then would some plugin/component from the handler on down (say a query parser or many other things) even know what it's collection even is? The standard way won't work in coordinator mode. But how is a plugin writer (developer or LLM nowadays LOL) to know? What's missing is documentation on `CoreDescriptor#getCloudDescriptor` to warn when *not* to call it, and to recommend `SolrQueryRequest.getCloudDescriptor` instead. Sadly, when this method was added, only one spot was changed to use the request when there are other spots that should. I see other places that should change: RealTimeGetComponent, ShardAugmenterFactory, AtomicUpdateDocumentMerger, DistributedZkUpdateProcessor, ... and more. -- 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]
Re: [PR] Coordinator: remove SolrQueryRequest.getCloudDescriptor [solr]
patsonluk commented on PR #2913: URL: https://github.com/apache/solr/pull/2913#issuecomment-2827217553 > @patsonluk do you know about this? **If** there's a problem, it's HttpShardHandler (the only caller) who passes this cloudDescriptor on to `org.apache.solr.handler.component.HttpShardHandler#canShortCircuit` which we want to return false for any coordinator. Maybe it will do so _anyway_. If we want to guarantee it, we could modify this code to recognize the coordinator somehow. So sorry @dsmiley ! I did not see the notification until now 😞 . Unfortunately, I don't know much about this neither. However, based on the code flow the removal should be fine as the existing method body of `getCloudDescriptor#getCloudDescriptor` is essentially the same as your [change](https://github.com/apache/solr/pull/2913/files#diff-ce2580113ae9a9d50f308985229130f094868e83d35a8727c5683e6a2f3f44daR376) ? Removing redundant method is always a nice thing to do 😊 ! -- 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]
Re: [PR] Coordinator: remove SolrQueryRequest.getCloudDescriptor [solr]
github-actions[bot] commented on PR #2913: URL: https://github.com/apache/solr/pull/2913#issuecomment-2825806189 This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again. -- 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]
Re: [PR] Coordinator: remove SolrQueryRequest.getCloudDescriptor [solr]
github-actions[bot] closed pull request #2913: Coordinator: remove SolrQueryRequest.getCloudDescriptor URL: https://github.com/apache/solr/pull/2913 -- 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]
Re: [PR] Coordinator: remove SolrQueryRequest.getCloudDescriptor [solr]
github-actions[bot] commented on PR #2913: URL: https://github.com/apache/solr/pull/2913#issuecomment-2676457368 This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the [email protected] mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution! -- 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]
Re: [PR] Coordinator: remove SolrQueryRequest.getCloudDescriptor [solr]
patsonluk commented on PR #2913: URL: https://github.com/apache/solr/pull/2913#issuecomment-2561336393 > @patsonluk do you know about this? **If** there's a problem, it's HttpShardHandler (the only caller) who passes this cloudDescriptor on to `org.apache.solr.handler.component.HttpShardHandler#canShortCircuit` which we want to return false for any coordinator. Maybe it will do so _anyway_. If we want to guarantee it, we could modify this code to recognize the coordinator somehow. I am unfamiliar with the short circuit logic but based on debugging if we stop using `_` as shard id in `CloudDescriptor` (as in this PR), then `canShortCircuit` would return true. Not totally sure about the implication but `rb.shortCircuitedURL = ZkCoreNodeProps.getCoreUrl(zkController.getBaseUrl(), coreDescriptor.getName());` will be executed and a short circuited URL will be used? which seems incorrect since the coordinator node does not actually host the shard? Sorry I'm really helping much here 😓 , however to play safe, we might want to keep returning `false` for canShortCircuit calls done on Cooridnator node as you proposed (ie modify this code to recognize the coordinator somehow)? -- 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]
Re: [PR] Coordinator: remove SolrQueryRequest.getCloudDescriptor [solr]
dsmiley commented on code in PR #2913:
URL: https://github.com/apache/solr/pull/2913#discussion_r1896868578
##
solr/core/src/java/org/apache/solr/servlet/CoordinatorHttpSolrCall.java:
##
@@ -160,28 +156,13 @@ protected String getCoreOrColName() {
return collectionName;
}
- public static SolrQueryRequest wrappedReq(
- SolrQueryRequest delegate, String collectionName, HttpSolrCall
httpSolrCall) {
-Properties p = new Properties();
-if (collectionName != null) {
- p.put(CoreDescriptor.CORE_COLLECTION, collectionName);
-}
-p.put(CloudDescriptor.REPLICA_TYPE, Replica.Type.PULL.toString());
-p.put(CoreDescriptor.CORE_SHARD, "_");
+ public static SolrQueryRequest wrappedReq(SolrQueryRequest delegate,
HttpSolrCall httpSolrCall) {
Review Comment:
Yup, that's what's happening and the rationale.
It would be hard to prevent this as most handler and related things like
SearchComponents can be "SolrCoreAware" or otherwise manage to get access to
the SolrCore via SolrQueryRequest.getCore. That said, I think it'd be unusual
for a handler or SearchComponent to do something erroneously with this access
when in the coordinator mode.
--
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]
Re: [PR] Coordinator: remove SolrQueryRequest.getCloudDescriptor [solr]
pvcnt commented on code in PR #2913:
URL: https://github.com/apache/solr/pull/2913#discussion_r1896798141
##
solr/core/src/java/org/apache/solr/servlet/CoordinatorHttpSolrCall.java:
##
@@ -160,28 +156,13 @@ protected String getCoreOrColName() {
return collectionName;
}
- public static SolrQueryRequest wrappedReq(
- SolrQueryRequest delegate, String collectionName, HttpSolrCall
httpSolrCall) {
-Properties p = new Properties();
-if (collectionName != null) {
- p.put(CoreDescriptor.CORE_COLLECTION, collectionName);
-}
-p.put(CloudDescriptor.REPLICA_TYPE, Replica.Type.PULL.toString());
-p.put(CoreDescriptor.CORE_SHARD, "_");
+ public static SolrQueryRequest wrappedReq(SolrQueryRequest delegate,
HttpSolrCall httpSolrCall) {
Review Comment:
Oh, I think I eventually got it! With coordinator nodes, there is a single
synthetic core created per config set, hence service multiple collections. This
method is here so make it able to serve different collections by injecting an
ad-hoc cloud descriptor. However, this seems like a very dodgy pattern, as
calls going to `getCore().getCoreDescriptor().getCloudDescriptor()` would not
work.
Test `TestCoordinatorRole#testMultiCollectionMultiNode` is supposed to test
this scenario though, I do not understand, if my hypothesis is correct, how it
can still pass.
--
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]
Re: [PR] Coordinator: remove SolrQueryRequest.getCloudDescriptor [solr]
pvcnt commented on code in PR #2913:
URL: https://github.com/apache/solr/pull/2913#discussion_r1896798141
##
solr/core/src/java/org/apache/solr/servlet/CoordinatorHttpSolrCall.java:
##
@@ -160,28 +156,13 @@ protected String getCoreOrColName() {
return collectionName;
}
- public static SolrQueryRequest wrappedReq(
- SolrQueryRequest delegate, String collectionName, HttpSolrCall
httpSolrCall) {
-Properties p = new Properties();
-if (collectionName != null) {
- p.put(CoreDescriptor.CORE_COLLECTION, collectionName);
-}
-p.put(CloudDescriptor.REPLICA_TYPE, Replica.Type.PULL.toString());
-p.put(CoreDescriptor.CORE_SHARD, "_");
+ public static SolrQueryRequest wrappedReq(SolrQueryRequest delegate,
HttpSolrCall httpSolrCall) {
Review Comment:
Oh, I think I eventually got it! With coordinator nodes, there is a single
synthetic core created per config set, hence service multiple collections. This
method is here so make it able to service different collections by injecting an
ad-hoc cloud descriptor. However, this seems like a very dodgy pattern, as
calls going to `getCore().getCoreDescriptor().getCloudDescriptor()` would not
work.
##
solr/core/src/java/org/apache/solr/servlet/CoordinatorHttpSolrCall.java:
##
@@ -160,28 +156,13 @@ protected String getCoreOrColName() {
return collectionName;
}
- public static SolrQueryRequest wrappedReq(
- SolrQueryRequest delegate, String collectionName, HttpSolrCall
httpSolrCall) {
-Properties p = new Properties();
-if (collectionName != null) {
- p.put(CoreDescriptor.CORE_COLLECTION, collectionName);
-}
-p.put(CloudDescriptor.REPLICA_TYPE, Replica.Type.PULL.toString());
-p.put(CoreDescriptor.CORE_SHARD, "_");
+ public static SolrQueryRequest wrappedReq(SolrQueryRequest delegate,
HttpSolrCall httpSolrCall) {
Review Comment:
Oh, I think I eventually got it! With coordinator nodes, there is a single
synthetic core created per config set, hence service multiple collections. This
method is here so make it able to serve different collections by injecting an
ad-hoc cloud descriptor. However, this seems like a very dodgy pattern, as
calls going to `getCore().getCoreDescriptor().getCloudDescriptor()` would not
work.
--
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]
Re: [PR] Coordinator: remove SolrQueryRequest.getCloudDescriptor [solr]
pvcnt commented on code in PR #2913:
URL: https://github.com/apache/solr/pull/2913#discussion_r1890431876
##
solr/core/src/java/org/apache/solr/servlet/CoordinatorHttpSolrCall.java:
##
@@ -160,28 +156,13 @@ protected String getCoreOrColName() {
return collectionName;
}
- public static SolrQueryRequest wrappedReq(
- SolrQueryRequest delegate, String collectionName, HttpSolrCall
httpSolrCall) {
-Properties p = new Properties();
-if (collectionName != null) {
- p.put(CoreDescriptor.CORE_COLLECTION, collectionName);
-}
-p.put(CloudDescriptor.REPLICA_TYPE, Replica.Type.PULL.toString());
-p.put(CoreDescriptor.CORE_SHARD, "_");
+ public static SolrQueryRequest wrappedReq(SolrQueryRequest delegate,
HttpSolrCall httpSolrCall) {
Review Comment:
I believe this method can be removed altogether, can't 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: [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]
Re: [PR] Coordinator: remove SolrQueryRequest.getCloudDescriptor [solr]
dsmiley commented on PR #2913: URL: https://github.com/apache/solr/pull/2913#issuecomment-2551617582 @patsonluk do you know about this? **If** there's a problem, it's HttpShardHandler (the only caller) who passes this cloudDescriptor on to `org.apache.solr.handler.component.HttpShardHandler#canShortCircuit` which we want to return false for any coordinator. Maybe it will do so _anyway_. If we want to guarantee it, we could modify this code to recognize the coordinator somehow. -- 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]
