SOLR-18056: Added method to get url scheme based on cluster type#4162
Open
VishnuPriyaChandraSekar wants to merge 2 commits intoapache:mainfrom
Open
SOLR-18056: Added method to get url scheme based on cluster type#4162VishnuPriyaChandraSekar wants to merge 2 commits intoapache:mainfrom
VishnuPriyaChandraSekar wants to merge 2 commits intoapache:mainfrom
Conversation
# Conflicts: # changelog/unreleased/SOLR-18056-urlScheme-csp.yml
dsmiley
reviewed
Feb 24, 2026
Contributor
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
Thanks for working on this :-)
| @@ -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 | |||
Contributor
There was a problem hiding this comment.
Suggested change
| title: Added method to get url scheme based on cluster type | |
| title: Improved CloudSolrClient's urlScheme detection by using the scheme of provided Solr URLs, or looking at "solr.ssl.enabled". |
| if (urlSchemeClusterProperty != null) { | ||
| return urlSchemeClusterProperty.toString(); | ||
| } | ||
| return Boolean.parseBoolean(System.getProperty(SOLR_SSL_ENABLED, "false")) ? "https" : "http"; |
Contributor
There was a problem hiding this comment.
This should come first -- thus if set, we ignore the cluster properties
| solrUrls.stream() | ||
| .map(BaseHttpClusterStateProvider::stringToUrl) | ||
| .collect(Collectors.toList()); | ||
| if (this.configuredNodes.stream().map(URL::getProtocol).collect(Collectors.toSet()).size() |
Contributor
There was a problem hiding this comment.
Suggested change
| if (this.configuredNodes.stream().map(URL::getProtocol).collect(Collectors.toSet()).size() | |
| if (this.configuredNodes.stream().map(URL::getProtocol).distinct().count() |
Contributor
There was a problem hiding this comment.
You could also consider simply not checking/enforcing this, and not even storing a field. Grabbing the first in getScheme is fine.
|
|
||
| String getQuorumHosts(); | ||
|
|
||
| /** Get url scheme. */ |
Contributor
There was a problem hiding this comment.
Suggested change
| /** Get url scheme. */ | |
| /** Get URL scheme -- http or https. Never null. */ |
Contributor
There was a problem hiding this comment.
Did you not want to put these tests into ClusterStateProviderTest ?
| assertEquals("https", clusterStateProvider.getUrlScheme()); | ||
| } finally { | ||
| System.clearProperty(SOLR_SSL_ENABLED); | ||
| assertNull(System.getProperty(SOLR_SSL_ENABLED)); |
Contributor
There was a problem hiding this comment.
jeesh; do we not trust Java?
dsmiley
reviewed
Feb 24, 2026
| if (urlSchemeClusterProperty != null) { | ||
| return urlSchemeClusterProperty.toString(); | ||
| } | ||
| return Boolean.parseBoolean(System.getProperty(SOLR_SSL_ENABLED, "false")) ? "https" : "http"; |
Contributor
There was a problem hiding this 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
dsmiley
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://issues.apache.org/jira/browse/SOLR-18056
Description
Currently,
CloudSolrClientdepends 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
getUrlSchememethod inClusterStateProviderabstract class so the url scheme can be determined based on cluster type.HttpClusterStateProviderwill determine the scheme from solr UrlsZkClientClusterStateProviderwill determine the scheme with the help of cluster property if available otherwise it depends on system propertysolr.ssl.enabled(Similar to the solution recommended in SOLR-18055Tests
ClusterStateProviderUrlSchemeTest.javato verify the correctness of getUrlScheme inHttpClusterStateProviderandZkClientClusterStateProviderChecklist
Please review the following and check all that apply:
mainbranch../gradlew check.