dsmiley commented on code in PR #3627:
URL: https://github.com/apache/solr/pull/3627#discussion_r2327475625
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -375,26 +375,38 @@ public ZkController(
this.overseerFailureMap = Overseer.getFailureMap(zkClient);
this.asyncIdsMap = Overseer.getAsyncIdsMap(zkClient);
+ createClusterZkNodes(zkClient);
+
zkStateReader =
new ZkStateReader(
zkClient,
() -> {
if (cc != null) cc.securityNodeChanged();
});
+ zkStateReader.createClusterStateWatchersAndUpdate(); // and reads cluster
properties
Review Comment:
moved from init() to immediately follow ZkStateReader construction. It does
critical initialization matters, including reading cluster properties.
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -375,26 +375,38 @@ public ZkController(
this.overseerFailureMap = Overseer.getFailureMap(zkClient);
this.asyncIdsMap = Overseer.getAsyncIdsMap(zkClient);
+ createClusterZkNodes(zkClient);
Review Comment:
Moved from init(). This particular destination seems best to me as it
doesn't require ZkStateReader so it can precede the construction of that.
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -1068,16 +1080,6 @@ private static void repairSecurityJson(SolrZkClient
zkClient)
private void init() {
Review Comment:
IMO all of init() should be inlined to it's only caller and don't bother
with the try-catch. This would add clarity to the initialization sequence.
I'm all for extracting a single-caller method but only if we can be clear about
what exactly we are initializing in the method name
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -375,26 +375,38 @@ public ZkController(
this.overseerFailureMap = Overseer.getFailureMap(zkClient);
this.asyncIdsMap = Overseer.getAsyncIdsMap(zkClient);
+ createClusterZkNodes(zkClient);
+
zkStateReader =
new ZkStateReader(
zkClient,
() -> {
if (cc != null) cc.securityNodeChanged();
});
+ zkStateReader.createClusterStateWatchersAndUpdate(); // and reads cluster
properties
+
+ // note: Can't read cluster properties until createClusterState ^ is called
+ final String urlSchemeFromClusterProp =
+ zkStateReader.getClusterProperty(ZkStateReader.URL_SCHEME,
ZkStateReader.HTTP);
+ // this must happen after zkStateReader has initialized the cluster props
+ this.baseURL = URLUtil.getBaseUrlForNodeName(this.nodeName,
urlSchemeFromClusterProp);
+
// Now that zkStateReader is available, read OVERSEER_ENABLED.
- // When overseerEnabled is false, both distributed features should be
enabled
- Boolean overseerEnabled =
- zkStateReader.getClusterProperty(ZkStateReader.OVERSEER_ENABLED, null);
- if (overseerEnabled == null) {
- overseerEnabled =
EnvUtils.getPropertyAsBool("solr.cloud.overseer.enabled", true);
- }
+ final boolean overseerEnabled =
+ Boolean.parseBoolean(
+ String.valueOf(
Review Comment:
Must parse strings. Strings very easily end up in cluster prop values,
despite Json, the backing storage, supporting native boolean. Both HTTP API &
CLI mechanisms set as string not boolean.
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -375,26 +375,38 @@ public ZkController(
this.overseerFailureMap = Overseer.getFailureMap(zkClient);
this.asyncIdsMap = Overseer.getAsyncIdsMap(zkClient);
+ createClusterZkNodes(zkClient);
+
zkStateReader =
new ZkStateReader(
zkClient,
() -> {
if (cc != null) cc.securityNodeChanged();
});
+ zkStateReader.createClusterStateWatchersAndUpdate(); // and reads cluster
properties
+
+ // note: Can't read cluster properties until createClusterState ^ is called
+ final String urlSchemeFromClusterProp =
+ zkStateReader.getClusterProperty(ZkStateReader.URL_SCHEME,
ZkStateReader.HTTP);
+ // this must happen after zkStateReader has initialized the cluster props
+ this.baseURL = URLUtil.getBaseUrlForNodeName(this.nodeName,
urlSchemeFromClusterProp);
Review Comment:
moved this from init() as well. It was and is adjacent to
`zkStateReader.createClusterStateWatchersAndUpdate()`
--
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]
