stefan-egli commented on code in PR #12:
URL:
https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2063865052
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -93,6 +97,12 @@ public TopologyView getTopology() {
.listInstances(localClusterView);
topology.addInstances(attachedInstances);
+ // Check if topology changes should be delayed
+ if (topologyReadinessHandler != null &&
topologyReadinessHandler.shouldDelayTopologyChange()) {
+ logger.debug("getTopology: topology changes are delayed, returning
old view");
+ return oldView;
+ }
+
Review Comment:
This only covers the startup case. For shutdown returning the `oldView` here
doesn't result in sending `TOPOLOGY_CHANGING` events, which is what should
happen. So this section needs to distinguish between startup and shutdown - and
in the shutdown case it should call `oldView.setNotCurrent();` (would probably
be good to have a test case for exactly this).
PS: calling also `handleIsolatedFromTopology();` (as is done in the above
try/catch block) would be an interesting addition. But I fear that would result
in the entire cluster noticing the change (and a `TOPOLOGY_CHANGING` for
everybody) - plus then it would stall the entire cluster, as it would expect a
stable (`current`) topology again - which the local instance wouldn't do, as it
now behaves differently when terminating. So doing
`handleIsolatedFromTopology()` would be risky (might be good to have a test
case that ensures only the local instance gets a `TOPOLOGY_CHANGING` and none
of the other cluster members).
--
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: dev-unsubscr...@sling.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
