dsmiley commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3055049192
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -472,29 +472,41 @@ public void testZkClusterStateProviderUrlScheme() throws
Exception {
}
private void testUrlSchemeDefault(SolrZkClient client) throws Exception {
+ if (isSSLMode()) {
Review Comment:
why are you this back & forth? in some methods?
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -365,13 +365,7 @@ public ZkController(
});
zkStateReader.createClusterStateWatchersAndUpdate(); // and reads cluster
properties
-
- // note: Can't read cluster properties until createClusterState ^ is called
Review Comment:
this comment is important to keep
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##########
@@ -433,6 +429,16 @@ private String buildUrl(String url) {
return url;
}
+ /**
+ * Get url scheme of a standalone server based on system property
"solr.ssl.enabled".
Review Comment:
why speak of "standalone"?
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1240,8 +1241,20 @@ public String getBaseUrlForNodeName(final String
nodeName) {
* @return url that looks like {@code https://localhost:8983/api}
*/
public String getBaseUrlV2ForNodeName(final String nodeName) {
- String urlScheme = getClusterProperty(URL_SCHEME, "http");
- return Utils.getBaseUrlForNodeName(nodeName, urlScheme, true);
+ return URLUtil.getBaseUrlForNodeName(nodeName, getUrlScheme(), true);
+ }
+
+ /**
+ * Deduce url scheme using environment variable if available otherwise from
cluster property
+ *
+ * @return url scheme of host
Review Comment:
show the 2 examples please
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1240,8 +1241,20 @@ public String getBaseUrlForNodeName(final String
nodeName) {
* @return url that looks like {@code https://localhost:8983/api}
*/
public String getBaseUrlV2ForNodeName(final String nodeName) {
- String urlScheme = getClusterProperty(URL_SCHEME, "http");
- return Utils.getBaseUrlForNodeName(nodeName, urlScheme, true);
+ return URLUtil.getBaseUrlForNodeName(nodeName, getUrlScheme(), true);
+ }
+
+ /**
+ * Deduce url scheme using environment variable if available otherwise from
cluster property
Review Comment:
IMO _in general_, how a method does its job is not the business of the
caller to know. Less to document, less to potentially get out of sync.
--
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]
