dsmiley commented on code in PR #2571:
URL: https://github.com/apache/solr/pull/2571#discussion_r1685236741
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java:
##########
@@ -156,6 +156,15 @@ public List<String> resolveAlias(String collection) {
public <T> T getClusterProperty(String propertyName, T def) {
return def;
}
+
+ @Override
+ public Set<String> resolveAliases(List<String> inputCollections) {
+ Set<String> res = new HashSet<>();
+ for (String c : inputCollections) {
+ res.add(c);
+ }
+ return res;
Review Comment:
There's a very short idiomatic way to do all this in one line :-)
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##########
@@ -246,6 +249,45 @@ public void testAliasHandling() throws Exception {
2, client.query(null,
paramsWithMixedCollectionAndAlias).getResults().getNumFound());
}
+ @Test
+
@LogLevel("org.apache.solr.client.solrj.impl.BaseHttpClusterStateProvider=DEBUG")
+ public void testHttpCSPPerf() throws Exception {
Review Comment:
testHttpCspPerf is the capitalization approach to prefer
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -789,7 +788,8 @@ protected NamedList<Object> requestWithRetryOnStaleState(
if (!inputCollections.isEmpty()
&& !isAdmin
&& !isCollectionRequestOfV2) { // don't do _stateVer_ checking for
admin, v2 api requests
- Set<String> requestedCollectionNames = resolveAliases(inputCollections);
+ Set<String> requestedCollectionNames =
+ getClusterStateProvider().resolveAliases(inputCollections);
Review Comment:
I like that you are moving more alias resolution logic to the CSP, but
nonetheless CloudSolrClient is still explicitly involved. I would prefer if
the matter was invisible to CloudSolrClient (make no reference here to aliases;
let it be a detail in CSP). Admittedly that might be too much for this
issue/PR.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##########
@@ -246,6 +249,45 @@ public void testAliasHandling() throws Exception {
2, client.query(null,
paramsWithMixedCollectionAndAlias).getResults().getNumFound());
}
+ @Test
+
@LogLevel("org.apache.solr.client.solrj.impl.BaseHttpClusterStateProvider=DEBUG")
+ public void testHttpCSPPerf() throws Exception {
+
+ String COLLECTION = "TEST_COLLECTION";
+ CollectionAdminRequest.createCollection(COLLECTION, "conf", 2, 1)
+ .process(cluster.getSolrClient());
+ cluster.waitForActiveCollection(COLLECTION, 2, 2);
+
+ SolrInputDocument doc = new SolrInputDocument("id", "1", "title_s", "my
doc");
+ httpBasedCloudSolrClient.add(COLLECTION, doc);
+ httpBasedCloudSolrClient.commit(COLLECTION);
+
+ LogListener entireClusterStateLogs =
+ LogListener.debug(BaseHttpClusterStateProvider.class)
+ .substring("Making a call to Solr to fetch entire cluster state");
+ LogListener collectionClusterStateLogs =
+ LogListener.debug(BaseHttpClusterStateProvider.class)
+ .substring("Making a call to Solr to fetch cluster state for
collection");
Review Comment:
You added both log calls just so that you could expressly listen for them
here. But that wasn't necessary; you could just listen for the request log
(already at INFO) on HttpSolrCall. Using the request log also gives better
assurance that you didn't miss some other request (maybe a future regression).
--
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]
