You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "gerlowskija (via GitHub)" <gi...@apache.org> on 2024/01/05 02:21:29 UTC
[PR] SOLR-17066: Only apply default collection to collection requests [solr]
gerlowskija opened a new pull request, #2177:
URL: https://github.com/apache/solr/pull/2177
https://issues.apache.org/jira/browse/SOLR-17066
# Description
SOLR-17066 recently expanded the concept of a "default collection" to all SolrClient implementations, as a migration path for folks away from baking core or collection names into the baseURL provided to clients.
This (unreleased) change still has a drawback though: clients still have issues with making "admin" (i.e. core-agnostic) requests, since the SolrClient path-building logic always adds a collection to the path. It doesn't have any way to differentiate between core-aware (e.g. `/select`, `/update`, `/config`) and core-agnostic (e.g. `/admin/info/health`) requests.
# Solution
This PR fixes this by adding a boolean metadata method to SolrRequest, called `requiresDataStore()`, which allows SolrClients to only selectively use the defaultCollection for requests that are supposed to be use a core/collection.
# Tests
Manual tests continue to pass. Minor unit test in `SolrClientTest`.
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [x] I have added tests for my changes.
- [x] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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-17066: Only apply default collection to collection requests [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #2177:
URL: https://github.com/apache/solr/pull/2177#discussion_r1442460595
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -175,6 +175,19 @@ public void setQueryParams(Set<String> queryParams) {
public abstract SolrParams getParams();
+ /**
+ * Determines whether this request should use or ignore any specified data stores (esp. {@link
+ * SolrClient#defaultCollection})
+ *
+ * <p>Many Solr requests target a particular data store (i.e. core or collection). But not all of
+ * them - many Solr APIs (e.g. security or other admin APIs) are agnostic of data stores entirely.
+ * This method gives these requests a way to opt out of using {@link SolrClient#defaultCollection}
+ * or other specified data stores.
+ */
+ public boolean requiresDataStore() {
Review Comment:
[Q] I've used the term "Data Store" here as a generic way to refer to either a core or collection name.
There's some precedent for that term already (see discussion [here](https://github.com/apache/solr/pull/1978#issuecomment-1747451445)), but I figured I'd flag it again in case anyone missed that earlier discussion. The choice of terminology hasn't spread very far yet, and we could probably still change it if anyone has huge objections. But absent any serious concerns, I'm going to stick with it since there is precedent...
(Now that I think about it, maybe the variable names and builder methods should also use the "data store" term, e.g. `SolrClientBuilder.withDefaultDataStore(..)`...)
--
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-17066: Only apply default collection to collection requests [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #2177:
URL: https://github.com/apache/solr/pull/2177#issuecomment-1894206680
How much do you hate the idea of not having `storeType`?? Could we just have two methods, one where it's `/core/{name}/select` and the other is `/collection/{name}/select`? Could we side step the issue of the name by jsut having two paths?
Barring somehow being able to avoid the name, I think I like `IndexType` better. I went and looked at the Ref Guide hoping for inspiration, and https://solr.apache.org/guide/solr/latest/getting-started/introduction.html says _In addition to providing a network accessible engine for Lucene based document retrieval, Solr provides the ability to scale beyond the limitations of a single machine. Indexes can be sharded and replicated for performance and reliability, using either one of two [Solr Cluster Types](https://solr.apache.org/guide/solr/latest/deployment-guide/cluster-types.html). One type of cluster requires no supporting infrastructure, and instances are managed directly by administrators. The second type uses [Apache ZookeeperTM](https://zookeeper.apache.org/) to coordinate management activities across the cluster._. It interestingly doesn't say *collection* or *core* ;-). So that would be an argument for the `IndexType`
--
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-17066: Only apply default collection to collection requests [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2177:
URL: https://github.com/apache/solr/pull/2177#discussion_r1442509698
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -175,6 +175,19 @@ public void setQueryParams(Set<String> queryParams) {
public abstract SolrParams getParams();
+ /**
+ * Determines whether this request should use or ignore any specified data stores (esp. {@link
+ * SolrClient#defaultCollection})
+ *
+ * <p>Many Solr requests target a particular data store (i.e. core or collection). But not all of
+ * them - many Solr APIs (e.g. security or other admin APIs) are agnostic of data stores entirely.
+ * This method gives these requests a way to opt out of using {@link SolrClient#defaultCollection}
+ * or other specified data stores.
+ */
+ public boolean requiresDataStore() {
Review Comment:
Sigh; I would have voted for "Index", i.e. `requiresAnIndex`.
--
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-17066: Only apply default collection to collection requests [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #2177:
URL: https://github.com/apache/solr/pull/2177#issuecomment-1894221183
a logical Index can either be a single core or made up of many cores in the form of a collection....
--
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-17066: Only apply default collection to collection requests [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2177:
URL: https://github.com/apache/solr/pull/2177#discussion_r1442510069
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -175,6 +175,19 @@ public void setQueryParams(Set<String> queryParams) {
public abstract SolrParams getParams();
+ /**
+ * Determines whether this request should use or ignore any specified data stores (esp. {@link
+ * SolrClient#defaultCollection})
+ *
+ * <p>Many Solr requests target a particular data store (i.e. core or collection). But not all of
+ * them - many Solr APIs (e.g. security or other admin APIs) are agnostic of data stores entirely.
+ * This method gives these requests a way to opt out of using {@link SolrClient#defaultCollection}
+ * or other specified data stores.
+ */
+ public boolean requiresDataStore() {
Review Comment:
This is a search engine after all; we don't have to be overly generic with this "data store" word.
--
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-17066: Only apply default collection to collection requests [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija merged PR #2177:
URL: https://github.com/apache/solr/pull/2177
--
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-17066: Only apply default collection to collection requests [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2177:
URL: https://github.com/apache/solr/pull/2177#issuecomment-1894231175
+1 to IndexType
--
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-17066: Only apply default collection to collection requests [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #2177:
URL: https://github.com/apache/solr/pull/2177#issuecomment-1894215477
Another slightly less arcane example is the [DataStoreSolrRequest](https://github.com/apache/solr/pull/2177/files#diff-e13377598540e9ce1e314f17966d3f37cbe71433fe3ec6b91785751aa57df622) class added on this PR.
"CollectionSolrRequest" or "CollectionRequiringSolrRequest" isn't totally unserviceable here, but I worry that some reader without the context of our discussion here might take these names to mean that these request classes can only be used with proper SolrCloud collections.
Javadocs can solve this to some extent. But that feels a bit like putting a bucket under your sink instead of fixing the leak. If you know when you're writing a class that you'll need the Javadocs to clarify the potentially-misconstrue-able name, IMO you need a different name 😛
> How much do you hate the idea of not having storeType?
I'm not especially attached to "store". I just think there are some places where it'd be helpful to have a term that's not "collection". I don't really care beyond *what* that term is, as long as I have *something* to use in those cases so I don't feel blocked.
--
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-17066: Only apply default collection to collection requests [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #2177:
URL: https://github.com/apache/solr/pull/2177#issuecomment-1894220549
`IndexType`? And add it to https://solr.apache.org/guide/solr/latest/getting-started/solr-glossary.html#SolrGlossary-I ?
--
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-17066: Only apply default collection to collection requests [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #2177:
URL: https://github.com/apache/solr/pull/2177#issuecomment-1894171295
If you guys prefer "collection" here, that's fine with me. (@epugh - I agree that some doc changes would be helpful in clarifying that "collection" is sometimes used generically in the "logical index" or "collection or core" sense). This'll work in 99% of cases.
That said, there are some edge cases where (IMO) we can't unambiguously use "collection". Consider `QueryingApi.JsonQuery`, a generated SolrJ class for making queries using the v2 paths:
```
public static class JsonQuery extends SolrRequest<JsonQueryResponse> {
....
public JsonQuery(StoreType storeType, String storeName, Object requestBody) {
super(
SolrRequest.METHOD.valueOf("POST"),
"/{storeType}/{storeName}/select"
.replace("{" + "storeType" + "}", storeType.toString())
.replace("{" + "storeName" + "}", storeName));
}
```
Our v2 API uses different paths to distinguish between queries to individual cores (`/cores/<coreName>/select`) and to collections (`/collections/<collName>/select`). In order for `QueryingApi.JsonQuery` to be re-usable for both these cases, its ctor takes an enum (currently called "StoreType") to indicate what is being queried so that it can build the correct HTTP path.
As an example: what would you all prefer as the name for this enum? "CollectionType" wouldn't make any sense here IMO. If we think "store" is more overloaded than "index", then I can rename it from `StoreType` to `IndexType`?
--
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-17066: Only apply default collection to collection requests [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #2177:
URL: https://github.com/apache/solr/pull/2177#issuecomment-1883191148
I am having a bit of heart burn on the term "DataStore".... The more I see it proliferate the more it just seems like a bit of classic Enterprise Java type naming that doesn't mean anything...
I do see in our codebase `ExpressionType.DATASTORE` is used in streaming expressions that refers to "Solr/Lucene" as a source of data. It is also used to refer to a JDBC DataStore as well in our codebase. Both of those represent Data Stores in the traditional generic fashion. And don't imply a search index or other Solr specific concept.
As I write this, I come back to why can't we just stick with Collection? Reading the ref guide glossary again, the definition of `Collection` is `In Solr, one or more [Documents](https://solr.apache.org/guide/solr/latest/getting-started/solr-glossary.html#document) grouped together in a single logical index using a single configuration and Schema.`.
Yes, it also says `Single-node installations and user-managed clusters use instead the concept of a [Core](https://solr.apache.org/guide/solr/latest/getting-started/solr-glossary.html#core).`. Maybe we rework this definition to be
```
Single-node installations and user-managed clusters also have the concept of a [Core](https://solr.apache.org/guide/solr/latest/getting-started/solr-glossary.html#core) being synonymous with a Collection.
```
Let's move forward with the concept of Collection representing a logical index regardless of the physical 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-17066: Only apply default collection to collection requests [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2177:
URL: https://github.com/apache/solr/pull/2177#issuecomment-1887202830
+1 to "collection". Standalone Solr users, which are maybe getting more scarce, should figure this wording choice out, and we can update the ref guide glossary to be more helpful, as Eric suggests.
Or "index", which I think needn't imply a limitation to a leaf index/core. The noun/verb I think is a non-issue when in context, and can be ameliorated with wording like "anIndex".
--
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-17066: Only apply default collection to collection requests [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #2177:
URL: https://github.com/apache/solr/pull/2177#discussion_r1442905887
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -175,6 +175,19 @@ public void setQueryParams(Set<String> queryParams) {
public abstract SolrParams getParams();
+ /**
+ * Determines whether this request should use or ignore any specified data stores (esp. {@link
+ * SolrClient#defaultCollection})
+ *
+ * <p>Many Solr requests target a particular data store (i.e. core or collection). But not all of
+ * them - many Solr APIs (e.g. security or other admin APIs) are agnostic of data stores entirely.
+ * This method gives these requests a way to opt out of using {@link SolrClient#defaultCollection}
+ * or other specified data stores.
+ */
+ public boolean requiresDataStore() {
Review Comment:
Yeah, obviously "index" was my first instinct in the discussion on PR #1978, where this initially came up. So I'm not totally against the idea.
The argument against it at the time, which ultimately swayed me, was that because we're in a Lucene/IR context "index" is just too (over)loaded of a term. "index" means something very specific to folks coming from Lucene, and in Solr's own Lucene-adjacent code. I think a lot of people would be confused if we started using "index" to refer to N different Lucene indices spread across M machines.
Or as Eric put it:
> I think I like anything BUT "index" because I think that is an overloaded term that has specific meaning the closer to disk you get.. A Collection is NEVER a index.
----
That said - happy to go with the consensus here if others like "index"? All the reasoning in the world isn't much good if folks find "index" intuitive and workable in practice 🤷
--
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-17066: Only apply default collection to collection requests [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #2177:
URL: https://github.com/apache/solr/pull/2177#issuecomment-1894246273
OK, sounds good - I'll tag you both on a PR shortly that replaces datastore/store with a combination of "collection" and "indextype".
--
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