You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org> on 2026/02/23 23:27:25 UTC
[PR] [SOLR-18056] Added method to get url scheme based on cluster type [solr]
VishnuPriyaChandraSekar opened a new pull request, #4162:
URL: https://github.com/apache/solr/pull/4162
https://issues.apache.org/jira/browse/SOLR-18056
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Solr:
* https://issues.apache.org/jira/projects/SOLR
For something minor (i.e. that wouldn't be worth putting in release notes), you can skip JIRA.
To create a Jira issue, you will need to create an account there first.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# Description
Currently, `CloudSolrClient` depends on the cluster property to determine the url scheme for Zookeeper and Http based cluster. This approach is rigid and non-user friendly. For example: url scheme can be determined using solr URLs for Http based cluster but currently the user has to unnecessarily has to provide the 'URL_SCHEME' in the cluster properties.
# Solution
Added `getUrlScheme` method in `ClusterStateProvider` abstract class so the url scheme can be determined based on cluster type.
* `HttpClusterStateProvider` will determine the scheme from solr Urls
* `ZkClientClusterStateProvider` will determine the scheme with the help of cluster property if available otherwise it depends on system property `solr.ssl.enabled` (Similar to the solution recommended in [SOLR-18055](https://issues.apache.org/jira/browse/SOLR-18055)
# Tests
* Added an integration test `ClusterStateProviderUrlSchemeTest.java` to verify the correctness of getUrlScheme in `HttpClusterStateProvider` and `ZkClientClusterStateProvider`
# Checklist
Please review the following and check all that apply:
- [X] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [X] I have created a Jira issue and added the issue ID to my pull request title.
- [X] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
- [X] I have developed this patch against the `main` branch.
- [X] I have run `./gradlew check`.
- [X] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
- [X] I have added a [changelog entry](https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc) for my change
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] [SOLR-18056] Added method to get url scheme based on cluster type [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4162:
URL: https://github.com/apache/solr/pull/4162#discussion_r2849828800
##########
changelog/unreleased/SOLR-18056-urlScheme-csp.yml:
##########
@@ -0,0 +1,8 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: Added method to get url scheme based on cluster type
Review Comment:
```suggestion
title: Improved CloudSolrClient's urlScheme detection by using the scheme of provided Solr URLs, or looking at "solr.ssl.enabled".
```
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java:
##########
@@ -339,4 +339,16 @@ public int getZkClientTimeout() {
public void setZkClientTimeout(int zkClientTimeout) {
this.zkClientTimeout = zkClientTimeout;
}
+
+ /**
+ * @return url scheme with the help of cluster property or environment variable.
+ */
+ @Override
+ public String getUrlScheme() {
+ Object urlSchemeClusterProperty = getClusterProperty(ClusterState.URL_SCHEME);
+ if (urlSchemeClusterProperty != null) {
+ return urlSchemeClusterProperty.toString();
+ }
+ return Boolean.parseBoolean(System.getProperty(SOLR_SSL_ENABLED, "false")) ? "https" : "http";
Review Comment:
This should come first -- thus if set, we ignore the cluster properties
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderUrlSchemeTest.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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.nio.file.Path;
+import java.util.List;
+import org.apache.solr.client.solrj.jetty.HttpJettySolrClient;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.cloud.ZkController;
+import org.apache.solr.cloud.ZkTestServer;
+import org.apache.solr.common.cloud.ClusterProperties;
+import org.apache.solr.common.cloud.SolrZkClient;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.junit.Test;
+
+public class ClusterStateProviderUrlSchemeTest extends SolrCloudTestCase {
+ private static final String SOLR_SSL_ENABLED = "solr.ssl.enabled";
+
+ @Test
+ public void testHttpClusterStateProviderUrlScheme() throws Exception {
+ List<String> solrUrls = List.of("http://localhost:8983/solr");
+ try (var httpClient = new HttpJettySolrClient.Builder().build();
+ ClusterStateProvider clusterStateProvider =
+ new HttpClusterStateProvider<>(solrUrls, httpClient)) {
+ assertEquals("http", clusterStateProvider.getUrlScheme());
+ }
+ }
+
+ @Test
+ public void testHttpClusterStateProviderUrlSchemeWithMixedProtocol() {
+ List<String> solrUrls = List.of("http://localhost:8983/solr", "https://localhost:8984/solr");
+ try (var httpClient = new HttpJettySolrClient.Builder().build()) {
+ expectThrows(
+ IllegalArgumentException.class,
+ () -> new HttpClusterStateProvider<>(solrUrls, httpClient));
+ }
+ }
+
+ @Test
+ public void testZkClusterStateProviderUrlScheme() throws Exception {
+ Path zkDir = createTempDir("zkData");
+
+ ZkTestServer server = new ZkTestServer(zkDir);
+ try {
+ server.run();
+ SolrZkClient client = new SolrZkClient.Builder().withUrl(server.getZkAddress()).build();
+ ZkController.createClusterZkNodes(client);
+ testUrlSchemeWithClusterProperties(client);
+ testUrlSchemeDefault(client);
+ testUrlSchemeWithSystemProperties(client);
+ client.close();
+ } finally {
+ server.shutdown();
+ }
+ }
+
+ private void testUrlSchemeDefault(SolrZkClient client) throws Exception {
+ try (var zkStateReader = new ZkStateReader(client);
+ var clusterStateProvider = new ZkClientClusterStateProvider(zkStateReader)) {
+ assertEquals("http", clusterStateProvider.getUrlScheme());
+ }
+ }
+
+ private void testUrlSchemeWithSystemProperties(SolrZkClient client) throws Exception {
+ System.setProperty(SOLR_SSL_ENABLED, "true");
+ try (var zkStateReader = new ZkStateReader(client);
+ var clusterStateProvider = new ZkClientClusterStateProvider(zkStateReader)) {
+ assertEquals("https", clusterStateProvider.getUrlScheme());
+ } finally {
+ System.clearProperty(SOLR_SSL_ENABLED);
+ assertNull(System.getProperty(SOLR_SSL_ENABLED));
Review Comment:
jeesh; do we not trust Java?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderUrlSchemeTest.java:
##########
Review Comment:
Did you not want to put these tests into ClusterStateProviderTest ?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -73,11 +73,16 @@ public abstract class BaseHttpClusterStateProvider implements ClusterStateProvid
Executors.newSingleThreadScheduledExecutor(
new SolrNamedThreadFactory("liveNodeReloadingExecutor"));
- protected void initConfiguredNodes(List<String> solrUrls) throws Exception {
+ protected void initConfiguredNodes(List<String> solrUrls) {
this.configuredNodes =
solrUrls.stream()
.map(BaseHttpClusterStateProvider::stringToUrl)
.collect(Collectors.toList());
+ if (this.configuredNodes.stream().map(URL::getProtocol).collect(Collectors.toSet()).size()
Review Comment:
You could also consider simply not checking/enforcing this, and not even storing a field. Grabbing the first in `getScheme` is fine.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -73,11 +73,16 @@ public abstract class BaseHttpClusterStateProvider implements ClusterStateProvid
Executors.newSingleThreadScheduledExecutor(
new SolrNamedThreadFactory("liveNodeReloadingExecutor"));
- protected void initConfiguredNodes(List<String> solrUrls) throws Exception {
+ protected void initConfiguredNodes(List<String> solrUrls) {
this.configuredNodes =
solrUrls.stream()
.map(BaseHttpClusterStateProvider::stringToUrl)
.collect(Collectors.toList());
+ if (this.configuredNodes.stream().map(URL::getProtocol).collect(Collectors.toSet()).size()
Review Comment:
```suggestion
if (this.configuredNodes.stream().map(URL::getProtocol).distinct().count()
```
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ClusterStateProvider.java:
##########
@@ -127,4 +127,7 @@ default <T> T getClusterProperty(String key, T defaultValue) {
void connect();
String getQuorumHosts();
+
+ /** Get url scheme. */
Review Comment:
```suggestion
/** Get URL scheme -- http or https. Never null. */
```
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18056: Added method to get url scheme based on cluster type [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4162:
URL: https://github.com/apache/solr/pull/4162#discussion_r2850110492
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java:
##########
@@ -339,4 +339,16 @@ public int getZkClientTimeout() {
public void setZkClientTimeout(int zkClientTimeout) {
this.zkClientTimeout = zkClientTimeout;
}
+
+ /**
+ * @return url scheme with the help of cluster property or environment variable.
+ */
+ @Override
+ public String getUrlScheme() {
+ Object urlSchemeClusterProperty = getClusterProperty(ClusterState.URL_SCHEME);
+ if (urlSchemeClusterProperty != null) {
+ return urlSchemeClusterProperty.toString();
+ }
+ return Boolean.parseBoolean(System.getProperty(SOLR_SSL_ENABLED, "false")) ? "https" : "http";
Review Comment:
Got it :) Will update it in the next revision
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18056: Added method to get url scheme based on cluster type [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4162:
URL: https://github.com/apache/solr/pull/4162#discussion_r2850111692
##########
changelog/unreleased/SOLR-18056-urlScheme-csp.yml:
##########
@@ -0,0 +1,8 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: Added method to get url scheme based on cluster type
Review Comment:
Will fix this in the next revision
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18056: Added method to get url scheme based on cluster type [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4162:
URL: https://github.com/apache/solr/pull/4162#discussion_r2850128898
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java:
##########
@@ -339,4 +339,16 @@ public int getZkClientTimeout() {
public void setZkClientTimeout(int zkClientTimeout) {
this.zkClientTimeout = zkClientTimeout;
}
+
+ /**
+ * @return url scheme with the help of cluster property or environment variable.
+ */
+ @Override
+ public String getUrlScheme() {
+ Object urlSchemeClusterProperty = getClusterProperty(ClusterState.URL_SCHEME);
+ if (urlSchemeClusterProperty != null) {
+ return urlSchemeClusterProperty.toString();
+ }
+ return Boolean.parseBoolean(System.getProperty(SOLR_SSL_ENABLED, "false")) ? "https" : "http";
Review Comment:
Should it return "http" if both cluster property and system property are not set ?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18056: Added method to get url scheme based on cluster type [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4162:
URL: https://github.com/apache/solr/pull/4162#discussion_r2850138407
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java:
##########
@@ -339,4 +339,16 @@ public int getZkClientTimeout() {
public void setZkClientTimeout(int zkClientTimeout) {
this.zkClientTimeout = zkClientTimeout;
}
+
+ /**
+ * @return url scheme with the help of cluster property or environment variable.
+ */
+ @Override
+ public String getUrlScheme() {
+ Object urlSchemeClusterProperty = getClusterProperty(ClusterState.URL_SCHEME);
+ if (urlSchemeClusterProperty != null) {
+ return urlSchemeClusterProperty.toString();
+ }
+ return Boolean.parseBoolean(System.getProperty(SOLR_SSL_ENABLED, "false")) ? "https" : "http";
Review Comment:
yes
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18056: Added method to get url scheme based on cluster type [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #4162:
URL: https://github.com/apache/solr/pull/4162#issuecomment-3958937698
Do we cover the EnvUtils thing in our agent instructions?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18056: Added method to get url scheme based on cluster type [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4162:
URL: https://github.com/apache/solr/pull/4162#discussion_r2850139431
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ClusterStateProvider.java:
##########
@@ -127,4 +127,7 @@ default <T> T getClusterProperty(String key, T defaultValue) {
void connect();
String getQuorumHosts();
+
+ /** Get url scheme. */
Review Comment:
Hmm. Should it always be "http" or "https" ?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] [SOLR-18056] Added method to get url scheme based on cluster type [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4162:
URL: https://github.com/apache/solr/pull/4162#discussion_r2849890302
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java:
##########
@@ -339,4 +339,16 @@ public int getZkClientTimeout() {
public void setZkClientTimeout(int zkClientTimeout) {
this.zkClientTimeout = zkClientTimeout;
}
+
+ /**
+ * @return url scheme with the help of cluster property or environment variable.
+ */
+ @Override
+ public String getUrlScheme() {
+ Object urlSchemeClusterProperty = getClusterProperty(ClusterState.URL_SCHEME);
+ if (urlSchemeClusterProperty != null) {
+ return urlSchemeClusterProperty.toString();
+ }
+ return Boolean.parseBoolean(System.getProperty(SOLR_SSL_ENABLED, "false")) ? "https" : "http";
Review Comment:
Solr, even SolrJ, uses EnvUtils to read system properties. It's important here since this way we allow an env var approach SOLR_SSL_ENABLED
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18056: Added method to get url scheme based on cluster type [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4162:
URL: https://github.com/apache/solr/pull/4162#discussion_r2850171870
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -73,11 +73,16 @@ public abstract class BaseHttpClusterStateProvider implements ClusterStateProvid
Executors.newSingleThreadScheduledExecutor(
new SolrNamedThreadFactory("liveNodeReloadingExecutor"));
- protected void initConfiguredNodes(List<String> solrUrls) throws Exception {
+ protected void initConfiguredNodes(List<String> solrUrls) {
this.configuredNodes =
solrUrls.stream()
.map(BaseHttpClusterStateProvider::stringToUrl)
.collect(Collectors.toList());
+ if (this.configuredNodes.stream().map(URL::getProtocol).collect(Collectors.toSet()).size()
Review Comment:
Got it :) hopefully, user don't provide mixed protocols.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org