sigram commented on code in PR #4934:
URL: https://github.com/apache/solr/pull/4934#discussion_r4092315624
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -1022,6 +1044,35 @@ public AllowListZkHostChecker
getAllowListZkHostChecker() {
return allowListZkHostChecker;
}
+ /**
+ * Validates a connection to a SolrCloud cluster: ZooKeeper via {@link
+ * #getAllowListZkHostChecker()}, HTTP via {@link AllowListUrlChecker} (live
nodes of this cluster
+ * are allowed).
+ *
+ * @throws SolrException FORBIDDEN if not allowed
+ */
+ public void validateSolrConnection(CloudSolrClient.CloudSolrClientConnection
solrConnection) {
+ if (solrConnection.isZookeeper()) {
+ String zkHost = solrConnection.toString();
+ if (!allowListZkHostChecker.isAllowed(zkHost)) {
Review Comment:
It's awkward that the two checkers behave differently for the same "check
urls" operation... `AllowListZkHostChecker.isAllowed` returns a boolean and
then you have to make it into an exception and remember to use the same error
code, while `AllowListUrlChecker.checkAllowList` simply throws an exception for
you, with the right code.
##########
solr/core/src/java/org/apache/solr/search/join/CrossCollectionJoinQuery.java:
##########
@@ -244,22 +243,20 @@ private TupleStream createCloudSolrStream(SolrClientCache
solrClientCache) throw
params.set(CommonParams.WT, CommonParams.JAVABIN);
StreamContext streamContext = new StreamContext();
- streamContext.setSolrClientCache(solrClientCache);
+ streamContext.setSolrClientCache(zkController.getSolrClientCache());
Review Comment:
Previously the check for zkController != null prevented NPE just when
building the transformer. There's no check here so if it's null we will get an
NPE both here and in the transformer construction below.
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java:
##########
@@ -39,74 +37,44 @@ public class SolrClientCache implements Closeable {
// Set the floor for timeouts to 60 seconds.
// Timeouts can be increased by setting the system properties defined below.
private static final int MIN_TIMEOUT = 60000;
- private static final int minConnTimeout =
+ protected static final int minConnTimeout =
Math.max(
Integer.getInteger(SolrHttpConstants.PROP_CONNECTION_TIMEOUT,
MIN_TIMEOUT), MIN_TIMEOUT);
- private static final int minSocketTimeout =
+ protected static final int minSocketTimeout =
Math.max(Integer.getInteger(SolrHttpConstants.PROP_SO_TIMEOUT,
MIN_TIMEOUT), MIN_TIMEOUT);
- protected String basicAuthCredentials = null; // Only support with the
httpJettySolrClient
+ /** The only permitted subclass when running inside a Solr server
(solr.solr.home is set). */
+ protected static final String INTERNAL_IMPL_CLASS =
Review Comment:
I'm not sure I like depending on such magic strings ... why not add a
`boolean isInternal()` to each implementation?
--
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]
