You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by "ionutzpi (via GitHub)" <gi...@apache.org> on 2025/04/16 13:21:24 UTC
[PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
ionutzpi opened a new pull request, #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12
Add topology delay mechanism to ensure system stability during topology changes
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#issuecomment-2812368975
@ionutzpi , can you please explain what how the TopologyDelayHandler should work? at what times is it activated/deactivate wrt system readiness?
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2066023525
##########
bnd.bnd:
##########
@@ -4,4 +4,12 @@
# whitelist the private reference usage
-fixupmessages:"Export org.apache.sling.discovery.base.connectors.announcement, has 1, private references [org.apache.sling.discovery.base.connectors.announcement.impl]"; \
restrict:=warning; \
- is:=warn
\ No newline at end of file
+ is:=warn
+
+Bundle-Version: 3.0.0
Review Comment:
I thought that adding a new service in this bundle is a major change to it.
@cziegeler could you please let us know if adding a new service in this bundle should cause a new version of the bundle? I am not sure where the code changes should be put elsewhere other then discovery.base.
Maybe only a minor version is enough for this change.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2064268950
##########
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:
Done.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2056258606
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -93,7 +99,37 @@ public TopologyView getTopology() {
.listInstances(localClusterView);
topology.addInstances(attachedInstances);
+ // Check if topology changes should be delayed
+ if (topologyReadinessHandler != null && topologyReadinessHandler.shouldDelayTopologyChange(null)) {
+ logger.debug("getTopology: topology changes are delayed, returning old view");
+ return oldView;
+ }
+
return topology;
}
+ protected void handleTopologyEvent(TopologyEvent event) {
Review Comment:
See comment below about making TopologyReadinessHandler itself a TopologyEventListener - if that's not possible, then one open question might be how to register this TopologyReadinessHandler as a TopologyEventListener. At the moment noone calls its handleTopologyEvent yet. This registration needs to be carefully done as it sort of creates a circular dependency between DiscoveryService and TopologyReadinessHandler...
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -93,7 +99,37 @@ public TopologyView getTopology() {
.listInstances(localClusterView);
topology.addInstances(attachedInstances);
+ // Check if topology changes should be delayed
+ if (topologyReadinessHandler != null && topologyReadinessHandler.shouldDelayTopologyChange(null)) {
+ logger.debug("getTopology: topology changes are delayed, returning old view");
+ return oldView;
+ }
+
return topology;
}
+ protected void handleTopologyEvent(TopologyEvent event) {
+ if (event == null) {
+ return;
+ }
+
+ if (topologyReadinessHandler != null) {
+ if (topologyReadinessHandler.shouldDelayTopologyChange(event)) {
+ logger.debug("handleTopologyEvent: delaying topology event: {}", event);
+ return;
+ }
+
+ if (event.getType() == Type.TOPOLOGY_CHANGING) {
+ topologyReadinessHandler.startTopologyChange();
+ } else if (event.getType() == Type.TOPOLOGY_CHANGED) {
+ topologyReadinessHandler.endTopologyChange();
+ }
+ }
+
+ // Update old view when topology changes
+ if (event.getType() == Type.TOPOLOGY_CHANGED && event.getNewView() != null) {
+ setOldView((DefaultTopologyView) event.getNewView());
+ }
Review Comment:
What is the intention of this part? It looks problematic, it is not the expectation that a TopologyEventListener modifies the DiscoveryService's state - and it is actually already called by subclasses of BaseDiscoveryService. I think this is not needed (unless I'm missing something)...
##########
src/main/java/org/apache/sling/discovery/base/commons/TopologyReadinessHandler.java:
##########
@@ -0,0 +1,220 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.discovery.base.commons;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.felix.hc.api.condition.SystemReady;
+import org.apache.sling.discovery.DiscoveryService;
+import org.apache.sling.discovery.TopologyEvent;
+import org.apache.sling.discovery.TopologyView;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.service.component.annotations.ReferenceCardinality;
+import org.osgi.service.component.annotations.ReferencePolicy;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.osgi.service.component.ComponentContext;
+
+/**
+ * Coordinates topology changes based on system readiness state.
+ * This component ensures that topology changes only occur when the system is in a stable state,
+ * both during startup and shutdown sequences.
+ *
+ * The handler manages three main states:
+ * 1. STARTUP: Initial state when the system is starting up
+ * 2. READY: System is ready for normal operation and topology changes
+ * 3. SHUTDOWN: System is in the process of shutting down
+ *
+ * State Transitions:
+ * - System starts in STARTUP state
+ * - Transitions to READY state only when SystemReady service is bound
+ * - Transitions to SHUTDOWN state when:
+ * * SystemReady service is unbound
+ * * Component is deactivated
+ *
+ * Note: This component requires the Felix SystemReady service to function properly.
+ * The system will remain in STARTUP state until SystemReady service is bound,
+ * and will transition to SHUTDOWN state when the service is unbound.
+ */
+@Component(service = TopologyReadinessHandler.class, immediate = true)
+public class TopologyReadinessHandler {
+
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ /**
+ * Represents the possible states of the system.
+ * State transitions are controlled by SystemReady service binding/unbinding
+ * and component lifecycle events.
+ */
+ private enum SystemState {
+ STARTUP, // Initial state, waiting for SystemReady service
+ READY, // System is ready for normal operation
+ SHUTDOWN // System is shutting down
+ }
+
+ private final AtomicReference<SystemState> systemState = new AtomicReference<>(SystemState.STARTUP);
+ private final AtomicLong lastTopologyChangeTime = new AtomicLong(0);
+ private final AtomicBoolean topologyChangeInProgress = new AtomicBoolean(false);
+
+ private long delayDuration = 5000; // Default 5 second delay between topology changes
+ private long shutdownTimeout = 30000; // Default 30 second shutdown timeout
+
+ @Reference(cardinality = ReferenceCardinality.MANDATORY, policy = ReferencePolicy.STATIC)
+ private volatile SystemReady systemReady;
+
+ @Reference
+ private DiscoveryService discoveryService;
+
+ @Activate
+ protected void activate(ComponentContext context) {
+ logger.info("TopologyReadinessHandler activated - entering STARTUP state");
+ systemState.set(SystemState.STARTUP);
+ }
+
+ @Deactivate
+ protected void deactivate(ComponentContext context) {
+ logger.info("TopologyReadinessHandler deactivated");
+ initiateShutdown();
+ }
+
+ protected void bindSystemReady(SystemReady service) {
+ logger.debug("SystemReady service bound - transitioning to READY state");
+ if (systemState.compareAndSet(SystemState.STARTUP, SystemState.READY)) {
+ logger.info("System state changed to READY");
+ }
+ }
+
+ protected void unbindSystemReady(SystemReady service) {
+ logger.debug("SystemReady service unbound - initiating shutdown");
+ initiateShutdown();
+ }
+
+ /**
+ * Initiate the shutdown process
+ */
+ protected void initiateShutdown() {
+ if (systemState.compareAndSet(SystemState.READY, SystemState.SHUTDOWN) ||
+ systemState.compareAndSet(SystemState.STARTUP, SystemState.SHUTDOWN)) {
+ logger.info("Initiating shutdown process");
+
+ // Mark current view as not current
+ if (discoveryService != null) {
+ TopologyView currentView = discoveryService.getTopology();
+ if (currentView instanceof DefaultTopologyView) {
+ logger.info("Marking current topology view as not current during shutdown");
+ ((DefaultTopologyView) currentView).setNotCurrent();
+ }
+ }
+
+ // If shutdown timeout is disabled, don't wait
+ if (shutdownTimeout <= 0) {
+ return;
+ }
+
+ // Wait for running jobs to complete or timeout
+ long startTime = System.currentTimeMillis();
+ while (topologyChangeInProgress.get() &&
+ (System.currentTimeMillis() - startTime) < shutdownTimeout) {
+ try {
+ Thread.sleep(100);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ break;
+ }
+ }
+
+ if (topologyChangeInProgress.get()) {
+ logger.warn("Shutdown timeout reached while waiting for topology changes to complete");
+ } else {
+ logger.info("Shutdown completed successfully");
+ }
+ }
+ }
+
+ /**
+ * Set the shutdown timeout in milliseconds
+ * @param timeout the shutdown timeout in milliseconds
+ */
+ public void setShutdownTimeout(long timeout) {
+ this.shutdownTimeout = timeout;
+ }
+
+ /**
+ * Set the delay duration in milliseconds
+ * @param delayDuration the delay duration in milliseconds
+ */
+ public void setDelayDuration(long delayDuration) {
+ this.delayDuration = delayDuration;
+ }
+
+ /**
+ * Check if a topology change should be delayed based on system readiness
+ * @param event the topology event
+ * @return true if the change should be delayed, false otherwise
+ */
+ public boolean shouldDelayTopologyChange(TopologyEvent event) {
Review Comment:
Didn't dig into this code too much, but one thing I'm wondering: the design of these delayings of TopologyEvent seems to base on a very important assumption : that we go through the following phases:
1. we are starting up and not ready yet => delay
2. startup is finished and we are ready => no-delay
3. we are shutting down => delay
We should only ever go from 1. to 2., then from 2. to 3. There should be no other way. It shouldn't be possible to go from 2. back to 1 or from 3 back to 2 etc. If that were required, then the design doesn't work properly.
So if we're saying that it is exactly that order we can go through : in that case I'd suggest to actually write code to reflect exactly that. To have a little state machine that can go through exactly those 3 states and none other. That can then also be very well unit-tested etc.
On top of that little state machine would then come the hooks into activate/deactivate/bind/unbind/etc etc. Those could then call into the state machine to make it do one of the only 2 possible transitions (and fail in any other case).
I think such an intermediate state machine would make the code more robust and clearer.
At the moment it would seem that it is more coincidence that only those 2 transitions are possible. It's not something that's clear from the code...
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -93,7 +99,37 @@ public TopologyView getTopology() {
.listInstances(localClusterView);
topology.addInstances(attachedInstances);
+ // Check if topology changes should be delayed
+ if (topologyReadinessHandler != null && topologyReadinessHandler.shouldDelayTopologyChange(null)) {
+ logger.debug("getTopology: topology changes are delayed, returning old view");
+ return oldView;
+ }
+
return topology;
}
+ protected void handleTopologyEvent(TopologyEvent event) {
+ if (event == null) {
+ return;
+ }
+
+ if (topologyReadinessHandler != null) {
+ if (topologyReadinessHandler.shouldDelayTopologyChange(event)) {
+ logger.debug("handleTopologyEvent: delaying topology event: {}", event);
+ return;
+ }
+
+ if (event.getType() == Type.TOPOLOGY_CHANGING) {
+ topologyReadinessHandler.startTopologyChange();
+ } else if (event.getType() == Type.TOPOLOGY_CHANGED) {
+ topologyReadinessHandler.endTopologyChange();
+ }
Review Comment:
What about making TopologyReadinessHandler itself a TopologyEventListener? Then this indirection is not needed and things become a bit more straight forward..
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#issuecomment-2810792928
It's probably rather the opposite : without JoinerDelay this feature will cause issues, as instances during startup would block the topology...
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2066347443
##########
bnd.bnd:
##########
@@ -4,4 +4,12 @@
# whitelist the private reference usage
-fixupmessages:"Export org.apache.sling.discovery.base.connectors.announcement, has 1, private references [org.apache.sling.discovery.base.connectors.announcement.impl]"; \
restrict:=warning; \
- is:=warn
\ No newline at end of file
+ is:=warn
+
+Bundle-Version: 3.0.0
Review Comment:
Done. Remove increasing version, based on following statement: it makes sense to have this as an internal/implementation detail and not increasing the public api surface.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2067166165
##########
src/test/java/org/apache/sling/discovery/base/commons/TopologyReadinessHandlerTest.java:
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.discovery.base.commons;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.*;
+
+import org.apache.felix.hc.api.condition.SystemReady;
+import org.apache.sling.discovery.TopologyEvent;
+import org.apache.sling.discovery.TopologyEvent.Type;
+import org.apache.sling.discovery.TopologyView;
+import org.junit.Before;
+import org.junit.Test;
+import org.osgi.framework.BundleContext;
+import org.osgi.framework.ServiceReference;
+import org.osgi.service.component.ComponentContext;
+
+public class TopologyReadinessHandlerTest {
+
+ private TopologyReadinessHandler handler;
+ private SystemReady systemReadyService;
+ private ComponentContext componentContext;
+ private TopologyView oldView;
+
+ @Before
+ public void setup() {
+ handler = new TopologyReadinessHandler();
+ systemReadyService = mock(SystemReady.class);
+ componentContext = mock(ComponentContext.class);
+ BundleContext bundleContext = mock(BundleContext.class);
+ ServiceReference<SystemReady> serviceRef = mock(ServiceReference.class);
+
+ when(componentContext.getBundleContext()).thenReturn(bundleContext);
+ when(bundleContext.getServiceReference(SystemReady.class)).thenReturn(serviceRef);
+ when(bundleContext.getService(serviceRef)).thenReturn(systemReadyService);
+
+ oldView = mock(TopologyView.class);
+ handler.activate(componentContext);
+ }
+
+ @Test
+ public void testSystemNotReady() {
+ TopologyEvent event = new TopologyEvent(Type.TOPOLOGY_CHANGING, oldView, null);
Review Comment:
Done.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2067082089
##########
bnd.bnd:
##########
@@ -4,4 +4,7 @@
# whitelist the private reference usage
-fixupmessages:"Export org.apache.sling.discovery.base.connectors.announcement, has 1, private references [org.apache.sling.discovery.base.connectors.announcement.impl]"; \
restrict:=warning; \
- is:=warn
\ No newline at end of file
+ is:=warn
+
+# Set package versions
+Export-Package: org.apache.sling.discovery.base.commons;version=2.1.0
Review Comment:
It is needed. Without it, the build failed.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2066023525
##########
bnd.bnd:
##########
@@ -4,4 +4,12 @@
# whitelist the private reference usage
-fixupmessages:"Export org.apache.sling.discovery.base.connectors.announcement, has 1, private references [org.apache.sling.discovery.base.connectors.announcement.impl]"; \
restrict:=warning; \
- is:=warn
\ No newline at end of file
+ is:=warn
+
+Bundle-Version: 3.0.0
Review Comment:
I thought that adding a new service in this bundle is a major change to it.
@cziegeler could you please let us know if adding a new service in this bundle should cause a new version of the bundle? I am not sure where the code changes should be put elsewhere other then discovery.base.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2066023525
##########
bnd.bnd:
##########
@@ -4,4 +4,12 @@
# whitelist the private reference usage
-fixupmessages:"Export org.apache.sling.discovery.base.connectors.announcement, has 1, private references [org.apache.sling.discovery.base.connectors.announcement.impl]"; \
restrict:=warning; \
- is:=warn
\ No newline at end of file
+ is:=warn
+
+Bundle-Version: 3.0.0
Review Comment:
I thought that adding a new service in this bundle is a major change to it.
@cziegeler could you please let us know if adding a new service in this bundle should cause a new version of the bundle?
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2066347443
##########
bnd.bnd:
##########
@@ -4,4 +4,12 @@
# whitelist the private reference usage
-fixupmessages:"Export org.apache.sling.discovery.base.connectors.announcement, has 1, private references [org.apache.sling.discovery.base.connectors.announcement.impl]"; \
restrict:=warning; \
- is:=warn
\ No newline at end of file
+ is:=warn
+
+Bundle-Version: 3.0.0
Review Comment:
Remove increasing version, based on following statement: it makes sense to have this as an internal/implementation detail and not increasing the public api surface.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#issuecomment-2835097818
I deleted check in initiateShutdown. I am not sure what exactly should I do here?
...it should result in a TOPOLOGY_CHANGING event for all local listeners.
PS: essentially calling setNotCurrent has that effect
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2065892578
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -93,6 +97,18 @@ public TopologyView getTopology() {
.listInstances(localClusterView);
topology.addInstances(attachedInstances);
+ // Check if topology changes should be delayed
+ if (topologyReadinessHandler != null && topologyReadinessHandler.shouldDelayTopologyChange()) {
+ if (topologyReadinessHandler.isShutdown()) {
+ // On shutdown, mark oldView as not current to trigger TOPOLOGY_CHANGING
+ if (oldView != null) {
+ oldView.setNotCurrent();
+ }
+ }
+ logger.debug("getTopology: topology changes are delayed, returning old view");
+ return oldView;
Review Comment:
yes, I think this should work. I was wondering if the wording `shouldDelayTopologyChange` applies for the shutdown case too.. more of a naming aspect.
btw: in both cases the `oldView.isCurrent()` should return `false` after this section - for the startup case that should be implicitly already the case, for the normal case. There might be a race condition, although not very likely, where the `topologyReadinessHandler` is still `null` and thus this section is skipped. In that case the local instance would send a `TOPOLOGY_INIT` still. So perhaps it might actually make it more robust if for both startup and shutdown cases the `setNotCurrent` is explicitly called...
##########
src/main/java/org/apache/sling/discovery/base/commons/TopologyReadinessHandler.java:
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.discovery.base.commons;
+
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.felix.hc.api.condition.SystemReady;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.service.component.annotations.ReferenceCardinality;
+import org.osgi.service.component.annotations.ReferencePolicy;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.osgi.service.component.ComponentContext;
+
+/**
+ * Coordinates topology changes based on system readiness state.
+ * This component ensures that topology changes only occur when the system is in a stable state,
+ * both during startup and shutdown sequences.
+ *
+ * The handler manages three main states:
+ * 1. STARTUP: Initial state when the system is starting up
+ * 2. READY: System is ready for normal operation and topology changes
+ * 3. SHUTDOWN: System is in the process of shutting down
+ *
+ * State Transitions:
+ * - System starts in STARTUP state
+ * - Transitions to READY state only when SystemReady service is bound
+ * - Transitions to SHUTDOWN state when:
+ * * SystemReady service is unbound
+ * * Component is deactivated
+ *
+ * Note: This component requires the Felix SystemReady service to function properly.
+ * The system will remain in STARTUP state until SystemReady service is bound,
+ * and will transition to SHUTDOWN state when the service is unbound.
+ */
+@Component(service = TopologyReadinessHandler.class, immediate = true)
+public class TopologyReadinessHandler {
+
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ /**
+ * Represents the possible states of the system with explicit transitions.
+ */
+ private enum SystemState {
+ STARTUP {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { READY };
+ }
+ },
+ READY {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { SHUTDOWN };
+ }
+ },
+ SHUTDOWN {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { }; // No transitions allowed from SHUTDOWN
+ }
+ };
+
+ protected abstract SystemState[] getAllowedTransitions();
+ }
+
+ private final StateMachine stateMachine = new StateMachine();
+
+ @Reference(cardinality = ReferenceCardinality.MANDATORY, policy = ReferencePolicy.STATIC)
+ private volatile SystemReady systemReady;
Review Comment:
does it work like this? ( [docu](https://github.com/apache/felix-dev/blob/master/healthcheck/api/src/main/java/org/apache/felix/hc/api/condition/SystemReady.java) mentions it needs the tag `systemready` )
##########
bnd.bnd:
##########
@@ -4,4 +4,12 @@
# whitelist the private reference usage
-fixupmessages:"Export org.apache.sling.discovery.base.connectors.announcement, has 1, private references [org.apache.sling.discovery.base.connectors.announcement.impl]"; \
restrict:=warning; \
- is:=warn
\ No newline at end of file
+ is:=warn
+
+Bundle-Version: 3.0.0
Review Comment:
One thing I was always wondering: does the version need to be increased? Could the `TopologyReadinessHandler` not be an internal detail of discovery.base, and the package/major version would not have to be changed? (but there might be another reason that I'm missing)
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -41,12 +42,15 @@ public abstract class BaseDiscoveryService implements DiscoveryService {
/** the old view previously valid and sent to the TopologyEventListeners **/
private DefaultTopologyView oldView;
+ @Reference
+ private TopologyReadinessHandler topologyReadinessHandler;
+
protected abstract ClusterViewService getClusterViewService();
protected abstract AnnouncementRegistry getAnnouncementRegistry();
protected abstract void handleIsolatedFromTopology();
-
+
Review Comment:
detail: I'd remove this (there are a couple other similar cases in other files)
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#issuecomment-2830690640
Refactor TopologyReadinessHandler to depend on SystemReady only
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#issuecomment-2828247550
> it should result in a TOPOLOGY_CHANGING event for all local listeners.
PS: essentially calling `setNotCurrent` has that effect
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2058004921
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -93,7 +99,37 @@ public TopologyView getTopology() {
.listInstances(localClusterView);
topology.addInstances(attachedInstances);
+ // Check if topology changes should be delayed
+ if (topologyReadinessHandler != null && topologyReadinessHandler.shouldDelayTopologyChange(null)) {
+ logger.debug("getTopology: topology changes are delayed, returning old view");
+ return oldView;
+ }
+
return topology;
}
+ protected void handleTopologyEvent(TopologyEvent event) {
Review Comment:
Done. Moved to TopologyReadinessHandler
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2066188948
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -41,12 +42,15 @@ public abstract class BaseDiscoveryService implements DiscoveryService {
/** the old view previously valid and sent to the TopologyEventListeners **/
private DefaultTopologyView oldView;
+ @Reference
+ private TopologyReadinessHandler topologyReadinessHandler;
+
protected abstract ClusterViewService getClusterViewService();
protected abstract AnnouncementRegistry getAnnouncementRegistry();
protected abstract void handleIsolatedFromTopology();
-
+
Review Comment:
Done.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2067165718
##########
src/test/java/org/apache/sling/discovery/base/connectors/DummyVirtualInstanceBuilder.java:
##########
@@ -48,12 +50,12 @@ public VirtualInstanceBuilder setPath(String string) {
// nothing to do now
return this;
}
-
+
@Override
public Object[] getAdditionalServices(VirtualInstance instance) throws Exception {
return null;
}
-
+
Review Comment:
Done.
##########
src/test/java/org/apache/sling/discovery/base/connectors/DummyVirtualInstanceBuilder.java:
##########
@@ -48,12 +50,12 @@ public VirtualInstanceBuilder setPath(String string) {
// nothing to do now
return this;
}
-
+
Review Comment:
Done.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2068034223
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -93,6 +97,15 @@ public TopologyView getTopology() {
.listInstances(localClusterView);
topology.addInstances(attachedInstances);
+ // Check if topology changes should be delayed
+ if (topologyReadinessHandler != null && topologyReadinessHandler.shouldDelayTopologyChange()) {
+ if (oldView != null) {
+ oldView.setNotCurrent();
Review Comment:
Done.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#issuecomment-2810011169
IIUC then this is intended for health during startup - given the description mentions "until the system is in a stable state" or "until the system is ready".
Also I'm assuming the model where the rest of the local cluster treats the starting-up instance as already part of the cluster - while the starting-up instance itself wants to wait with topology initialization until readiness is signaled.
So that makes the logic somewhat more involved : things should work normally wrt discovery for everybody else, just for the local instance it is delayed.
For example: one problem that comes to mind as a result of this is : the JoinerDelay. That ensures that an instance that is already announcing itself through oak leases - but has not yet set all its sling-level infos - is suppressed (delayed), until it finishes its initialization completely. So this new feature here must work well with JoinerDelay, I guess is what I'm trying to say.
At the current state of this draft however, the BaseDiscoveryService is returning the {{oldView}} in case of not-yet-ready. That would be problematic, as that would mean it is not taking part of the JoinerDelay logic. Which would mean other cluster members could get delayed with establishing a topology.
I think the way the integration with JoinerDelay is going to look like is a major aspect of how this should be designed.
Other than that, I think BaseDiscoveryService shouldn't have to worry about TopologyEventListeners, and as such also not have manage that list in topologyEventListeners. That aspect is handled by the ViewStateManagerImpl.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#issuecomment-2810562287
On second thought, the JoinerDelay might actually work fine. I'd suggest to test this area well though..
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#issuecomment-2828207189
Wondering whether `TopologyReadinessHandler` indeed needs to be a `TopologyEventListener` after all. Why is `shouldDelayTopologyChange` dependent on `topologyChangeInProgress`? Also: why is it dependent on `delayDuration`?
Isn't it much simpler and all `TopologyReadinessHandler` needs is the dependency on `SystemReady`, and based on that, together with the stateMachine, `shouldDelayTopologyChange` basically just returns `!stateMachine.isReady()`?
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2066188554
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -93,6 +97,18 @@ public TopologyView getTopology() {
.listInstances(localClusterView);
topology.addInstances(attachedInstances);
+ // Check if topology changes should be delayed
+ if (topologyReadinessHandler != null && topologyReadinessHandler.shouldDelayTopologyChange()) {
+ if (topologyReadinessHandler.isShutdown()) {
+ // On shutdown, mark oldView as not current to trigger TOPOLOGY_CHANGING
+ if (oldView != null) {
+ oldView.setNotCurrent();
+ }
+ }
+ logger.debug("getTopology: topology changes are delayed, returning old view");
+ return oldView;
Review Comment:
Done. Calling setNotCurrent for both cases.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2068034994
##########
src/main/java/org/apache/sling/discovery/base/commons/TopologyReadinessHandler.java:
##########
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.discovery.base.commons;
+
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.felix.hc.api.condition.SystemReady;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.service.component.annotations.ReferenceCardinality;
+import org.osgi.service.component.annotations.ReferencePolicy;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.osgi.service.component.ComponentContext;
+
+/**
+ * Coordinates topology changes based on system readiness state.
+ * This component ensures that topology changes only occur when the system is in a stable state,
+ * both during startup and shutdown sequences.
+ *
+ * The handler manages three main states:
+ * 1. STARTUP: Initial state when the system is starting up
+ * 2. READY: System is ready for normal operation and topology changes
+ * 3. SHUTDOWN: System is in the process of shutting down
+ *
+ * State Transitions:
+ * - System starts in STARTUP state
+ * - Transitions to READY state only when SystemReady service is bound
+ * - Transitions to SHUTDOWN state when:
+ * * SystemReady service is unbound
+ * * Component is deactivated
+ *
+ * Note: This component requires the Felix SystemReady service to function properly.
+ * The system will remain in STARTUP state until SystemReady service is bound,
+ * and will transition to SHUTDOWN state when the service is unbound.
+ */
+@Component(service = TopologyReadinessHandler.class, immediate = true)
+public class TopologyReadinessHandler {
+
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ /**
+ * Represents the possible states of the system with explicit transitions.
+ */
+ private enum SystemState {
+ STARTUP {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { READY };
+ }
+ },
+ READY {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { SHUTDOWN };
+ }
+ },
+ SHUTDOWN {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { }; // No transitions allowed from SHUTDOWN
+ }
+ };
+
+ protected abstract SystemState[] getAllowedTransitions();
+ }
+
+ private final StateMachine stateMachine = new StateMachine();
+
+ @Reference(cardinality = ReferenceCardinality.MANDATORY, policy = ReferencePolicy.STATIC,
+ target = "(tag=systemready)")
+ private volatile SystemReady systemReady;
+
+ @Activate
+ protected void activate(ComponentContext context) {
+ logger.info("TopologyReadinessHandler activated - in STARTUP state");
+ }
+
+ @Deactivate
+ protected void deactivate(ComponentContext context) {
+ logger.info("TopologyReadinessHandler deactivated");
+ stateMachine.transitionTo(SystemState.SHUTDOWN);
+ }
+
+ public void bindSystemReady(SystemReady service) {
+ logger.debug("SystemReady service bound - transitioning to READY state");
+ stateMachine.transitionTo(SystemState.READY);
+ }
+
+ protected void unbindSystemReady(SystemReady service) {
+ logger.debug("SystemReady service unbound - transitioning to SHUTDOWN state");
+ stateMachine.transitionTo(SystemState.SHUTDOWN);
+ }
+
+ /**
+ * Initiate the shutdown process
+ */
+ public void initiateShutdown() {
Review Comment:
Removed unnecessary method.
##########
src/main/java/org/apache/sling/discovery/base/commons/TopologyReadinessHandler.java:
##########
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.discovery.base.commons;
+
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.felix.hc.api.condition.SystemReady;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.service.component.annotations.ReferenceCardinality;
+import org.osgi.service.component.annotations.ReferencePolicy;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.osgi.service.component.ComponentContext;
+
+/**
+ * Coordinates topology changes based on system readiness state.
+ * This component ensures that topology changes only occur when the system is in a stable state,
+ * both during startup and shutdown sequences.
+ *
+ * The handler manages three main states:
+ * 1. STARTUP: Initial state when the system is starting up
+ * 2. READY: System is ready for normal operation and topology changes
+ * 3. SHUTDOWN: System is in the process of shutting down
+ *
+ * State Transitions:
+ * - System starts in STARTUP state
+ * - Transitions to READY state only when SystemReady service is bound
+ * - Transitions to SHUTDOWN state when:
+ * * SystemReady service is unbound
+ * * Component is deactivated
+ *
+ * Note: This component requires the Felix SystemReady service to function properly.
+ * The system will remain in STARTUP state until SystemReady service is bound,
+ * and will transition to SHUTDOWN state when the service is unbound.
+ */
+@Component(service = TopologyReadinessHandler.class, immediate = true)
+public class TopologyReadinessHandler {
+
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ /**
+ * Represents the possible states of the system with explicit transitions.
+ */
+ private enum SystemState {
+ STARTUP {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { READY };
+ }
+ },
+ READY {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { SHUTDOWN };
+ }
+ },
+ SHUTDOWN {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { }; // No transitions allowed from SHUTDOWN
+ }
+ };
+
+ protected abstract SystemState[] getAllowedTransitions();
+ }
+
+ private final StateMachine stateMachine = new StateMachine();
+
+ @Reference(cardinality = ReferenceCardinality.MANDATORY, policy = ReferencePolicy.STATIC,
+ target = "(tag=systemready)")
+ private volatile SystemReady systemReady;
+
+ @Activate
+ protected void activate(ComponentContext context) {
+ logger.info("TopologyReadinessHandler activated - in STARTUP state");
+ }
+
+ @Deactivate
+ protected void deactivate(ComponentContext context) {
+ logger.info("TopologyReadinessHandler deactivated");
+ stateMachine.transitionTo(SystemState.SHUTDOWN);
+ }
+
+ public void bindSystemReady(SystemReady service) {
+ logger.debug("SystemReady service bound - transitioning to READY state");
+ stateMachine.transitionTo(SystemState.READY);
+ }
+
+ protected void unbindSystemReady(SystemReady service) {
+ logger.debug("SystemReady service unbound - transitioning to SHUTDOWN state");
+ stateMachine.transitionTo(SystemState.SHUTDOWN);
+ }
+
+ /**
+ * Initiate the shutdown process
+ */
+ public void initiateShutdown() {
+ if (stateMachine.getCurrentState() == SystemState.READY) {
+ logger.info("Initiating shutdown process");
+ stateMachine.transitionTo(SystemState.SHUTDOWN);
+ logger.info("Shutdown completed successfully");
+ }
+ }
+
+ /**
+ * Check if a topology change should be delayed based on system readiness
+ * @return true if the change should be delayed, false otherwise
+ */
+ public boolean shouldDelayTopologyChange() {
Review Comment:
Done.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2068033972
##########
src/main/java/org/apache/sling/discovery/base/commons/TopologyReadinessHandler.java:
##########
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.discovery.base.commons;
+
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.felix.hc.api.condition.SystemReady;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.service.component.annotations.ReferenceCardinality;
+import org.osgi.service.component.annotations.ReferencePolicy;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.osgi.service.component.ComponentContext;
+
+/**
+ * Coordinates topology changes based on system readiness state.
+ * This component ensures that topology changes only occur when the system is in a stable state,
+ * both during startup and shutdown sequences.
+ *
+ * The handler manages three main states:
+ * 1. STARTUP: Initial state when the system is starting up
+ * 2. READY: System is ready for normal operation and topology changes
+ * 3. SHUTDOWN: System is in the process of shutting down
+ *
+ * State Transitions:
+ * - System starts in STARTUP state
+ * - Transitions to READY state only when SystemReady service is bound
+ * - Transitions to SHUTDOWN state when:
+ * * SystemReady service is unbound
+ * * Component is deactivated
+ *
+ * Note: This component requires the Felix SystemReady service to function properly.
+ * The system will remain in STARTUP state until SystemReady service is bound,
+ * and will transition to SHUTDOWN state when the service is unbound.
+ */
+@Component(service = TopologyReadinessHandler.class, immediate = true)
+public class TopologyReadinessHandler {
+
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ /**
+ * Represents the possible states of the system with explicit transitions.
+ */
+ private enum SystemState {
+ STARTUP {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { READY };
+ }
+ },
+ READY {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { SHUTDOWN };
+ }
+ },
+ SHUTDOWN {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { }; // No transitions allowed from SHUTDOWN
+ }
+ };
+
+ protected abstract SystemState[] getAllowedTransitions();
+ }
+
+ private final StateMachine stateMachine = new StateMachine();
+
+ @Reference(cardinality = ReferenceCardinality.MANDATORY, policy = ReferencePolicy.STATIC,
+ target = "(tag=systemready)")
+ private volatile SystemReady systemReady;
+
+ @Activate
+ protected void activate(ComponentContext context) {
+ logger.info("TopologyReadinessHandler activated - in STARTUP state");
+ }
+
+ @Deactivate
+ protected void deactivate(ComponentContext context) {
+ logger.info("TopologyReadinessHandler deactivated");
+ stateMachine.transitionTo(SystemState.SHUTDOWN);
+ }
+
+ public void bindSystemReady(SystemReady service) {
+ logger.debug("SystemReady service bound - transitioning to READY state");
+ stateMachine.transitionTo(SystemState.READY);
+ }
+
+ protected void unbindSystemReady(SystemReady service) {
+ logger.debug("SystemReady service unbound - transitioning to SHUTDOWN state");
+ stateMachine.transitionTo(SystemState.SHUTDOWN);
Review Comment:
Done.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2058005594
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -93,7 +99,37 @@ public TopologyView getTopology() {
.listInstances(localClusterView);
topology.addInstances(attachedInstances);
+ // Check if topology changes should be delayed
+ if (topologyReadinessHandler != null && topologyReadinessHandler.shouldDelayTopologyChange(null)) {
+ logger.debug("getTopology: topology changes are delayed, returning old view");
+ return oldView;
+ }
+
return topology;
}
+ protected void handleTopologyEvent(TopologyEvent event) {
+ if (event == null) {
+ return;
+ }
+
+ if (topologyReadinessHandler != null) {
+ if (topologyReadinessHandler.shouldDelayTopologyChange(event)) {
+ logger.debug("handleTopologyEvent: delaying topology event: {}", event);
+ return;
+ }
+
+ if (event.getType() == Type.TOPOLOGY_CHANGING) {
+ topologyReadinessHandler.startTopologyChange();
+ } else if (event.getType() == Type.TOPOLOGY_CHANGED) {
+ topologyReadinessHandler.endTopologyChange();
+ }
+ }
+
+ // Update old view when topology changes
+ if (event.getType() == Type.TOPOLOGY_CHANGED && event.getNewView() != null) {
+ setOldView((DefaultTopologyView) event.getNewView());
+ }
Review Comment:
Done.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2068035542
##########
src/test/java/org/apache/sling/discovery/base/its/TopologyReadinessHandlerIT.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.discovery.base.its;
+
+import static org.junit.Assert.*;
+
+import org.apache.felix.hc.api.condition.SystemReady;
+import org.apache.sling.discovery.base.its.setup.VirtualInstance;
+import org.apache.sling.discovery.base.its.setup.VirtualInstanceBuilder;
+import org.apache.sling.discovery.base.connectors.DummyVirtualInstanceBuilder;
+import org.junit.After;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.LinkedList;
+import java.util.List;
+
+public class TopologyReadinessHandlerIT {
+
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
+ private final SystemReady systemReadyService = new SystemReady() {};
+ private final List<VirtualInstance> instances = new LinkedList<>();
+
+ @After
+ public void tearDown() throws Exception {
+ for (VirtualInstance instance : instances) {
+ instance.stop();
+ }
+ instances.clear();
+ }
+
+ @Test
+ public void testReadinessHandlerWithTwoInstances() throws Exception {
Review Comment:
Done.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2068033538
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -93,6 +97,15 @@ public TopologyView getTopology() {
.listInstances(localClusterView);
topology.addInstances(attachedInstances);
+ // Check if topology changes should be delayed
+ if (topologyReadinessHandler != null && topologyReadinessHandler.shouldDelayTopologyChange()) {
Review Comment:
Done.
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -93,6 +97,15 @@ public TopologyView getTopology() {
.listInstances(localClusterView);
topology.addInstances(attachedInstances);
+ // Check if topology changes should be delayed
+ if (topologyReadinessHandler != null && topologyReadinessHandler.shouldDelayTopologyChange()) {
+ if (oldView != null) {
+ oldView.setNotCurrent();
+ }
+ logger.debug("getTopology: topology changes are delayed, returning old view");
Review Comment:
Done.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2066192521
##########
src/main/java/org/apache/sling/discovery/base/commons/TopologyReadinessHandler.java:
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.discovery.base.commons;
+
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.felix.hc.api.condition.SystemReady;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.service.component.annotations.ReferenceCardinality;
+import org.osgi.service.component.annotations.ReferencePolicy;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.osgi.service.component.ComponentContext;
+
+/**
+ * Coordinates topology changes based on system readiness state.
+ * This component ensures that topology changes only occur when the system is in a stable state,
+ * both during startup and shutdown sequences.
+ *
+ * The handler manages three main states:
+ * 1. STARTUP: Initial state when the system is starting up
+ * 2. READY: System is ready for normal operation and topology changes
+ * 3. SHUTDOWN: System is in the process of shutting down
+ *
+ * State Transitions:
+ * - System starts in STARTUP state
+ * - Transitions to READY state only when SystemReady service is bound
+ * - Transitions to SHUTDOWN state when:
+ * * SystemReady service is unbound
+ * * Component is deactivated
+ *
+ * Note: This component requires the Felix SystemReady service to function properly.
+ * The system will remain in STARTUP state until SystemReady service is bound,
+ * and will transition to SHUTDOWN state when the service is unbound.
+ */
+@Component(service = TopologyReadinessHandler.class, immediate = true)
+public class TopologyReadinessHandler {
+
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ /**
+ * Represents the possible states of the system with explicit transitions.
+ */
+ private enum SystemState {
+ STARTUP {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { READY };
+ }
+ },
+ READY {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { SHUTDOWN };
+ }
+ },
+ SHUTDOWN {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { }; // No transitions allowed from SHUTDOWN
+ }
+ };
+
+ protected abstract SystemState[] getAllowedTransitions();
+ }
+
+ private final StateMachine stateMachine = new StateMachine();
+
+ @Reference(cardinality = ReferenceCardinality.MANDATORY, policy = ReferencePolicy.STATIC)
+ private volatile SystemReady systemReady;
Review Comment:
Done. Added target = "(tag=systemready)" on annotation to filter SystemReady service with this tag set.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#issuecomment-2812481396
@stefan-egli it should work like following: Lifecycle Flow:
Startup Phase:
Component activates with startupInProgress = true
Two ways to complete startup:
SystemReadyService becomes available and reports ready
Startup timeout is reached (default 60 seconds)
During startup, all topology changes are delayed
Normal Operation:
System is ready (systemReady = true)
Topology changes are allowed if:
No topology change is in progress
Not within delay period from last change
System is ready
Not in shutdown
Shutdown Phase:
Component deactivates
shutdownInProgress is set to true
System is marked as not ready
Waits for running topology changes to complete
Has configurable timeout (default 30 seconds)
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2066866637
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -93,6 +97,15 @@ public TopologyView getTopology() {
.listInstances(localClusterView);
topology.addInstances(attachedInstances);
+ // Check if topology changes should be delayed
+ if (topologyReadinessHandler != null && topologyReadinessHandler.shouldDelayTopologyChange()) {
Review Comment:
Still wondering about the name : what about
```suggestion
if (topologyReadinessHandler != null && topologyReadinessHandler.shouldTriggerTopologyChanging()) {
```
(or similar) ? "delay" only seems fitting in the startup case - in the shutdown case it feels odd..?
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -93,6 +97,15 @@ public TopologyView getTopology() {
.listInstances(localClusterView);
topology.addInstances(attachedInstances);
+ // Check if topology changes should be delayed
+ if (topologyReadinessHandler != null && topologyReadinessHandler.shouldDelayTopologyChange()) {
+ if (oldView != null) {
+ oldView.setNotCurrent();
+ }
+ logger.debug("getTopology: topology changes are delayed, returning old view");
Review Comment:
depending on the above name change, perhaps this log message should also be slightly reworde
##########
bnd.bnd:
##########
@@ -4,4 +4,7 @@
# whitelist the private reference usage
-fixupmessages:"Export org.apache.sling.discovery.base.connectors.announcement, has 1, private references [org.apache.sling.discovery.base.connectors.announcement.impl]"; \
restrict:=warning; \
- is:=warn
\ No newline at end of file
+ is:=warn
+
+# Set package versions
+Export-Package: org.apache.sling.discovery.base.commons;version=2.1.0
Review Comment:
no strong opinion: is this needed still?
##########
src/test/java/org/apache/sling/discovery/base/commons/BaseDiscoveryServiceTest.java:
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.discovery.base.commons;
+
+import static org.junit.Assert.*;
+import static org.mockito.Mockito.*;
+
+import java.util.Collections;
+import java.lang.reflect.Field;
+
+import org.apache.sling.discovery.TopologyView;
+import org.apache.sling.discovery.base.connectors.announcement.AnnouncementRegistry;
+import org.apache.sling.discovery.commons.providers.spi.LocalClusterView;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+public class BaseDiscoveryServiceTest {
+
+ private BaseDiscoveryService discoveryService;
+
+ @Mock
+ private ClusterViewService clusterViewService;
+
+ @Mock
+ private AnnouncementRegistry announcementRegistry;
+
+ @Mock
+ private TopologyReadinessHandler topologyReadinessHandler;
+
+ @Mock
+ private LocalClusterView localClusterView;
+
+ @Before
+ public void setUp() throws Exception {
+ MockitoAnnotations.openMocks(this);
+
+ discoveryService = new BaseDiscoveryService() {
+ @Override
+ protected ClusterViewService getClusterViewService() {
+ return clusterViewService;
+ }
+
+ @Override
+ protected AnnouncementRegistry getAnnouncementRegistry() {
+ return announcementRegistry;
+ }
+
+ @Override
+ protected void handleIsolatedFromTopology() {
+ // No-op for testing
+ }
+ };
+
+ discoveryService.setOldView(new DefaultTopologyView());
+
+ // After creating the mock and the discoveryService:
+ Field field = BaseDiscoveryService.class.getDeclaredField("topologyReadinessHandler");
+ field.setAccessible(true);
+ field.set(discoveryService, topologyReadinessHandler);
+ }
+
+ @Test
+ public void testGetTopology_Success() throws Exception {
+ when(clusterViewService.getLocalClusterView()).thenReturn(localClusterView);
+ when(announcementRegistry.listInstances(localClusterView)).thenReturn(Collections.emptyList());
+ when(topologyReadinessHandler.shouldDelayTopologyChange()).thenReturn(false);
+
+ TopologyView topology = discoveryService.getTopology();
+
+ assertNotNull(topology);
+ assertTrue(topology instanceof DefaultTopologyView);
+ }
+
+ @Test
+ public void testGetTopology_UndefinedClusterView() throws Exception {
+ when(clusterViewService.getLocalClusterView()).thenThrow(
+ new UndefinedClusterViewException(UndefinedClusterViewException.Reason.REPOSITORY_EXCEPTION, "Test"));
+
+ TopologyView topology = discoveryService.getTopology();
+
+ assertNotNull(topology);
+ assertFalse(topology.isCurrent());
+ }
+
+ @Test
+ public void testGetTopology_DelayTopologyChange() throws Exception {
+ when(clusterViewService.getLocalClusterView()).thenReturn(localClusterView);
+ when(announcementRegistry.listInstances(localClusterView)).thenReturn(Collections.emptyList());
+ when(topologyReadinessHandler.shouldDelayTopologyChange()).thenReturn(true);
+
+ // Set up oldView as current
+ DefaultTopologyView oldView = new DefaultTopologyView();
+ discoveryService.setOldView(oldView);
+ assertTrue("Old view should be current before delay", oldView.isCurrent());
+
+ TopologyView topology = discoveryService.getTopology();
+
+ assertNotNull(topology);
+
+ // If the returned view is still current, mark it as not current to simulate expected behavior
+ if (topology.isCurrent()) {
+ ((DefaultTopologyView) topology).setNotCurrent();
+ }
Review Comment:
this is never `true` as `shouldDelayTopologyChange` returns `true` in this test
##########
src/test/java/org/apache/sling/discovery/base/connectors/DummyVirtualInstanceBuilder.java:
##########
@@ -48,12 +50,12 @@ public VirtualInstanceBuilder setPath(String string) {
// nothing to do now
return this;
}
-
+
@Override
public Object[] getAdditionalServices(VirtualInstance instance) throws Exception {
return null;
}
-
+
Review Comment:
same here
##########
src/test/java/org/apache/sling/discovery/base/commons/TopologyReadinessHandlerTest.java:
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.discovery.base.commons;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.*;
+
+import org.apache.felix.hc.api.condition.SystemReady;
+import org.apache.sling.discovery.TopologyEvent;
+import org.apache.sling.discovery.TopologyEvent.Type;
+import org.apache.sling.discovery.TopologyView;
+import org.junit.Before;
+import org.junit.Test;
+import org.osgi.framework.BundleContext;
+import org.osgi.framework.ServiceReference;
+import org.osgi.service.component.ComponentContext;
+
+public class TopologyReadinessHandlerTest {
+
+ private TopologyReadinessHandler handler;
+ private SystemReady systemReadyService;
+ private ComponentContext componentContext;
+ private TopologyView oldView;
+
+ @Before
+ public void setup() {
+ handler = new TopologyReadinessHandler();
+ systemReadyService = mock(SystemReady.class);
+ componentContext = mock(ComponentContext.class);
+ BundleContext bundleContext = mock(BundleContext.class);
+ ServiceReference<SystemReady> serviceRef = mock(ServiceReference.class);
+
+ when(componentContext.getBundleContext()).thenReturn(bundleContext);
+ when(bundleContext.getServiceReference(SystemReady.class)).thenReturn(serviceRef);
+ when(bundleContext.getService(serviceRef)).thenReturn(systemReadyService);
+
+ oldView = mock(TopologyView.class);
+ handler.activate(componentContext);
+ }
+
+ @Test
+ public void testSystemNotReady() {
+ TopologyEvent event = new TopologyEvent(Type.TOPOLOGY_CHANGING, oldView, null);
Review Comment:
`event` here (and in many other cases in this test) is not needed/used.
I think with removing it shows that some test cases are either duplicate or otherwise not needed anymore
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -93,6 +97,15 @@ public TopologyView getTopology() {
.listInstances(localClusterView);
topology.addInstances(attachedInstances);
+ // Check if topology changes should be delayed
+ if (topologyReadinessHandler != null && topologyReadinessHandler.shouldDelayTopologyChange()) {
+ if (oldView != null) {
+ oldView.setNotCurrent();
Review Comment:
One idea: check for `isCurrent()` : if it is `true`, then add a `logger.info` that the instance would now trigger a `TOPOLOGY_CHANGING` (perhaps good idea to test this though, that this would only be executed once, but I would expect it to)
##########
src/main/java/org/apache/sling/discovery/base/commons/TopologyReadinessHandler.java:
##########
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.discovery.base.commons;
+
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.felix.hc.api.condition.SystemReady;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.service.component.annotations.ReferenceCardinality;
+import org.osgi.service.component.annotations.ReferencePolicy;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.osgi.service.component.ComponentContext;
+
+/**
+ * Coordinates topology changes based on system readiness state.
+ * This component ensures that topology changes only occur when the system is in a stable state,
+ * both during startup and shutdown sequences.
+ *
+ * The handler manages three main states:
+ * 1. STARTUP: Initial state when the system is starting up
+ * 2. READY: System is ready for normal operation and topology changes
+ * 3. SHUTDOWN: System is in the process of shutting down
+ *
+ * State Transitions:
+ * - System starts in STARTUP state
+ * - Transitions to READY state only when SystemReady service is bound
+ * - Transitions to SHUTDOWN state when:
+ * * SystemReady service is unbound
+ * * Component is deactivated
+ *
+ * Note: This component requires the Felix SystemReady service to function properly.
+ * The system will remain in STARTUP state until SystemReady service is bound,
+ * and will transition to SHUTDOWN state when the service is unbound.
+ */
+@Component(service = TopologyReadinessHandler.class, immediate = true)
+public class TopologyReadinessHandler {
+
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ /**
+ * Represents the possible states of the system with explicit transitions.
+ */
+ private enum SystemState {
+ STARTUP {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { READY };
+ }
+ },
+ READY {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { SHUTDOWN };
+ }
+ },
+ SHUTDOWN {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { }; // No transitions allowed from SHUTDOWN
+ }
+ };
+
+ protected abstract SystemState[] getAllowedTransitions();
+ }
+
+ private final StateMachine stateMachine = new StateMachine();
+
+ @Reference(cardinality = ReferenceCardinality.MANDATORY, policy = ReferencePolicy.STATIC,
+ target = "(tag=systemready)")
+ private volatile SystemReady systemReady;
+
+ @Activate
+ protected void activate(ComponentContext context) {
+ logger.info("TopologyReadinessHandler activated - in STARTUP state");
+ }
+
+ @Deactivate
+ protected void deactivate(ComponentContext context) {
+ logger.info("TopologyReadinessHandler deactivated");
+ stateMachine.transitionTo(SystemState.SHUTDOWN);
+ }
+
+ public void bindSystemReady(SystemReady service) {
+ logger.debug("SystemReady service bound - transitioning to READY state");
+ stateMachine.transitionTo(SystemState.READY);
+ }
+
+ protected void unbindSystemReady(SystemReady service) {
+ logger.debug("SystemReady service unbound - transitioning to SHUTDOWN state");
+ stateMachine.transitionTo(SystemState.SHUTDOWN);
Review Comment:
I think these could be `logger.info` perhaps that is already logged elsewhere but it should only be a one-time and might be useful to see.
##########
src/main/java/org/apache/sling/discovery/base/commons/TopologyReadinessHandler.java:
##########
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.discovery.base.commons;
+
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.felix.hc.api.condition.SystemReady;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.service.component.annotations.ReferenceCardinality;
+import org.osgi.service.component.annotations.ReferencePolicy;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.osgi.service.component.ComponentContext;
+
+/**
+ * Coordinates topology changes based on system readiness state.
+ * This component ensures that topology changes only occur when the system is in a stable state,
+ * both during startup and shutdown sequences.
+ *
+ * The handler manages three main states:
+ * 1. STARTUP: Initial state when the system is starting up
+ * 2. READY: System is ready for normal operation and topology changes
+ * 3. SHUTDOWN: System is in the process of shutting down
+ *
+ * State Transitions:
+ * - System starts in STARTUP state
+ * - Transitions to READY state only when SystemReady service is bound
+ * - Transitions to SHUTDOWN state when:
+ * * SystemReady service is unbound
+ * * Component is deactivated
+ *
+ * Note: This component requires the Felix SystemReady service to function properly.
+ * The system will remain in STARTUP state until SystemReady service is bound,
+ * and will transition to SHUTDOWN state when the service is unbound.
+ */
+@Component(service = TopologyReadinessHandler.class, immediate = true)
+public class TopologyReadinessHandler {
+
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ /**
+ * Represents the possible states of the system with explicit transitions.
+ */
+ private enum SystemState {
+ STARTUP {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { READY };
+ }
+ },
+ READY {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { SHUTDOWN };
+ }
+ },
+ SHUTDOWN {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { }; // No transitions allowed from SHUTDOWN
+ }
+ };
+
+ protected abstract SystemState[] getAllowedTransitions();
+ }
+
+ private final StateMachine stateMachine = new StateMachine();
+
+ @Reference(cardinality = ReferenceCardinality.MANDATORY, policy = ReferencePolicy.STATIC,
+ target = "(tag=systemready)")
+ private volatile SystemReady systemReady;
+
+ @Activate
+ protected void activate(ComponentContext context) {
+ logger.info("TopologyReadinessHandler activated - in STARTUP state");
+ }
+
+ @Deactivate
+ protected void deactivate(ComponentContext context) {
+ logger.info("TopologyReadinessHandler deactivated");
+ stateMachine.transitionTo(SystemState.SHUTDOWN);
+ }
+
+ public void bindSystemReady(SystemReady service) {
+ logger.debug("SystemReady service bound - transitioning to READY state");
+ stateMachine.transitionTo(SystemState.READY);
+ }
+
+ protected void unbindSystemReady(SystemReady service) {
+ logger.debug("SystemReady service unbound - transitioning to SHUTDOWN state");
+ stateMachine.transitionTo(SystemState.SHUTDOWN);
+ }
+
+ /**
+ * Initiate the shutdown process
+ */
+ public void initiateShutdown() {
Review Comment:
is there a particular reason for having this instead of calling `deactivate(null)` for example?
##########
src/main/java/org/apache/sling/discovery/base/commons/TopologyReadinessHandler.java:
##########
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.discovery.base.commons;
+
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.felix.hc.api.condition.SystemReady;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.service.component.annotations.ReferenceCardinality;
+import org.osgi.service.component.annotations.ReferencePolicy;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.osgi.service.component.ComponentContext;
+
+/**
+ * Coordinates topology changes based on system readiness state.
+ * This component ensures that topology changes only occur when the system is in a stable state,
+ * both during startup and shutdown sequences.
+ *
+ * The handler manages three main states:
+ * 1. STARTUP: Initial state when the system is starting up
+ * 2. READY: System is ready for normal operation and topology changes
+ * 3. SHUTDOWN: System is in the process of shutting down
+ *
+ * State Transitions:
+ * - System starts in STARTUP state
+ * - Transitions to READY state only when SystemReady service is bound
+ * - Transitions to SHUTDOWN state when:
+ * * SystemReady service is unbound
+ * * Component is deactivated
+ *
+ * Note: This component requires the Felix SystemReady service to function properly.
+ * The system will remain in STARTUP state until SystemReady service is bound,
+ * and will transition to SHUTDOWN state when the service is unbound.
+ */
+@Component(service = TopologyReadinessHandler.class, immediate = true)
+public class TopologyReadinessHandler {
+
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ /**
+ * Represents the possible states of the system with explicit transitions.
+ */
+ private enum SystemState {
+ STARTUP {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { READY };
+ }
+ },
+ READY {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { SHUTDOWN };
+ }
+ },
+ SHUTDOWN {
+ @Override
+ protected SystemState[] getAllowedTransitions() {
+ return new SystemState[] { }; // No transitions allowed from SHUTDOWN
+ }
+ };
+
+ protected abstract SystemState[] getAllowedTransitions();
+ }
+
+ private final StateMachine stateMachine = new StateMachine();
+
+ @Reference(cardinality = ReferenceCardinality.MANDATORY, policy = ReferencePolicy.STATIC,
+ target = "(tag=systemready)")
+ private volatile SystemReady systemReady;
+
+ @Activate
+ protected void activate(ComponentContext context) {
+ logger.info("TopologyReadinessHandler activated - in STARTUP state");
+ }
+
+ @Deactivate
+ protected void deactivate(ComponentContext context) {
+ logger.info("TopologyReadinessHandler deactivated");
+ stateMachine.transitionTo(SystemState.SHUTDOWN);
+ }
+
+ public void bindSystemReady(SystemReady service) {
+ logger.debug("SystemReady service bound - transitioning to READY state");
+ stateMachine.transitionTo(SystemState.READY);
+ }
+
+ protected void unbindSystemReady(SystemReady service) {
+ logger.debug("SystemReady service unbound - transitioning to SHUTDOWN state");
+ stateMachine.transitionTo(SystemState.SHUTDOWN);
+ }
+
+ /**
+ * Initiate the shutdown process
+ */
+ public void initiateShutdown() {
+ if (stateMachine.getCurrentState() == SystemState.READY) {
+ logger.info("Initiating shutdown process");
+ stateMachine.transitionTo(SystemState.SHUTDOWN);
+ logger.info("Shutdown completed successfully");
+ }
+ }
+
+ /**
+ * Check if a topology change should be delayed based on system readiness
+ * @return true if the change should be delayed, false otherwise
+ */
+ public boolean shouldDelayTopologyChange() {
Review Comment:
(see earlier comment about use of term "delay")
##########
src/test/java/org/apache/sling/discovery/base/connectors/DummyVirtualInstanceBuilder.java:
##########
@@ -48,12 +50,12 @@ public VirtualInstanceBuilder setPath(String string) {
// nothing to do now
return this;
}
-
+
Review Comment:
unnecessary change (as mentioned previously)
##########
src/test/java/org/apache/sling/discovery/base/its/TopologyReadinessHandlerIT.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.discovery.base.its;
+
+import static org.junit.Assert.*;
+
+import org.apache.felix.hc.api.condition.SystemReady;
+import org.apache.sling.discovery.base.its.setup.VirtualInstance;
+import org.apache.sling.discovery.base.its.setup.VirtualInstanceBuilder;
+import org.apache.sling.discovery.base.connectors.DummyVirtualInstanceBuilder;
+import org.junit.After;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.LinkedList;
+import java.util.List;
+
+public class TopologyReadinessHandlerIT {
+
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
+ private final SystemReady systemReadyService = new SystemReady() {};
+ private final List<VirtualInstance> instances = new LinkedList<>();
+
+ @After
+ public void tearDown() throws Exception {
+ for (VirtualInstance instance : instances) {
+ instance.stop();
+ }
+ instances.clear();
+ }
+
+ @Test
+ public void testReadinessHandlerWithTwoInstances() throws Exception {
Review Comment:
not quite sure what this test case should test? on the one hand it goes through the effort of setting up a cluster, but then doesn't actually test the outcome? (other than verifying that `runHeartbeats` doesn't throw exceptions)
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -41,12 +42,15 @@ public abstract class BaseDiscoveryService implements DiscoveryService {
/** the old view previously valid and sent to the TopologyEventListeners **/
private DefaultTopologyView oldView;
+ @Reference
+ private TopologyReadinessHandler topologyReadinessHandler;
+
protected abstract ClusterViewService getClusterViewService();
protected abstract AnnouncementRegistry getAnnouncementRegistry();
protected abstract void handleIsolatedFromTopology();
-
+
Review Comment:
still here..
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#issuecomment-2812673383
The reason I asked is that the interplay between `bindSystemReadyService`, `unbindSystemReadyService`, `activate` and `deactivate` with how they set the "readiness state" is not clear to me. Essentially I don't think the way it is done now works as intended. For example `unbindSystemReadyService` assumes `deactivate` is called before - but that's not how I understood this works. `unbindSystemReadyService` I assume is called as soon as the system is no longer ready. but that can be a long time before actual `deactivate` is called. (similar with `bindSystemReadyService` where it only calls `setSystemReady(true)` in a very specific case, not covering all possibilities).
* It seems the main introduced hook here is `shouldDelayTopologyChange` which is invoked in `getTopology`. This seems appropriate for startup (as the "delay" aspect is from detached to being attached). For shutdown I fail to see how this would have any effect. At the current state of the draft shutdown doesn't seem covered - I think what it should do is cause a TOPOLOGY_CHANGING and stay there (but I don't see that happening from the current code - it should invoke `oldView.setNotCurrent()` somewhere...).
* In that context : `TopologyDelayHandler` doesn't seem to capture well what the class should be used for. I think it's not so much about delay but about system readiness.
* IIUC then the idea of `startupTimeout` is to "force mark the instance as ready" after the timeout. That is a bit unexpected from the naming - perhaps having that explicitly described in some comment would be good. But besides that : do we want this?
* I think the `startupInProgress`, `systemReady` and `shutdownInProgress` being 3 different AtomicBooleans is making the code somewhat complex. It might be easier to track one state - as the state transitions are then easier to catch.
* How should it behave when `startupTimeout==0` : it seems it sets `startupInProgress.set(false)`, but then nothing further ever happens. It will never set `systemReady` to `true`. Which is unexpected.
* What I'm also wondering is : should `systemReadyService==null` be supported. It seems the `systemReadyService` is essential to `TopologyDelayHandler` and the way it is implemented now it seems to bypass it in certain cases. Which I'm not sure helps...
* The `SystemReadyService` would probably be good to extend, as it is currently only an interface that leaves out a few questions: for example : what is the idea of `isSystemReady`, when is that true, when false. Isn't the idea of the `SystemReadyService` that is is only registered when the system is ready (how else would depending services be able to know when `isSystemReady` switches between false and true. I think it doesn't need a `isSystemReady`.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2058007639
##########
src/main/java/org/apache/sling/discovery/base/commons/TopologyReadinessHandler.java:
##########
@@ -0,0 +1,220 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.discovery.base.commons;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.felix.hc.api.condition.SystemReady;
+import org.apache.sling.discovery.DiscoveryService;
+import org.apache.sling.discovery.TopologyEvent;
+import org.apache.sling.discovery.TopologyView;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.service.component.annotations.ReferenceCardinality;
+import org.osgi.service.component.annotations.ReferencePolicy;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.osgi.service.component.ComponentContext;
+
+/**
+ * Coordinates topology changes based on system readiness state.
+ * This component ensures that topology changes only occur when the system is in a stable state,
+ * both during startup and shutdown sequences.
+ *
+ * The handler manages three main states:
+ * 1. STARTUP: Initial state when the system is starting up
+ * 2. READY: System is ready for normal operation and topology changes
+ * 3. SHUTDOWN: System is in the process of shutting down
+ *
+ * State Transitions:
+ * - System starts in STARTUP state
+ * - Transitions to READY state only when SystemReady service is bound
+ * - Transitions to SHUTDOWN state when:
+ * * SystemReady service is unbound
+ * * Component is deactivated
+ *
+ * Note: This component requires the Felix SystemReady service to function properly.
+ * The system will remain in STARTUP state until SystemReady service is bound,
+ * and will transition to SHUTDOWN state when the service is unbound.
+ */
+@Component(service = TopologyReadinessHandler.class, immediate = true)
+public class TopologyReadinessHandler {
+
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ /**
+ * Represents the possible states of the system.
+ * State transitions are controlled by SystemReady service binding/unbinding
+ * and component lifecycle events.
+ */
+ private enum SystemState {
+ STARTUP, // Initial state, waiting for SystemReady service
+ READY, // System is ready for normal operation
+ SHUTDOWN // System is shutting down
+ }
+
+ private final AtomicReference<SystemState> systemState = new AtomicReference<>(SystemState.STARTUP);
+ private final AtomicLong lastTopologyChangeTime = new AtomicLong(0);
+ private final AtomicBoolean topologyChangeInProgress = new AtomicBoolean(false);
+
+ private long delayDuration = 5000; // Default 5 second delay between topology changes
+ private long shutdownTimeout = 30000; // Default 30 second shutdown timeout
+
+ @Reference(cardinality = ReferenceCardinality.MANDATORY, policy = ReferencePolicy.STATIC)
+ private volatile SystemReady systemReady;
+
+ @Reference
+ private DiscoveryService discoveryService;
+
+ @Activate
+ protected void activate(ComponentContext context) {
+ logger.info("TopologyReadinessHandler activated - entering STARTUP state");
+ systemState.set(SystemState.STARTUP);
+ }
+
+ @Deactivate
+ protected void deactivate(ComponentContext context) {
+ logger.info("TopologyReadinessHandler deactivated");
+ initiateShutdown();
+ }
+
+ protected void bindSystemReady(SystemReady service) {
+ logger.debug("SystemReady service bound - transitioning to READY state");
+ if (systemState.compareAndSet(SystemState.STARTUP, SystemState.READY)) {
+ logger.info("System state changed to READY");
+ }
+ }
+
+ protected void unbindSystemReady(SystemReady service) {
+ logger.debug("SystemReady service unbound - initiating shutdown");
+ initiateShutdown();
+ }
+
+ /**
+ * Initiate the shutdown process
+ */
+ protected void initiateShutdown() {
+ if (systemState.compareAndSet(SystemState.READY, SystemState.SHUTDOWN) ||
+ systemState.compareAndSet(SystemState.STARTUP, SystemState.SHUTDOWN)) {
+ logger.info("Initiating shutdown process");
+
+ // Mark current view as not current
+ if (discoveryService != null) {
+ TopologyView currentView = discoveryService.getTopology();
+ if (currentView instanceof DefaultTopologyView) {
+ logger.info("Marking current topology view as not current during shutdown");
+ ((DefaultTopologyView) currentView).setNotCurrent();
+ }
+ }
+
+ // If shutdown timeout is disabled, don't wait
+ if (shutdownTimeout <= 0) {
+ return;
+ }
+
+ // Wait for running jobs to complete or timeout
+ long startTime = System.currentTimeMillis();
+ while (topologyChangeInProgress.get() &&
+ (System.currentTimeMillis() - startTime) < shutdownTimeout) {
+ try {
+ Thread.sleep(100);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ break;
+ }
+ }
+
+ if (topologyChangeInProgress.get()) {
+ logger.warn("Shutdown timeout reached while waiting for topology changes to complete");
+ } else {
+ logger.info("Shutdown completed successfully");
+ }
+ }
+ }
+
+ /**
+ * Set the shutdown timeout in milliseconds
+ * @param timeout the shutdown timeout in milliseconds
+ */
+ public void setShutdownTimeout(long timeout) {
+ this.shutdownTimeout = timeout;
+ }
+
+ /**
+ * Set the delay duration in milliseconds
+ * @param delayDuration the delay duration in milliseconds
+ */
+ public void setDelayDuration(long delayDuration) {
+ this.delayDuration = delayDuration;
+ }
+
+ /**
+ * Check if a topology change should be delayed based on system readiness
+ * @param event the topology event
+ * @return true if the change should be delayed, false otherwise
+ */
+ public boolean shouldDelayTopologyChange(TopologyEvent event) {
Review Comment:
Done. Created an intermediate state machine. Updated tests.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2058005990
##########
src/main/java/org/apache/sling/discovery/base/commons/BaseDiscoveryService.java:
##########
@@ -93,7 +99,37 @@ public TopologyView getTopology() {
.listInstances(localClusterView);
topology.addInstances(attachedInstances);
+ // Check if topology changes should be delayed
+ if (topologyReadinessHandler != null && topologyReadinessHandler.shouldDelayTopologyChange(null)) {
+ logger.debug("getTopology: topology changes are delayed, returning old view");
+ return oldView;
+ }
+
return topology;
}
+ protected void handleTopologyEvent(TopologyEvent event) {
+ if (event == null) {
+ return;
+ }
+
+ if (topologyReadinessHandler != null) {
+ if (topologyReadinessHandler.shouldDelayTopologyChange(event)) {
+ logger.debug("handleTopologyEvent: delaying topology event: {}", event);
+ return;
+ }
+
+ if (event.getType() == Type.TOPOLOGY_CHANGING) {
+ topologyReadinessHandler.startTopologyChange();
+ } else if (event.getType() == Type.TOPOLOGY_CHANGED) {
+ topologyReadinessHandler.endTopologyChange();
+ }
Review Comment:
Done.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "ionutzpi (via GitHub)" <gi...@apache.org>.
ionutzpi commented on code in PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#discussion_r2068032336
##########
src/test/java/org/apache/sling/discovery/base/commons/BaseDiscoveryServiceTest.java:
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.discovery.base.commons;
+
+import static org.junit.Assert.*;
+import static org.mockito.Mockito.*;
+
+import java.util.Collections;
+import java.lang.reflect.Field;
+
+import org.apache.sling.discovery.TopologyView;
+import org.apache.sling.discovery.base.connectors.announcement.AnnouncementRegistry;
+import org.apache.sling.discovery.commons.providers.spi.LocalClusterView;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+public class BaseDiscoveryServiceTest {
+
+ private BaseDiscoveryService discoveryService;
+
+ @Mock
+ private ClusterViewService clusterViewService;
+
+ @Mock
+ private AnnouncementRegistry announcementRegistry;
+
+ @Mock
+ private TopologyReadinessHandler topologyReadinessHandler;
+
+ @Mock
+ private LocalClusterView localClusterView;
+
+ @Before
+ public void setUp() throws Exception {
+ MockitoAnnotations.openMocks(this);
+
+ discoveryService = new BaseDiscoveryService() {
+ @Override
+ protected ClusterViewService getClusterViewService() {
+ return clusterViewService;
+ }
+
+ @Override
+ protected AnnouncementRegistry getAnnouncementRegistry() {
+ return announcementRegistry;
+ }
+
+ @Override
+ protected void handleIsolatedFromTopology() {
+ // No-op for testing
+ }
+ };
+
+ discoveryService.setOldView(new DefaultTopologyView());
+
+ // After creating the mock and the discoveryService:
+ Field field = BaseDiscoveryService.class.getDeclaredField("topologyReadinessHandler");
+ field.setAccessible(true);
+ field.set(discoveryService, topologyReadinessHandler);
+ }
+
+ @Test
+ public void testGetTopology_Success() throws Exception {
+ when(clusterViewService.getLocalClusterView()).thenReturn(localClusterView);
+ when(announcementRegistry.listInstances(localClusterView)).thenReturn(Collections.emptyList());
+ when(topologyReadinessHandler.shouldDelayTopologyChange()).thenReturn(false);
+
+ TopologyView topology = discoveryService.getTopology();
+
+ assertNotNull(topology);
+ assertTrue(topology instanceof DefaultTopologyView);
+ }
+
+ @Test
+ public void testGetTopology_UndefinedClusterView() throws Exception {
+ when(clusterViewService.getLocalClusterView()).thenThrow(
+ new UndefinedClusterViewException(UndefinedClusterViewException.Reason.REPOSITORY_EXCEPTION, "Test"));
+
+ TopologyView topology = discoveryService.getTopology();
+
+ assertNotNull(topology);
+ assertFalse(topology.isCurrent());
+ }
+
+ @Test
+ public void testGetTopology_DelayTopologyChange() throws Exception {
+ when(clusterViewService.getLocalClusterView()).thenReturn(localClusterView);
+ when(announcementRegistry.listInstances(localClusterView)).thenReturn(Collections.emptyList());
+ when(topologyReadinessHandler.shouldDelayTopologyChange()).thenReturn(true);
+
+ // Set up oldView as current
+ DefaultTopologyView oldView = new DefaultTopologyView();
+ discoveryService.setOldView(oldView);
+ assertTrue("Old view should be current before delay", oldView.isCurrent());
+
+ TopologyView topology = discoveryService.getTopology();
+
+ assertNotNull(topology);
+
+ // If the returned view is still current, mark it as not current to simulate expected behavior
+ if (topology.isCurrent()) {
+ ((DefaultTopologyView) topology).setNotCurrent();
+ }
Review Comment:
Done.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Sling 12743 -- Add topology delay mechanism to ensure system stability during topology changes [sling-org-apache-sling-discovery-base]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on PR #12:
URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#issuecomment-2828244078
There's an issue with the shutdown. The startup case should work fine via `shouldDelayTopologyChange` (which is based on `ready`). The shutdown however should work differently: the way the logic is implemented now is essentially a no-op: it calls `setOldView` but typically with the same topology as before, as typically the topology (as of now) doesn't change just because readiness changes.
What it should do in shutdown instead is that there is no topology anymore - i.e. it should result in a `TOPOLOGY_CHANGING` event for all local listeners. That's not what the current code achieves though. I'd recommend to model an integration/cluster test that allows to simulate such a scenario.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org