dsmiley commented on code in PR #3643:
URL: https://github.com/apache/solr/pull/3643#discussion_r2335441804
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -413,7 +419,18 @@ public ZkController(
? Optional.of(new DistributedCollectionConfigSetCommandRunner(cc,
zkClient))
: Optional.empty();
- init();
+ if (lowestNodeVersion.isPresent()) {
+ checkClusterVersionCompatibility(lowestNodeVersion.get());
+ }
+
+ registerLiveNodesListener();
+
+ startOverseer();
Review Comment:
You'll see init() has been reduced to only starting the overseer. So now we
call it that.
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -392,13 +392,19 @@ public ZkController(
// 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.
+ checkForExistingEphemeralNode(); // removes our live node if present
Review Comment:
Moved from init() to occur a little sooner because we're about to read live
nodes (to get cluster version) right after. Just being thorough; seems this
check is an edge case.
##########
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java:
##########
@@ -1031,7 +1031,7 @@ public static class Builder {
private boolean trackJettyMetrics;
private boolean overseerEnabled =
- EnvUtils.getPropertyAsBool("solr.cloud.overseer.enabled", true);
+ EnvUtils.getPropertyAsBool("solr.cloud.overseer.enabled", false);
Review Comment:
It'd be tempting to put the randomized default here. I dunno... it might
couple this utility base classes that a 3rd party plugin writer may not want to
use. Granted this may already be the case; we don't have a real way of testing
that.
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -1078,47 +1097,25 @@ private static void repairSecurityJson(SolrZkClient
zkClient)
}
}
- private void init() {
- try {
- checkForExistingEphemeralNode();
- registerLiveNodesListener();
- checkClusterVersionCompatibility();
-
- // Start the overseer now since the following code may need it's
processing.
- // Note: even when using distributed processing, we still create an
Overseer anyway since
- // cluster singleton processing is linked to the elected Overseer.
- if (!zkRunOnly) {
- overseerElector = new LeaderElector(zkClient);
- this.overseer =
- new Overseer(
- (HttpShardHandler)
cc.getShardHandlerFactory().getShardHandler(),
- cc.getUpdateShardHandler(),
- CommonParams.CORES_HANDLER_PATH,
- zkStateReader,
- this,
- cloudConfig);
- ElectionContext context = new OverseerElectionContext(zkClient,
overseer, getNodeName());
- overseerElector.setup(context);
- if (cc.nodeRoles.isOverseerAllowedOrPreferred()) {
- overseerElector.joinElection(context, false);
- }
- }
-
- Stat stat = zkClient.exists(ZkStateReader.LIVE_NODES_ZKNODE, null, true);
- if (stat != null && stat.getNumChildren() > 0) {
- publishAndWaitForDownStates();
- }
-
- // Do this last to signal we're up.
- createEphemeralLiveNode();
- } catch (InterruptedException e) {
- // Restore the interrupted status
- Thread.currentThread().interrupt();
- log.error("", e);
- throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR, "",
e);
- } catch (KeeperException e) {
- log.error("", e);
- throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR, "",
e);
Review Comment:
This exception stuff is actually redundant. ZkContainer calls the
constructor of ZkController and it's got the same stuff.
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -413,7 +419,18 @@ public ZkController(
? Optional.of(new DistributedCollectionConfigSetCommandRunner(cc,
zkClient))
: Optional.empty();
- init();
+ if (lowestNodeVersion.isPresent()) {
+ checkClusterVersionCompatibility(lowestNodeVersion.get());
+ }
Review Comment:
Brought this out of init() because I want to pass lowestNodeVersion to it.
This was the catalyst of doing a bit of reorganization that you'll see in this
PR here. I hope it's not "too much" in the same PR that has an objective to
flip a default.
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -423,6 +440,10 @@ public ZkController(
this.overseerCollectionQueue = overseer.getCollectionQueue(zkClient);
this.overseerConfigSetQueue = overseer.getConfigSetQueue(zkClient);
this.sysPropsCacher = new
NodesSysPropsCacher(cc.getDefaultHttpSolrClient(), zkStateReader);
+
+ // Do this last to signal we're up.
+ createEphemeralLiveNode();
+
Review Comment:
Moved this to be even more "last" than it was :-)
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -423,6 +440,10 @@ public ZkController(
this.overseerCollectionQueue = overseer.getCollectionQueue(zkClient);
this.overseerConfigSetQueue = overseer.getConfigSetQueue(zkClient);
Review Comment:
It's tempting to continue moving things around... like these overseer queues
should probably be set up in startOverseer.
--
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]
