Re: [PR] SOLR-17877: fix overseerEnabled cluster prop [solr]

2025-09-11 Thread via GitHub 


dsmiley merged PR #3627:
URL: https://github.com/apache/solr/pull/3627


-- 
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]

Re: [PR] SOLR-17877: fix overseerEnabled cluster prop [solr]

2025-09-08 Thread via GitHub 


dsmiley commented on PR #3627:
URL: https://github.com/apache/solr/pull/3627#issuecomment-3268154136

   FWIW I did a bit of hands-on testing of this with a debugger on some tests 
to see that it was working as it should.  I should have done that on the prior 
PR.  My bad.
   
   Planning to merge Wednesday moring EST if no further review.
   
   My following action in pursuit of SOLR-17293 is simply to flip the default.


-- 
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]

Re: [PR] SOLR-17877: fix overseerEnabled cluster prop [solr]

2025-09-06 Thread via GitHub 


dsmiley commented on code in PR #3627:
URL: https://github.com/apache/solr/pull/3627#discussion_r2327879370


##
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java:
##
@@ -1108,20 +1108,6 @@ public Builder addConfig(String configName, Path 
configPath) {
   return this;
 }
 
-/**
- * This method makes the MiniSolrCloudCluster use the "other" Collection 
API execution strategy
- * than it normally would. When some test classes call this method (and 
some don't) we make sure
- * that a run of multiple tests with a single seed will exercise both code 
lines (distributed
- * and Overseer based Collection API) so regressions can be spotted faster.
- *
- * The real need is for a few tests covering reasonable use cases to 
call this method. If
- * you're adding a new test, you don't have to call it (but it's ok if you 
do).
- */
-public Builder flipOverseerEnablement() {
-  overseerEnabled = !overseerEnabled;
-  return this;
-}
-

Review Comment:
   I think there's no point to this really.  It's randomly set by 
SolrCloudTestCase.  A test can explicitly set it one way or the other if it 
wants to (and some do).



##
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java:
##
@@ -1181,7 +1167,7 @@ public MiniSolrCloudCluster configure() throws Exception {
  * @throws Exception if an error occurs on startup
  */
 public MiniSolrCloudCluster build() throws Exception {
-  this.clusterProperties.put(ZkStateReader.OVERSEER_ENABLED, 
Boolean.toString(overseerEnabled));
+  System.setProperty("solr.cloud.overseer.enabled", 
Boolean.toString(overseerEnabled));

Review Comment:
   Because there's a deficiency in MiniSolrCloudCluster in which cluster 
properties get set later (after node start) instead of early.  Not a big deal 
to do this instead since an entire test is going to set this one way or the 
other; not vary per cluster.  I'd rather a separate PR improve that specific 
matter but I'm not sure I'll get to it.



-- 
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]

Re: [PR] SOLR-17877: fix overseerEnabled cluster prop [solr]

2025-09-06 Thread via GitHub 


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