dsmiley commented on code in PR #4320:
URL: https://github.com/apache/solr/pull/4320#discussion_r3295157183
##########
solr/core/src/java/org/apache/solr/search/join/CrossCollectionJoinQuery.java:
##########
@@ -302,7 +305,7 @@ private TupleStream createSolrStream() {
private DocSet getDocSet() throws IOException {
SolrClientCache solrClientCache =
searcher.getCore().getCoreContainer().getSolrClientCache();
Review Comment:
this line should move down to where we actually need it, guarded by an 'if'
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/expr/StreamFactory.java:
##########
@@ -91,13 +124,30 @@ public String getDefaultSort() {
return this.defaultSort;
}
+ /**
+ * @deprecated use {@link #getDefaultSolrConnection()}
+ */
+ @Deprecated
public String getDefaultZkHost() {
- return this.defaultZkHost;
+ return getDefaultSolrConnection().toString();
+ }
+
+ public CloudSolrClient.CloudSolrClientConnection getDefaultSolrConnection() {
+ return this.defaultSolrConnection;
}
+ /**
+ * @deprecated use {@link #getCollectionSolrConnection(String)}
+ */
+ @Deprecated
public String getCollectionZkHost(String collectionName) {
- if (this.collectionZkHosts.containsKey(collectionName)) {
- return this.collectionZkHosts.get(collectionName);
+ return getCollectionSolrConnection(collectionName).toString();
+ }
+
+ public CloudSolrClient.CloudSolrClientConnection getCollectionSolrConnection(
Review Comment:
I suggest name `getConnectionForCollection(String name) and it should not
return null -- defaulting to the default connection on the cache. Notice every
caller checks null and grabs the default.
##########
solr/core/src/java/org/apache/solr/handler/StreamHandler.java:
##########
@@ -111,15 +112,16 @@ public void inform(SolrCore core) {
if (coreContainer.isZooKeeperAware()) {
defaultCollection = core.getCoreDescriptor().getCollectionName();
defaultZkhost =
core.getCoreContainer().getZkController().getZkServerAddress();
- streamFactory.withCollectionZkHost(defaultCollection, defaultZkhost);
- streamFactory.withDefaultZkHost(defaultZkhost);
+ var solrConnection =
CloudSolrClient.CloudSolrClientConnection.parse(defaultZkhost);
+ streamFactory.withCollectionSolrConnection(defaultCollection,
solrConnection);
+ streamFactory.withDefaultSolrConnection(solrConnection);
Review Comment:
this line seems redundant with the previous
##########
solr/modules/sql/src/java/org/apache/solr/handler/sql/CalciteSolrDriver.java:
##########
@@ -87,7 +89,13 @@ public Connection connect(String url, Properties info)
throws SQLException {
if (schemaName == null) {
throw new SQLException("zk must be set");
}
- final SolrSchema solrSchema = new SolrSchema(info, solrClientCache);
+ var solrConnection =
CloudSolrClient.CloudSolrClientConnection.parse(schemaName);
+ if (!solrConnection.isZookeeper()) {
+ throw new SQLException(
+ String.format(
+ Locale.ROOT, "Expected ZooKeeper connection string, but got:
'%s'.", schemaName));
+ }
Review Comment:
not sure there's any point in ensuring this.
##########
solr/core/src/java/org/apache/solr/core/InternalSolrClientCache.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.core;
+
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.impl.HttpSolrClient;
+import org.apache.solr.client.solrj.io.SolrClientCache;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.util.EnvUtils;
+
+/** A restricted {@link SolrClientCache} that only permits access local solr
cluster */
+public class InternalSolrClientCache extends SolrClientCache {
Review Comment:
I think this would be better placed in the "cloud" package. The system prop
references cloud; it mostly has to do with SolrCloud.
##########
solr/core/src/java/org/apache/solr/search/join/CrossCollectionJoinQuery.java:
##########
@@ -224,11 +225,12 @@ private String createHashRangeFq() {
private TupleStream createCloudSolrStream(SolrClientCache solrClientCache)
throws IOException {
ZkController zkController =
searcher.getCore().getCoreContainer().getZkController();
- String streamZkHost;
- if (zkHost != null) {
- streamZkHost = zkHost;
+ CloudSolrClient.CloudSolrClientConnection streamingSolrConnection;
+ if (solrConnection != null) {
+ streamingSolrConnection = solrConnection;
} else {
- streamZkHost = zkController.getZkServerAddress();
+ streamingSolrConnection =
+
CloudSolrClient.CloudSolrClientConnection.parse(zkController.getZkServerAddress());
Review Comment:
not a big deal but couldn't we get the default from the solrClientCache?
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/expr/StreamFactory.java:
##########
@@ -42,24 +43,36 @@
/** Used to convert strings into stream expressions */
public class StreamFactory implements Serializable {
- private transient HashMap<String, String> collectionZkHosts;
+ private transient HashMap<String, CloudSolrClient.CloudSolrClientConnection>
+ collectionSolrConnection;
private transient HashMap<String, Supplier<Class<? extends Expressible>>>
functionNames;
- private transient String defaultZkHost;
+ private transient CloudSolrClient.CloudSolrClientConnection
defaultSolrConnection;
private transient String defaultCollection;
private transient String defaultSort;
public StreamFactory() {
- collectionZkHosts = new HashMap<>();
+ collectionSolrConnection = new HashMap<>();
functionNames = new HashMap<>();
}
public StreamFactory(HashMap<String, Supplier<Class<? extends Expressible>>>
functionNames) {
this.functionNames = functionNames;
- collectionZkHosts = new HashMap<>();
+ collectionSolrConnection = new HashMap<>();
}
+ /**
+ * @deprecated use {@link #withCollectionSolrConnection(String,
+ * CloudSolrClient.CloudSolrClientConnection)}
+ */
+ @Deprecated
public StreamFactory withCollectionZkHost(String collectionName, String
zkHost) {
- this.collectionZkHosts.put(collectionName, zkHost);
+ var solrConnection = zkHostToSolrConnection(zkHost);
+ return withCollectionSolrConnection(collectionName, solrConnection);
+ }
+
+ public StreamFactory withCollectionSolrConnection(
Review Comment:
A matter of taste but I suggest `withCollectionUseThisConnection`
##########
solr/core/src/java/org/apache/solr/handler/GraphHandler.java:
##########
@@ -79,8 +80,9 @@ public void inform(SolrCore core) {
if (coreContainer.isZooKeeperAware()) {
defaultCollection = core.getCoreDescriptor().getCollectionName();
defaultZkhost =
core.getCoreContainer().getZkController().getZkServerAddress();
- streamFactory.withCollectionZkHost(defaultCollection, defaultZkhost);
- streamFactory.withDefaultZkHost(defaultZkhost);
+ var solrConnection =
CloudSolrClient.CloudSolrClientConnection.parse(defaultZkhost);
+ streamFactory.withCollectionSolrConnection(defaultCollection,
solrConnection);
+ streamFactory.withDefaultSolrConnection(solrConnection);
Review Comment:
this line appears redundant with the previous
--
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]
