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/11/10 00:12:38 UTC
[GitHub] [lucene-solr] madrob commented on a change in pull request #2067: SOLR-14987: Reuse HttpSolrClient per node vs. one per Solr core when using CloudSolrStream
madrob commented on a change in pull request #2067:
URL: https://github.com/apache/lucene-solr/pull/2067#discussion_r520198395
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/CloudSolrStream.java
##########
@@ -334,11 +334,6 @@ private StreamComparator parseComp(String sort, String fl) throws IOException {
public static Slice[] getSlices(String collectionName, ZkStateReader zkStateReader, boolean checkAlias) throws IOException {
ClusterState clusterState = zkStateReader.getClusterState();
- Map<String, DocCollection> collectionsMap = clusterState.getCollectionsMap();
Review comment:
related: can we update the javadoc on clusterState.getCollectionsMap to be more explicit that it _will_ make a call to zk, instead of the current _may_
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/CloudSolrStream.java
##########
@@ -334,11 +334,6 @@ private StreamComparator parseComp(String sort, String fl) throws IOException {
public static Slice[] getSlices(String collectionName, ZkStateReader zkStateReader, boolean checkAlias) throws IOException {
ClusterState clusterState = zkStateReader.getClusterState();
- Map<String, DocCollection> collectionsMap = clusterState.getCollectionsMap();
-
- //TODO we should probably split collection by comma to query more than one
- // which is something already supported in other parts of Solr
-
// check for alias or collection
Review comment:
Should we cache the value of `zkStateReader.getAliases` below to avoid volatile reads?
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/SolrStream.java
##########
@@ -126,6 +135,17 @@ public void open() throws IOException {
}
}
+ private String getNodeUrl() {
Review comment:
Can we precomute this in the constructor?
----------------------------------------------------------------
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