dsmiley commented on code in PR #4260:
URL: https://github.com/apache/solr/pull/4260#discussion_r3025338215
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -241,6 +241,59 @@ public Builder(ClusterStateProvider stateProvider) {
this.stateProvider = stateProvider;
}
+ /**
+ * Provide a universal connection string that can represent either:
+ *
+ * <ul>
+ * <li>A <b>Zookeeper ensemble</b> with optional chroot, e.g.
+ * <pre>"zk1:2181,zk2:2181,zk3:2181/solr"</pre>
+ * In this case, the client will use Zookeeper, to discover the Solr
cluster topology.
+ * <li>A list of <b>direct HTTP(S) Solr URLs</b>, e.g.
+ * <pre>"http://solr1:8983/solr,http://solr2:8983/solr";</pre>
+ * In this case, the client will directly connect to provided Solr
nodes and fetch cluster
+ * state via HTTP.
+ * </ul>
+ *
+ * <p>The parser automatically detects the mode based on the presence of a
scheme:
+ *
+ * <ul>
+ * <li>If any part starts with {@code http://} or {@code https://}, the
entire string is
+ * treated as a list of HTTP(S) URLs.
+ * <li>Otherwise, it treated as a ZooKeeper connection string (with
optional chroot)
+ * </ul>
+ *
+ * <p><b>Important:</b> Mixed schemes (e.g. zk hosts + HTTP URLs) are not
allowed and will
+ * result in an error when building the client.
+ *
+ * <p>Usage examples:
+ *
+ * <pre>{@code
+ * // ZooKeeper with chroot
+ * new CloudSolrClient.Builder("zk1:2181,zk2:2181,zk3:2181/solr");
+ *
+ * // ZooKeeper without chroot
+ * new CloudSolrClient.Builder("zk1:2181,zk2:2181,zk3:2181");
+ *
+ * // Direct HTTPS connections
+ * new
CloudSolrClient.Builder("https://solr1:8983/solr,https://solr2:8983/solr";);
+ * }</pre>
+ *
Review Comment:
I would drop all this honestly
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudClientConnectionString.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.client.solrj.impl;
+
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Locale;
+import java.util.Objects;
+
+/** Universal connection string parser logic. */
+public final class CloudClientConnectionString {
Review Comment:
Are you familiar with Java records?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudClientConnectionString.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.client.solrj.impl;
+
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Locale;
+import java.util.Objects;
+
+/** Universal connection string parser logic. */
+public final class CloudClientConnectionString {
+ private final boolean isZk;
+ private final List<String> quorumItems;
+ private final String zkChroot;
+
+ private CloudClientConnectionString(boolean isZk, List<String> quorumItems,
String zkChroot) {
+ this.isZk = isZk;
+ this.quorumItems = quorumItems;
+ this.zkChroot = zkChroot;
+ }
+
+ public static boolean isValidHttpUrl(String s) {
+ if (s == null || s.isBlank()) return false;
+
+ try {
+ URI uri = new URI(s);
+
+ return ("http".equalsIgnoreCase(uri.getScheme()) ||
"https".equalsIgnoreCase(uri.getScheme()))
+ && uri.getHost() != null;
+
+ } catch (Exception e) {
+ return false;
+ }
+ }
+
+ public static CloudClientConnectionString parse(String connectionString) {
+ if (connectionString == null || connectionString.trim().isEmpty()) {
+ throw new IllegalArgumentException("Connection string must not be null
or empty");
+ }
+ connectionString = connectionString.trim();
+ List<String> parts =
+ Arrays.stream(connectionString.split(","))
+ .map(String::trim)
+ .filter(s -> !s.isEmpty())
+ .toList();
+ if (parts.isEmpty()) {
+ throw new IllegalArgumentException(
+ "No valid hosts/urls found in connection string: " +
connectionString);
+ }
Review Comment:
can instead be in the constructor of the connection string
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudClientConnectionString.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.client.solrj.impl;
+
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Locale;
+import java.util.Objects;
+
+/** Universal connection string parser logic. */
+public final class CloudClientConnectionString {
+ private final boolean isZk;
+ private final List<String> quorumItems;
+ private final String zkChroot;
+
+ private CloudClientConnectionString(boolean isZk, List<String> quorumItems,
String zkChroot) {
+ this.isZk = isZk;
+ this.quorumItems = quorumItems;
+ this.zkChroot = zkChroot;
+ }
+
+ public static boolean isValidHttpUrl(String s) {
+ if (s == null || s.isBlank()) return false;
+
+ try {
+ URI uri = new URI(s);
+
+ return ("http".equalsIgnoreCase(uri.getScheme()) ||
"https".equalsIgnoreCase(uri.getScheme()))
+ && uri.getHost() != null;
+
+ } catch (Exception e) {
+ return false;
+ }
+ }
+
+ public static CloudClientConnectionString parse(String connectionString) {
+ if (connectionString == null || connectionString.trim().isEmpty()) {
+ throw new IllegalArgumentException("Connection string must not be null
or empty");
+ }
+ connectionString = connectionString.trim();
+ List<String> parts =
Review Comment:
IMO the very first thing after trimming to do is see if "://" is found and
bifercate 2 different parse methods based on that. Very simple. The ZK side
would then, first thing, extract off the trailing chroot, if present, before
doing comma splitting.
Just trying to suggest something simple.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -241,6 +241,59 @@ public Builder(ClusterStateProvider stateProvider) {
this.stateProvider = stateProvider;
}
+ /**
+ * Provide a universal connection string that can represent either:
Review Comment:
This method we're documenting does not "provide a connection string".
Javadocs should start with what the method does and secondarily say something
about the arguments.
disclaimer: I'm a stickler for javadoc style. And plenty of javadocs here
are non-compliant.
```suggestion
* Creates a client builder based on a connection string of 2 possible
formats:
```
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudClientConnectionString.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.client.solrj.impl;
+
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Locale;
+import java.util.Objects;
+
+/** Universal connection string parser logic. */
+public final class CloudClientConnectionString {
+ private final boolean isZk;
+ private final List<String> quorumItems;
+ private final String zkChroot;
+
+ private CloudClientConnectionString(boolean isZk, List<String> quorumItems,
String zkChroot) {
+ this.isZk = isZk;
+ this.quorumItems = quorumItems;
+ this.zkChroot = zkChroot;
+ }
+
+ public static boolean isValidHttpUrl(String s) {
+ if (s == null || s.isBlank()) return false;
+
+ try {
+ URI uri = new URI(s);
+
+ return ("http".equalsIgnoreCase(uri.getScheme()) ||
"https".equalsIgnoreCase(uri.getScheme()))
+ && uri.getHost() != null;
+
+ } catch (Exception e) {
+ return false;
+ }
+ }
+
+ public static CloudClientConnectionString parse(String connectionString) {
+ if (connectionString == null || connectionString.trim().isEmpty()) {
+ throw new IllegalArgumentException("Connection string must not be null
or empty");
+ }
+ connectionString = connectionString.trim();
+ List<String> parts =
+ Arrays.stream(connectionString.split(","))
+ .map(String::trim)
+ .filter(s -> !s.isEmpty())
+ .toList();
Review Comment:
instead use `org.apache.solr.common.util.StrUtils#split`
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -241,6 +241,59 @@ public Builder(ClusterStateProvider stateProvider) {
this.stateProvider = stateProvider;
}
+ /**
+ * Provide a universal connection string that can represent either:
+ *
+ * <ul>
+ * <li>A <b>Zookeeper ensemble</b> with optional chroot, e.g.
+ * <pre>"zk1:2181,zk2:2181,zk3:2181/solr"</pre>
+ * In this case, the client will use Zookeeper, to discover the Solr
cluster topology.
+ * <li>A list of <b>direct HTTP(S) Solr URLs</b>, e.g.
+ * <pre>"http://solr1:8983/solr,http://solr2:8983/solr";</pre>
+ * In this case, the client will directly connect to provided Solr
nodes and fetch cluster
+ * state via HTTP.
+ * </ul>
+ *
+ * <p>The parser automatically detects the mode based on the presence of a
scheme:
+ *
+ * <ul>
+ * <li>If any part starts with {@code http://} or {@code https://}, the
entire string is
+ * treated as a list of HTTP(S) URLs.
+ * <li>Otherwise, it treated as a ZooKeeper connection string (with
optional chroot)
+ * </ul>
+ *
+ * <p><b>Important:</b> Mixed schemes (e.g. zk hosts + HTTP URLs) are not
allowed and will
+ * result in an error when building the client.
+ *
+ * <p>Usage examples:
+ *
+ * <pre>{@code
+ * // ZooKeeper with chroot
+ * new CloudSolrClient.Builder("zk1:2181,zk2:2181,zk3:2181/solr");
+ *
+ * // ZooKeeper without chroot
+ * new CloudSolrClient.Builder("zk1:2181,zk2:2181,zk3:2181");
+ *
+ * // Direct HTTPS connections
+ * new
CloudSolrClient.Builder("https://solr1:8983/solr,https://solr2:8983/solr";);
+ * }</pre>
+ *
+ * @param connectionString a string specifying either ZooKeeper connection
string or HTTP(S)
+ * Solr URLs
+ * @throws IllegalArgumentException if string is null, empty, or malformed
+ * @see CloudClientConnectionString#parse(String) for parsing logic
+ * @since 11.0.0
+ */
+ public Builder(String connectionString) {
+ CloudClientConnectionString connStr =
CloudClientConnectionString.parse(connectionString);
+ if (connStr.isZk()) {
+ this.zkHosts = List.copyOf(connStr.getQuorumItems());
Review Comment:
no point in copying... the result of parsing isn't shared / at-risk of
shared use
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -241,6 +241,59 @@ public Builder(ClusterStateProvider stateProvider) {
this.stateProvider = stateProvider;
}
+ /**
+ * Provide a universal connection string that can represent either:
+ *
+ * <ul>
+ * <li>A <b>Zookeeper ensemble</b> with optional chroot, e.g.
+ * <pre>"zk1:2181,zk2:2181,zk3:2181/solr"</pre>
+ * In this case, the client will use Zookeeper, to discover the Solr
cluster topology.
+ * <li>A list of <b>direct HTTP(S) Solr URLs</b>, e.g.
+ * <pre>"http://solr1:8983/solr,http://solr2:8983/solr";</pre>
+ * In this case, the client will directly connect to provided Solr
nodes and fetch cluster
+ * state via HTTP.
+ * </ul>
+ *
+ * <p>The parser automatically detects the mode based on the presence of a
scheme:
+ *
+ * <ul>
+ * <li>If any part starts with {@code http://} or {@code https://}, the
entire string is
+ * treated as a list of HTTP(S) URLs.
+ * <li>Otherwise, it treated as a ZooKeeper connection string (with
optional chroot)
+ * </ul>
+ *
+ * <p><b>Important:</b> Mixed schemes (e.g. zk hosts + HTTP URLs) are not
allowed and will
+ * result in an error when building the client.
+ *
+ * <p>Usage examples:
+ *
+ * <pre>{@code
+ * // ZooKeeper with chroot
+ * new CloudSolrClient.Builder("zk1:2181,zk2:2181,zk3:2181/solr");
+ *
+ * // ZooKeeper without chroot
+ * new CloudSolrClient.Builder("zk1:2181,zk2:2181,zk3:2181");
+ *
+ * // Direct HTTPS connections
+ * new
CloudSolrClient.Builder("https://solr1:8983/solr,https://solr2:8983/solr";);
+ * }</pre>
+ *
+ * @param connectionString a string specifying either ZooKeeper connection
string or HTTP(S)
+ * Solr URLs
+ * @throws IllegalArgumentException if string is null, empty, or malformed
+ * @see CloudClientConnectionString#parse(String) for parsing logic
Review Comment:
no; that class seems should be a private detail, not a public API
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java:
##########
@@ -77,29 +77,33 @@ public void setDefaultZKHost(String zkHost) {
}
}
- public synchronized CloudSolrClient getCloudSolrClient(String zkHost) {
+ public synchronized CloudSolrClient getCloudSolrClient(String
connectionString) {
ensureOpen();
- Objects.requireNonNull(zkHost, "ZooKeeper host cannot be null!");
- if (solrClients.containsKey(zkHost)) {
- return (CloudSolrClient) solrClients.get(zkHost);
+ Objects.requireNonNull(connectionString, "Connection string cannot be
null!");
+ if (solrClients.containsKey(connectionString)) {
+ return (CloudSolrClient) solrClients.get(connectionString);
}
// Can only use ZK ACLs if there is a default ZK Host, and the given ZK
host contains that
// default.
// Basically the ZK ACLs are assumed to be only used for the default ZK
host,
// thus we should only provide the ACLs to that Zookeeper instance.
- String zkHostNoChroot = zkHost.split("/")[0];
- boolean canUseACLs =
-
Optional.ofNullable(defaultZkHost.get()).map(zkHostNoChroot::equals).orElse(false);
+ boolean canUseACLs = false;
+ CloudClientConnectionString cloudClientConnectionString =
+ CloudClientConnectionString.parse(connectionString);
+ if (cloudClientConnectionString.isZk()) {
+ String zkHostNoChroot = connectionString.split("/")[0];
Review Comment:
the chroot should be a field of cloudClientConnectionString
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudClientConnectionString.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.client.solrj.impl;
+
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Locale;
+import java.util.Objects;
+
+/** Universal connection string parser logic. */
Review Comment:
```suggestion
/** Parses ZK and HTTP SolrCloud connection strings. */
```
--
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]
