dsmiley commented on code in PR #4320:
URL: https://github.com/apache/solr/pull/4320#discussion_r3177600952
##########
solr/modules/sql/src/java/org/apache/solr/handler/sql/SolrSchema.java:
##########
@@ -86,7 +86,8 @@ public boolean isClosed() {
@Override
protected Map<String, Table> getTableMap() {
String zk = this.properties.getProperty("zk");
Review Comment:
it's reasonably in scope to switch this out too... still supporting "zk" for
back-compat.
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/FeaturesSelectionStream.java:
##########
@@ -411,7 +422,7 @@ public NamedList<?> call() throws Exception {
params.add(DISTRIB, "false");
params.add("fq", "{!igain}");
- for (Map.Entry<String, String> entry : paramsMap.entrySet()) {
+ for (Map.Entry<String, String[]> entry : paramsMap) {
params.add(entry.getKey(), entry.getValue());
}
Review Comment:
the switch to SolrParams instead of a Map means we can simply call
`params.add(paramsMap)` in here and some other places in this big PR
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/RandomStream.java:
##########
@@ -135,12 +138,14 @@ public StreamExpressionParameter
toExpression(StreamFactory factory) throws IOEx
expression.addParameter(collection);
// parameters
- for (Entry<String, String> param : props.entrySet()) {
- expression.addParameter(new
StreamExpressionNamedParameter(param.getKey(), param.getValue()));
+ for (Entry<String, String[]> param : props) {
+ for (String paramValue : param.getValue()) {
+ expression.addParameter(new
StreamExpressionNamedParameter(param.getKey(), paramValue));
+ }
}
Review Comment:
I've seen this enough times that think there ought to be a utility method,
maybe on a base class, that does this. up to you.
##########
solr/modules/sql/src/java/org/apache/solr/handler/sql/SolrTable.java:
##########
@@ -295,7 +297,7 @@ private Metric getMetric(Pair<String, String> metricPair) {
}
private TupleStream handleSelect(
- String zk,
+ CloudSolrClient.CloudSolrClientConnection solrClientConnection,
Review Comment:
minor: rename to solrConnection
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/FeaturesSelectionStream.java:
##########
@@ -381,14 +392,14 @@ protected static class FeaturesSelectionCall implements
Callable<NamedList<?>> {
private final String baseUrl;
private final String outcome;
private final String field;
- private final Map<String, String> paramsMap;
Review Comment:
In many places like here you declare a param or field with the type
ModifiableSolrParams but that's too specific -- SolrParams should be used.
##########
solr/core/src/java/org/apache/solr/search/join/CrossCollectionJoinQuery.java:
##########
@@ -260,7 +261,8 @@ private TupleStream createCloudSolrStream(SolrClientCache
solrClientCache) throw
streamContext.setRequestReplicaListTransformerGenerator(rltg);
}
- TupleStream cloudSolrStream = new CloudSolrStream(streamZkHost,
collection, params);
+ var solrConnection =
CloudSolrClient.CloudSolrClientConnection.parse(streamZkHost);
Review Comment:
it's reasonably in-scope to update cross-collection join queries to take a
"solrConnection" string from the join params, keeping zkHost for back-compat
fallback.
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/CloudSolrStream.java:
##########
@@ -106,9 +110,9 @@ public CloudSolrStream(StreamExpression expression,
StreamFactory factory) throw
expression));
}
- // Validate there are no unknown parameters - solrCloud/zkHost and alias
are namedParameter, so
- // we don't
- // need to count it twice
+ // Validate there are no unknown parameters - solrConnection/zkHost and
alias are
+ // namedParameter,
Review Comment:
wrap
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/SignificantTermsStream.java:
##########
@@ -395,26 +406,24 @@ public SignificantTermsCall(
@Override
public NamedList<?> call() throws Exception {
- ModifiableSolrParams params = new ModifiableSolrParams();
+ ModifiableSolrParams queryRequestParams = new ModifiableSolrParams();
Review Comment:
I recommend `SolrQuery` here, and it has some convenience methods for some
of these well known params too.
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java:
##########
@@ -76,36 +76,35 @@ public void setDefaultZKHost(String zkHost) {
}
}
- public synchronized CloudSolrClient getCloudSolrClient(String
connectionString) {
+ public synchronized CloudSolrClient getCloudSolrClient(
Review Comment:
I think we'll need a deprecated overload here for the previous signature
--
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]
