New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SOLR-12217: Support shards.preference for individual shard requests #984
SOLR-12217: Support shards.preference for individual shard requests #984
Conversation
f938245
to
0744230
Compare
0744230
to
7a49167
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I don't know much about the streaming part, maybe @joel-bernstein can take a look, but in general, LGTM other than the comments.
@@ -651,6 +659,13 @@ protected RouteException getRouteException(SolrException.ErrorCode serverError, | |||
} | |||
} | |||
} | |||
|
|||
// Sort the non-leader replicas according to the request parameters | |||
replicaListTransformer.transform(urls); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary for updates? are you trying to keep consistency only or do you have some use case in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly for consistency. I guess it's mostly a no-op, since the leader is always going to be first anyways (if a leader exists).
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseCloudSolrClient.java
Outdated
Show resolved
Hide resolved
solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/SignificantTermsStream.java
Outdated
Show resolved
Hide resolved
718e247
to
9aef437
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'm running tests and there are some failures, most probably unrelated but want to make sure
Want to hear your opinion @tflobbe . So I changed the logic for non-update/admin/v2 request routing to use core URLs instead of collection URLs, unless the request is querying multiple collections. In this way, if there are certain replicas on a host that you want to target you can make that distinction. However that does change the default behavior in a non-trivial way. The only place where I see this breaks any test is weirdly enough in the Hadoop auth tests for sending a commit request. It seems that since the commit request is sent directly to a core, instead of the collection url, it is missing an additional count of unauthenticated requests. |
I've decided to be conservative and revert that logic. So for non-update requests, SolrJ will send the request to the collection URL, not the replica URL. All tests pass on my end now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM @HoustonPutman. You don't need me for merging now :)
…equests (apache#984) (cherry picked from commit 53345cb)
Description
Use shard preference rules when choosing replicas to send individual shard requests. This includes requests from SolrJ as well as requests sent via streaming expressions.
Solution
How routing preferences are determined:
For Streaming Expressions, it will use the preferences from the following sources, in order of preference (defaults last):
DEFAULT_SHARD_PREFERENCES
set in cluster propertiesFor SolrJ, it will use the preferences from the params of the request it is sending.
Tests
Current tests pass.
TODO Add tests for routing functionality
Checklist
Please review the following and check all that apply:
master
branch.ant precommit
and the appropriate test suite.