You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/07/22 18:51:01 UTC
[GitHub] [lucene-solr] andyvuong opened a new pull request #1687: SOLR-14658: Fix colstatus solrj interfaces to return one or all collections
andyvuong opened a new pull request #1687:
URL: https://github.com/apache/lucene-solr/pull/1687
# Description
This fixes a bug in the solrj interface for the [COLSTATUS collection api](https://lucene.apache.org/solr/guide/8_6/collection-management.html#colstatus) as detailed in the [JIRA](https://issues.apache.org/jira/browse/SOLR-14658) such that using the class CollectionAdminRequest.collectionStatus(collectionName) to issue colstatus commands would return all collections vs just the one specified. This is an issue as the operation sends a request to all collections' shard leaders.
# Solution
CollectionAdminRequest.collectionStatus(collectionName) will now only return one collection. To support getting the status for all collections, I've added CollectionAdminRequest.collectionStatuses(). The documented API still works exactly as it is detailed.
# Tests
I've added a new test case to test these new interfaces. I've also provided a snippet of code and steps for manual validation in the JIRA (to repro the bug) but i can be used to test the fix as well:
```
String host = "http://localhost:8983/solr";
HttpSolrClient.Builder builder = new HttpSolrClient.Builder(host);
HttpSolrClient solrClient = builder.build();
String collection = "tes";
final NamedList<Object> response =
//solrClient.request(CollectionAdminRequest.collectionStatus(collection));
solrClient.request(CollectionAdminRequest.collectionStatuses());
response._forEachEntry((k,v) -> {
System.out.println(k);
});
System.out.println(response);
```
Using just the API and not solrj for verification:
- http://localhost:8983/solr/admin/collections?action=COLSTATUS&collection=tester1
- return one collection
- http://localhost:8983/solr/admin/collections?action=COLSTATUS
- return all collections
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) 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 `master` branch.
- [x] I have run `ant precommit` and the appropriate test suite.
- [x] I have added tests for my changes.
- [ ] I have added documentation for the [Ref Guide](https://github.com/apache/lucene-solr/tree/master/solr/solr-ref-guide) (for Solr changes only).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley merged pull request #1687: SOLR-14658: Fix colstatus solrj interfaces to return one or all collections
Posted by GitBox <gi...@apache.org>.
dsmiley merged pull request #1687:
URL: https://github.com/apache/lucene-solr/pull/1687
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on pull request #1687: SOLR-14658: Fix colstatus solrj interfaces to return one or all collections
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #1687:
URL: https://github.com/apache/lucene-solr/pull/1687#issuecomment-685826394
In addition to the minor expectThrows addition, @andyvuong can you also please add a CHANGES.txt entry for 8.7 in the bug section? Perhaps something like:
````
* SOLR-14658: SolrJ's CollectionAdminRequest.collectionStatus(collection) would internally get all collection statuses instead of just for the specified collection. (Andy Vuong)
````
Maybe this even goes under the "Optimization" section? From a user's standpoint, that's what this is. It wasn't broken before.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1687: SOLR-14658: Fix colstatus solrj interfaces to return one or all collections
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1687:
URL: https://github.com/apache/lucene-solr/pull/1687#discussion_r474312456
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java
##########
@@ -517,10 +517,7 @@ private static void addStatusToResponse(NamedList<Object> results, RequestStatus
ColStatus.RAW_SIZE_DETAILS_PROP,
ColStatus.RAW_SIZE_SAMPLING_PERCENT_PROP,
ColStatus.SIZE_INFO_PROP);
- // make sure we can get the name if there's "name" but not "collection"
- if (props.containsKey(CoreAdminParams.NAME) && !props.containsKey(COLLECTION_PROP)) {
Review comment:
CC @sigram I believe you added the lines of code here that Andy thinks should be removed.
##########
File path: solr/core/src/test/org/apache/solr/cloud/CollectionsAPISolrJTest.java
##########
@@ -662,6 +662,44 @@ public void testColStatus() throws Exception {
Number down = (Number) rsp.getResponse().findRecursive(collectionName, "shards", "shard1", "replicas", "down");
assertTrue("should be some down replicas, but there were none in shard1:" + rsp, down.intValue() > 0);
}
+
+ @Test
+ public void testColStatusCollectionName() throws Exception {
+ final String[] collectionNames = {"collectionStatusTest_1", "collectionStatusTest_2"};
+ for (String collectionName : collectionNames) {
+ CollectionAdminRequest.createCollection(collectionName, "conf2", 1, 1)
+ .process(cluster.getSolrClient());
+ cluster.waitForActiveCollection(collectionName, 1, 1);
+ }
+ // assert only one collection is returned using the solrj colstatus interface
+ CollectionAdminRequest.ColStatus req = CollectionAdminRequest.collectionStatus(collectionNames[0]);
+ CollectionAdminResponse rsp = req.process(cluster.getSolrClient());
+ assertNotNull(rsp.getResponse().get(collectionNames[0]));
+ assertNull(rsp.getResponse().get(collectionNames[1]));
+
+ req = CollectionAdminRequest.collectionStatus(collectionNames[1]);
+ rsp = req.process(cluster.getSolrClient());
+ assertNotNull(rsp.getResponse().get(collectionNames[1]));
+ assertNull(rsp.getResponse().get(collectionNames[0]));
+
+ // assert passing null collection fails
+ try {
Review comment:
please convert to expectThrows
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] andyvuong commented on a change in pull request #1687: SOLR-14658: Fix colstatus solrj interfaces to return one or all collections
Posted by GitBox <gi...@apache.org>.
andyvuong commented on a change in pull request #1687:
URL: https://github.com/apache/lucene-solr/pull/1687#discussion_r459012987
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java
##########
@@ -863,13 +863,23 @@ public SolrParams getParams() {
}
/**
- * Return a SolrRequest for low-level detailed status of the collection.
+ * Return a SolrRequest for low-level detailed status of the specified collection.
+ * @param collection the collection to get the status of.
*/
public static ColStatus collectionStatus(String collection) {
+ checkNotNull(CoreAdminParams.COLLECTION, collection);
return new ColStatus(collection);
}
+
+ /**
+ * Return a SolrRequest for low-level detailed status of all collections on the cluster.
+ */
+ public static ColStatus collectionStatuses() {
+ return new ColStatus();
+ }
- public static class ColStatus extends AsyncCollectionSpecificAdminRequest {
Review comment:
AsyncCollectionSpecificAdminRequest enforces a non-null collection name passed in which doesn't work if the solrj colstatus interface's intention is to allow either one collection return or all collections returned as documented in the collection api doc. If no collection = null, i.e. empty, then collectionshandler will treat it as returning all collections staying consistent with the documented api.
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java
##########
@@ -517,10 +517,7 @@ private static void addStatusToResponse(NamedList<Object> results, RequestStatus
ColStatus.RAW_SIZE_DETAILS_PROP,
ColStatus.RAW_SIZE_SAMPLING_PERCENT_PROP,
ColStatus.SIZE_INFO_PROP);
- // make sure we can get the name if there's "name" but not "collection"
- if (props.containsKey(CoreAdminParams.NAME) && !props.containsKey(COLLECTION_PROP)) {
Review comment:
Removed this because it's not documented in the API anyway and the CoreAdminParams.NAME isn't copied anyway.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org