From 31ef30f64847c74948434c536ef8af738573939b Mon Sep 17 00:00:00 2001 From: michaellx1057 Date: Wed, 16 Sep 2026 16:15:24 +0800 Subject: [PATCH 1/5] [Fix-18644][Registry] Reconcile HA roles using instance ownership --- docs/docs/en/guide/upgrade/incompatible.md | 4 + .../alert/registry/AlertRegistryClient.java | 13 +- .../alert/service/AlertHAServer.java | 1 + .../alert/AlertServerHATest.java | 116 ++++++ .../master/engine/MasterCoordinator.java | 1 + .../registry/api/ha/AbstractHAServer.java | 90 ++++- .../registry/api/ha/AbstractHAServerTest.java | 376 ++++++++++++++++++ 7 files changed, 578 insertions(+), 23 deletions(-) create mode 100644 dolphinscheduler-alert/dolphinscheduler-alert-server/src/test/java/org/apache/dolphinscheduler/alert/AlertServerHATest.java create mode 100644 dolphinscheduler-registry/dolphinscheduler-registry-api/src/test/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServerTest.java diff --git a/docs/docs/en/guide/upgrade/incompatible.md b/docs/docs/en/guide/upgrade/incompatible.md index ad118a1d6088..2311354f9c68 100644 --- a/docs/docs/en/guide/upgrade/incompatible.md +++ b/docs/docs/en/guide/upgrade/incompatible.md @@ -56,3 +56,7 @@ This document records the incompatible updates between each version. You need to * **Removed derived properties**: `cmdTypeIfComplement`, `complementData` (related to complement-data executions; use the detail API to obtain them) * To obtain any of these fields, use the detail API `GET /projects/{projectCode}/workflow-instances/{id}` instead, which continues to return the full `WorkflowInstance` object. ([#18444](https://github.com/apache/dolphinscheduler/pull/18444)) +## Next version + +* Master and Alert HA selector values now include a unique instance identifier after the server address. Treat these values as opaque ownership tokens, not network addresses. Registry paths and database schemas are unchanged. Upgrade all HA participants to obtain the ownership fix; unpatched participants retain their previous election behavior during a rolling upgrade or rollback. This change does not add fencing against delayed or missing registry notifications. Custom `AbstractHAServer` subclasses that override `close()` must call `super.close()` to stop further election callbacks after shutdown. + diff --git a/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/registry/AlertRegistryClient.java b/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/registry/AlertRegistryClient.java index 1b7839d81628..be91c96ebf26 100644 --- a/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/registry/AlertRegistryClient.java +++ b/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/registry/AlertRegistryClient.java @@ -22,6 +22,8 @@ import org.apache.dolphinscheduler.meter.metrics.MetricsProvider; import org.apache.dolphinscheduler.registry.api.RegistryClient; +import java.io.IOException; + import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Autowired; @@ -54,9 +56,16 @@ public void start() { } @Override - public void close() { + public void close() throws IOException { log.info("AlertRegistryClient closing..."); - alertHeartbeatTask.shutdown(); + try { + if (alertHeartbeatTask != null) { + alertHeartbeatTask.shutdown(); + } + } finally { + // Stop renewing the HA selector when the entire AlertServer shuts down. + registryClient.close(); + } log.info("AlertRegistryClient closed..."); } diff --git a/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/service/AlertHAServer.java b/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/service/AlertHAServer.java index 67382ac470a7..cc5a920cd1c1 100644 --- a/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/service/AlertHAServer.java +++ b/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/service/AlertHAServer.java @@ -42,6 +42,7 @@ public void start() { @Override public void close() { + super.close(); log.info("AlertHAServer shutdown..."); } } diff --git a/dolphinscheduler-alert/dolphinscheduler-alert-server/src/test/java/org/apache/dolphinscheduler/alert/AlertServerHATest.java b/dolphinscheduler-alert/dolphinscheduler-alert-server/src/test/java/org/apache/dolphinscheduler/alert/AlertServerHATest.java new file mode 100644 index 000000000000..199385d86e47 --- /dev/null +++ b/dolphinscheduler-alert/dolphinscheduler-alert-server/src/test/java/org/apache/dolphinscheduler/alert/AlertServerHATest.java @@ -0,0 +1,116 @@ +/* + * 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.dolphinscheduler.alert; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import org.apache.dolphinscheduler.alert.config.AlertConfig; +import org.apache.dolphinscheduler.alert.plugin.AlertPluginManager; +import org.apache.dolphinscheduler.alert.registry.AlertHeartbeatTask; +import org.apache.dolphinscheduler.alert.registry.AlertRegistryClient; +import org.apache.dolphinscheduler.alert.rpc.AlertRpcServer; +import org.apache.dolphinscheduler.alert.service.AlertBootstrapService; +import org.apache.dolphinscheduler.alert.service.AlertHAServer; +import org.apache.dolphinscheduler.common.lifecycle.ServerLifeCycleManager; +import org.apache.dolphinscheduler.common.thread.ThreadUtils; +import org.apache.dolphinscheduler.registry.api.Event; +import org.apache.dolphinscheduler.registry.api.Registry; +import org.apache.dolphinscheduler.registry.api.RegistryClient; +import org.apache.dolphinscheduler.registry.api.SubscribeListener; +import org.apache.dolphinscheduler.registry.api.enums.RegistryNodeType; + +import java.util.concurrent.atomic.AtomicReference; + +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.MockedStatic; +import org.springframework.test.util.ReflectionTestUtils; + +class AlertServerHATest { + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void testDemotionClosesAlertServerWithoutReactivatingClosedServices(boolean heartbeatShutdownFails) throws Exception { + Registry registry = mock(Registry.class); + AlertConfig config = mock(AlertConfig.class); + when(config.getAlertServerAddress()).thenReturn("alert-0:50052"); + AlertHAServer haServer = new AlertHAServer(registry, config); + AlertBootstrapService bootstrapService = mock(AlertBootstrapService.class); + AlertRpcServer rpcServer = mock(AlertRpcServer.class); + AlertRegistryClient registryClient = spy(new AlertRegistryClient()); + AlertHeartbeatTask heartbeatTask = mock(AlertHeartbeatTask.class); + if (heartbeatShutdownFails) { + doThrow(new IllegalStateException("heartbeat shutdown failed")).when(heartbeatTask).shutdown(); + } + ReflectionTestUtils.setField(registryClient, "registryClient", new RegistryClient(registry)); + ReflectionTestUtils.setField(registryClient, "alertHeartbeatTask", heartbeatTask); + doNothing().when(registryClient).start(); + AlertServer alertServer = new AlertServer(); + ReflectionTestUtils.setField(alertServer, "alertHAServer", haServer); + ReflectionTestUtils.setField(alertServer, "alertBootstrapService", bootstrapService); + ReflectionTestUtils.setField(alertServer, "alertRpcServer", rpcServer); + ReflectionTestUtils.setField(alertServer, "alertRegistryClient", registryClient); + ReflectionTestUtils.setField(alertServer, "alertPluginManager", mock(AlertPluginManager.class)); + + String selectorPath = RegistryNodeType.ALERT_HA_LEADER.getRegistryPath(); + AtomicReference subscriber = new AtomicReference<>(); + doAnswer(invocation -> { + subscriber.set(invocation.getArgument(1)); + return null; + }).when(registry).subscribe(eq(selectorPath), org.mockito.ArgumentMatchers.any()); + when(registry.acquireLock(anyString())).thenReturn(true); + try ( + MockedStatic lifecycle = mockStatic(ServerLifeCycleManager.class); + MockedStatic ignored = mockStatic(ThreadUtils.class)) { + lifecycle.when(ServerLifeCycleManager::toStopped).thenReturn(true); + alertServer.run(); + assertTrue(haServer.isActive()); + verify(bootstrapService).start(); + + // AlertServer's real demotion listener closes the whole server, rather than pausing it. + // A transient election error must not retry and restart these closed services. + when(registry.exists(selectorPath)) + .thenThrow(new IllegalStateException("temporary registry failure")) + .thenReturn(false); + Event removal = new Event(selectorPath, selectorPath, "", Event.Type.REMOVE); + subscriber.get().notify(removal); + subscriber.get().notify(removal); + haServer.start(); + assertFalse(haServer.isActive()); + verify(bootstrapService, times(1)).start(); + verify(bootstrapService, times(1)).close(); + verify(rpcServer).close(); + verify(registryClient).close(); + verify(heartbeatTask).shutdown(); + verify(registry).close(); + verify(registry, times(2)).acquireLock(anyString()); + } + } +} diff --git a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/MasterCoordinator.java b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/MasterCoordinator.java index eb2a9dda02cd..8a374acfa8dd 100644 --- a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/MasterCoordinator.java +++ b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/MasterCoordinator.java @@ -71,6 +71,7 @@ public void start() { @Override public void close() { + super.close(); taskGroupCoordinator.close(); log.info("MasterCoordinator shutdown..."); } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServer.java b/dolphinscheduler-registry/dolphinscheduler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServer.java index a79b14d99d5c..c1ac07adaffd 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServer.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServer.java @@ -25,6 +25,7 @@ import org.apache.dolphinscheduler.registry.api.SubscribeListener; import java.util.List; +import java.util.UUID; import lombok.extern.slf4j.Slf4j; @@ -39,7 +40,11 @@ public abstract class AbstractHAServer implements HAServer { private final String serverIdentify; - private ServerStatus serverStatus; + private final String electionIdentity; + + private volatile ServerStatus serverStatus; + + private volatile boolean closed; private final List serverStatusChangeListeners; @@ -51,24 +56,23 @@ public AbstractHAServer(final Registry registry, final String selectorPath, fina this.registry = registry; this.selectorPath = checkNotNull(selectorPath); this.serverIdentify = checkNotNull(serverIdentify); + // An address can be reused while the previous process still owns an ephemeral node. + this.electionIdentity = serverIdentify + "#" + UUID.randomUUID(); this.serverStatus = ServerStatus.STAND_BY; this.serverStatusChangeListeners = Lists.newArrayList(new DefaultServerStatusChangeListener()); } @Override public void start() { + if (closed) { + return; + } registry.subscribe(selectorPath, new SubscribeListener() { @Override public void notify(Event event) { if (Event.Type.REMOVE.equals(event.getType())) { - if (serverIdentify.equals(event.getEventData())) { - statusChange(ServerStatus.STAND_BY); - } else { - if (participateElection()) { - statusChange(ServerStatus.ACTIVE); - } - } + reconcileElection(); } } @@ -78,9 +82,24 @@ public SubscribeScope getSubscribeScope() { } }); - if (participateElection()) { + reconcileElection(); + } + + private synchronized void reconcileElection() { + if (closed) { + return; + } + // Serialize election and publication with callbacks, including callbacks during startup. + // REMOVE may be delayed or have no previous value, so consult current ownership instead. + boolean elected = participateElection(); + // A demotion listener may close the entire server (for example, AlertServer). + if (closed) { + return; + } + if (elected) { statusChange(ServerStatus.ACTIVE); } else { + statusChange(ServerStatus.STAND_BY); log.info("Server {} is standby", serverIdentify); } } @@ -91,25 +110,43 @@ public boolean isActive() { } @Override - public boolean participateElection() { + public synchronized boolean participateElection() { final String electionLock = selectorPath + "-lock"; // If meet exception during participate election, will retry. // This can avoid the situation that the server is not elected as leader due to network jitter. for (int i = 0; i < DEFAULT_MAX_RETRY_TIMES; i++) { + if (closed) { + return false; + } + boolean lockAcquired = false; try { try { - if (registry.acquireLock(electionLock)) { + lockAcquired = registry.acquireLock(electionLock); + if (lockAcquired) { + if (closed) { + return false; + } if (!registry.exists(selectorPath)) { - registry.put(selectorPath, serverIdentify, true); + if (closed) { + return false; + } + registry.put(selectorPath, electionIdentity, true); return true; } - return serverIdentify.equals(registry.get(selectorPath)); + return electionIdentity.equals(registry.get(selectorPath)); } return false; } finally { - registry.releaseLock(electionLock); + if (lockAcquired) { + registry.releaseLock(electionLock); + } } } catch (Exception e) { + // Do not keep coordinator services active while ownership cannot be verified. + statusChange(ServerStatus.STAND_BY); + if (closed) { + return false; + } log.error("Participate election error, meet an exception, will retry after {}ms", DEFAULT_RETRY_INTERVAL, e); ThreadUtils.sleep(DEFAULT_RETRY_INTERVAL); @@ -119,6 +156,16 @@ public boolean participateElection() { "Participate election failed after retry " + DEFAULT_MAX_RETRY_TIMES + " times"); } + @Override + public void close() { + // Publish shutdown before waiting for an in-flight election to release the monitor. + closed = true; + synchronized (this) { + // Consumers close their own services; notifying listeners here could recurse. + serverStatus = ServerStatus.STAND_BY; + } + } + @Override public void addServerStatusChangeListener(ServerStatusChangeListener listener) { serverStatusChangeListeners.add(listener); @@ -129,15 +176,16 @@ public ServerStatus getServerStatus() { return serverStatus; } - private void statusChange(ServerStatus targetStatus) { + private synchronized void statusChange(ServerStatus targetStatus) { + if (closed) { + return; + } final ServerStatus originStatus = serverStatus; serverStatus = targetStatus; - synchronized (this) { - try { - serverStatusChangeListeners.forEach(listener -> listener.change(originStatus, serverStatus)); - } catch (Exception ex) { - log.error("Trigger ServerStatusChangeListener from {} -> {} error", originStatus, targetStatus, ex); - } + try { + serverStatusChangeListeners.forEach(listener -> listener.change(originStatus, targetStatus)); + } catch (Exception ex) { + log.error("Trigger ServerStatusChangeListener from {} -> {} error", originStatus, targetStatus, ex); } } } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-api/src/test/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServerTest.java b/dolphinscheduler-registry/dolphinscheduler-registry-api/src/test/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServerTest.java new file mode 100644 index 000000000000..da599399377d --- /dev/null +++ b/dolphinscheduler-registry/dolphinscheduler-registry-api/src/test/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServerTest.java @@ -0,0 +1,376 @@ +/* + * 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.dolphinscheduler.registry.api.ha; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.CALLS_REAL_METHODS; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import org.apache.dolphinscheduler.common.thread.ThreadUtils; +import org.apache.dolphinscheduler.registry.api.Event; +import org.apache.dolphinscheduler.registry.api.Registry; +import org.apache.dolphinscheduler.registry.api.SubscribeListener; + +import java.lang.management.LockInfo; +import java.lang.management.ManagementFactory; +import java.lang.management.ThreadInfo; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; + +class AbstractHAServerTest { + + private static final String SELECTOR_PATH = "/coordinator"; + private static final String ELECTION_LOCK = SELECTOR_PATH + "-lock"; + private static final String ADDRESS = "master-0:5678"; + + private Registry registry; + private AtomicReference owner; + private AtomicReference subscriber; + private AbstractServerStatusChangeListener statusListener; + private AbstractHAServer server; + + @BeforeEach + void setUp() { + registry = mock(Registry.class); + owner = new AtomicReference<>(); + subscriber = new AtomicReference<>(); + statusListener = mock(AbstractServerStatusChangeListener.class, CALLS_REAL_METHODS); + server = newServer(); + server.addServerStatusChangeListener(statusListener); + when(registry.acquireLock(ELECTION_LOCK)).thenReturn(true); + when(registry.exists(SELECTOR_PATH)).thenAnswer(invocation -> owner.get() != null); + when(registry.get(SELECTOR_PATH)).thenAnswer(invocation -> owner.get()); + doAnswer(invocation -> { + owner.set(invocation.getArgument(1)); + return null; + }).when(registry).put(eq(SELECTOR_PATH), anyString(), eq(true)); + doAnswer(invocation -> { + subscriber.set(invocation.getArgument(1)); + return null; + }).when(registry).subscribe(eq(SELECTOR_PATH), org.mockito.ArgumentMatchers.any()); + } + + @Test + void testInitialLeaderAndFollower() { + server.start(); + assertTrue(server.isActive()); + verify(statusListener).changeToActive(); + assertEquals(SubscribeListener.SubscribeScope.PATH_ONLY, subscriber.get().getSubscribeScope()); + + AbstractHAServer follower = newServer("master-1:5678"); + AbstractServerStatusChangeListener followerListener = + mock(AbstractServerStatusChangeListener.class, CALLS_REAL_METHODS); + follower.addServerStatusChangeListener(followerListener); + follower.start(); + assertFalse(follower.isActive()); + verify(followerListener, never()).changeToActive(); + verify(registry, times(1)).put(eq(SELECTOR_PATH), anyString(), eq(true)); + } + + @Test + void testDoesNotAdoptPreviousProcessWithSameAddress() { + // A legacy selector may survive its process until its lease/session expires. + owner.set(ADDRESS); + server.start(); + assertFalse(server.isActive()); + verify(statusListener, never()).changeToActive(); + verify(registry, never()).put(eq(SELECTOR_PATH), anyString(), eq(true)); + + owner.set(null); + remove(ADDRESS); + assertTrue(server.isActive()); + assertNotEquals(ADDRESS, owner.get()); + verify(statusListener).changeToActive(); + } + + @Test + void testSameAddressInstancesHaveDifferentOwnership() { + server.start(); + String previousOwner = owner.get(); + AbstractHAServer replacement = newServer(); + AbstractServerStatusChangeListener replacementListener = + mock(AbstractServerStatusChangeListener.class, CALLS_REAL_METHODS); + replacement.addServerStatusChangeListener(replacementListener); + replacement.start(); + assertFalse(replacement.isActive()); + verify(replacementListener, never()).changeToActive(); + + owner.set(null); + remove(previousOwner); + assertTrue(replacement.isActive()); + assertNotEquals(previousOwner, owner.get()); + verify(replacementListener).changeToActive(); + } + + @Test + void testEmptyRemoveDemotesFormerLeaderWhenPeerOwnsSelector() { + server.start(); + owner.set("master-1:5678#peer-instance"); + remove(""); + assertFalse(server.isActive()); + verify(statusListener).changeToActive(); + verify(statusListener).changeToStandBy(); + } + + @Test + void testDelayedRemoveDoesNotRestartCurrentOwner() { + server.start(); + // The old notification arrives after this instance has already acquired the current key. + remove(owner.get()); + remove(""); + assertTrue(server.isActive()); + verify(statusListener, times(1)).changeToActive(); + verify(statusListener, never()).changeToStandBy(); + } + + @Test + void testOwnRemovalCanReelectWithoutAnotherPeer() { + server.start(); + String previousOwner = owner.get(); + owner.set(null); + remove(previousOwner); + assertTrue(server.isActive()); + verify(registry, times(2)).put(eq(SELECTOR_PATH), anyString(), eq(true)); + verify(statusListener, times(1)).changeToActive(); + verify(statusListener, never()).changeToStandBy(); + } + + @Test + void testAddAndUpdateDoNotTriggerElection() { + owner.set("master-1:5678#peer-instance"); + server.start(); + owner.set(null); + subscriber.get().notify(new Event(SELECTOR_PATH, SELECTOR_PATH, "", Event.Type.ADD)); + subscriber.get().notify(new Event(SELECTOR_PATH, SELECTOR_PATH, "", Event.Type.UPDATE)); + assertFalse(server.isActive()); + verify(registry, times(1)).acquireLock(ELECTION_LOCK); + verify(statusListener, never()).changeToActive(); + } + + @Test + void testUnacquiredLockIsNotReleased() { + when(registry.acquireLock(ELECTION_LOCK)).thenReturn(false); + server.start(); + assertFalse(server.isActive()); + verify(registry, never()).releaseLock(ELECTION_LOCK); + verify(statusListener, never()).changeToActive(); + } + + @Test + void testActiveServerDemotesWhenLockCannotBeAcquired() { + server.start(); + when(registry.acquireLock(ELECTION_LOCK)).thenReturn(false); + remove(""); + assertFalse(server.isActive()); + verify(statusListener).changeToStandBy(); + verify(registry, times(1)).releaseLock(ELECTION_LOCK); + } + + @Test + void testElectionErrorDemotesBeforeRetryAndReleasesAcquiredLock() { + server.start(); + when(registry.exists(SELECTOR_PATH)).thenThrow(new IllegalStateException("registry unavailable")); + try (MockedStatic threadUtils = mockStatic(ThreadUtils.class)) { + // Verify safety at each retry boundary, without spending real time on retry sleeps. + threadUtils.when(() -> ThreadUtils.sleep(5_000)).thenAnswer(invocation -> { + assertFalse(server.isActive()); + verify(statusListener, times(1)).changeToStandBy(); + return null; + }); + assertThrows(IllegalStateException.class, () -> remove("")); + } + assertFalse(server.isActive()); + verify(statusListener, times(1)).changeToStandBy(); + verify(registry, times(21)).releaseLock(ELECTION_LOCK); + } + + @Test + void testTransientElectionErrorStopsAndRestartsCoordinator() { + server.start(); + when(registry.exists(SELECTOR_PATH)) + .thenThrow(new IllegalStateException("temporary registry failure")) + .thenReturn(true); + try (MockedStatic threadUtils = mockStatic(ThreadUtils.class)) { + threadUtils.when(() -> ThreadUtils.sleep(5_000)).thenAnswer(invocation -> { + assertFalse(server.isActive()); + verify(statusListener).changeToStandBy(); + return null; + }); + remove(""); + } + assertTrue(server.isActive()); + verify(statusListener, times(2)).changeToActive(); + verify(statusListener).changeToStandBy(); + } + + @Test + void testAcquisitionErrorDoesNotReleaseUnacquiredLock() { + when(registry.acquireLock(ELECTION_LOCK)).thenThrow(new IllegalStateException("lock unavailable")); + try (MockedStatic ignored = mockStatic(ThreadUtils.class)) { + assertThrows(IllegalStateException.class, server::start); + } + assertFalse(server.isActive()); + verify(registry, never()).releaseLock(ELECTION_LOCK); + verify(statusListener, never()).changeToActive(); + } + + @Test + void testShutdownDuringLockAcquisitionDoesNotClaimSelector() { + when(registry.acquireLock(ELECTION_LOCK)).thenAnswer(invocation -> { + // Shutdown is requested before the blocking acquisition returns to the election. + server.close(); + return true; + }); + server.start(); + assertFalse(server.isActive()); + verify(registry, never()).put(eq(SELECTOR_PATH), anyString(), eq(true)); + verify(registry).releaseLock(ELECTION_LOCK); + verify(statusListener, never()).changeToActive(); + } + + @Test + void testTerminalDemotionListenerPreventsRetryAndReactivation() { + doAnswer(invocation -> { + server.close(); + return null; + }).when(statusListener).changeToStandBy(); + server.start(); + when(registry.exists(SELECTOR_PATH)) + .thenThrow(new IllegalStateException("temporary registry failure")) + .thenReturn(true); + try (MockedStatic threadUtils = mockStatic(ThreadUtils.class)) { + remove(""); + // AlertServer shuts down on demotion, unlike restartable Master coordinators. + threadUtils.verifyNoInteractions(); + } + remove(""); + assertFalse(server.isActive()); + verify(statusListener, times(1)).changeToActive(); + verify(statusListener, times(1)).changeToStandBy(); + verify(registry, times(2)).acquireLock(ELECTION_LOCK); + } + + @Test + void testClosePreventsRestartAndFurtherElection() { + server.start(); + server.close(); + remove(""); + server.start(); + assertFalse(server.participateElection()); + assertFalse(server.isActive()); + verify(statusListener, times(1)).changeToActive(); + verify(registry, times(1)).acquireLock(ELECTION_LOCK); + verify(registry, times(1)).subscribe(eq(SELECTOR_PATH), org.mockito.ArgumentMatchers.any()); + } + + @Test + void testRemoveCannotBeOverwrittenByEarlierStartupElection() throws Exception { + CountDownLatch startupElectionFinished = new CountDownLatch(1); + CountDownLatch allowStartupToReturn = new CountDownLatch(1); + CountDownLatch callbackStarted = new CountDownLatch(1); + AtomicBoolean firstRelease = new AtomicBoolean(true); + AtomicReference callbackThread = new AtomicReference<>(); + when(registry.releaseLock(ELECTION_LOCK)).thenAnswer(invocation -> { + if (firstRelease.getAndSet(false)) { + // Pause after the successful election but before startup publishes ACTIVE. + startupElectionFinished.countDown(); + assertTrue(allowStartupToReturn.await(5, TimeUnit.SECONDS)); + } + return true; + }); + ExecutorService executor = Executors.newFixedThreadPool(2); + try { + Future startup = executor.submit(server::start); + assertTrue(startupElectionFinished.await(5, TimeUnit.SECONDS)); + String previousOwner = owner.get(); + owner.set("master-1:5678#peer-instance"); + Future callback = executor.submit(() -> { + callbackThread.set(Thread.currentThread()); + callbackStarted.countDown(); + remove(previousOwner); + }); + assertTrue(callbackStarted.await(5, TimeUnit.SECONDS)); + // Wait for actual monitor contention (fixed code), or completion (old code). + // This forces the relevant ordering rather than relying on a sleep or scheduler luck. + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5); + while (!callback.isDone() && !isBlockedOnServer(callbackThread.get()) + && System.nanoTime() < deadline) { + Thread.yield(); + } + assertTrue(callback.isDone() || isBlockedOnServer(callbackThread.get())); + allowStartupToReturn.countDown(); + startup.get(5, TimeUnit.SECONDS); + callback.get(5, TimeUnit.SECONDS); + assertFalse(server.isActive()); + verify(statusListener).changeToActive(); + verify(statusListener).changeToStandBy(); + } finally { + allowStartupToReturn.countDown(); + executor.shutdownNow(); + assertTrue(executor.awaitTermination(5, TimeUnit.SECONDS)); + } + } + + private boolean isBlockedOnServer(Thread thread) { + ThreadInfo threadInfo = ManagementFactory.getThreadMXBean().getThreadInfo(thread.getId()); + if (threadInfo == null || threadInfo.getThreadState() != Thread.State.BLOCKED) { + return false; + } + LockInfo lockInfo = threadInfo.getLockInfo(); + return lockInfo != null && lockInfo.getIdentityHashCode() == System.identityHashCode(server); + } + + private void remove(String previousOwner) { + subscriber.get().notify(new Event(SELECTOR_PATH, SELECTOR_PATH, previousOwner, Event.Type.REMOVE)); + } + + private AbstractHAServer newServer() { + return newServer(ADDRESS); + } + + private AbstractHAServer newServer(String address) { + return new AbstractHAServer(registry, SELECTOR_PATH, address) { + + @Override + public void close() { + super.close(); + } + }; + } +} From 814b7e24b33790bb9b0857a8815fb016bdc040b2 Mon Sep 17 00:00:00 2001 From: michaellx1057 Date: Wed, 16 Sep 2026 17:16:14 +0800 Subject: [PATCH 2/5] [Fix-18644][Registry] Simplify instance identity following review --- docs/docs/en/guide/upgrade/incompatible.md | 4 ---- .../registry/api/ha/AbstractHAServer.java | 12 ++++------- .../registry/api/ha/AbstractHAServerTest.java | 21 +++++++++---------- 3 files changed, 14 insertions(+), 23 deletions(-) diff --git a/docs/docs/en/guide/upgrade/incompatible.md b/docs/docs/en/guide/upgrade/incompatible.md index 2311354f9c68..ad118a1d6088 100644 --- a/docs/docs/en/guide/upgrade/incompatible.md +++ b/docs/docs/en/guide/upgrade/incompatible.md @@ -56,7 +56,3 @@ This document records the incompatible updates between each version. You need to * **Removed derived properties**: `cmdTypeIfComplement`, `complementData` (related to complement-data executions; use the detail API to obtain them) * To obtain any of these fields, use the detail API `GET /projects/{projectCode}/workflow-instances/{id}` instead, which continues to return the full `WorkflowInstance` object. ([#18444](https://github.com/apache/dolphinscheduler/pull/18444)) -## Next version - -* Master and Alert HA selector values now include a unique instance identifier after the server address. Treat these values as opaque ownership tokens, not network addresses. Registry paths and database schemas are unchanged. Upgrade all HA participants to obtain the ownership fix; unpatched participants retain their previous election behavior during a rolling upgrade or rollback. This change does not add fencing against delayed or missing registry notifications. Custom `AbstractHAServer` subclasses that override `close()` must call `super.close()` to stop further election callbacks after shutdown. - diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServer.java b/dolphinscheduler-registry/dolphinscheduler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServer.java index c1ac07adaffd..a984eebaaeba 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServer.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServer.java @@ -25,7 +25,6 @@ import org.apache.dolphinscheduler.registry.api.SubscribeListener; import java.util.List; -import java.util.UUID; import lombok.extern.slf4j.Slf4j; @@ -40,8 +39,6 @@ public abstract class AbstractHAServer implements HAServer { private final String serverIdentify; - private final String electionIdentity; - private volatile ServerStatus serverStatus; private volatile boolean closed; @@ -55,9 +52,8 @@ public abstract class AbstractHAServer implements HAServer { public AbstractHAServer(final Registry registry, final String selectorPath, final String serverIdentify) { this.registry = registry; this.selectorPath = checkNotNull(selectorPath); - this.serverIdentify = checkNotNull(serverIdentify); - // An address can be reused while the previous process still owns an ephemeral node. - this.electionIdentity = serverIdentify + "#" + UUID.randomUUID(); + // Include the creation time to distinguish restarts at the same address. + this.serverIdentify = checkNotNull(serverIdentify) + "#" + System.currentTimeMillis(); this.serverStatus = ServerStatus.STAND_BY; this.serverStatusChangeListeners = Lists.newArrayList(new DefaultServerStatusChangeListener()); } @@ -130,10 +126,10 @@ public synchronized boolean participateElection() { if (closed) { return false; } - registry.put(selectorPath, electionIdentity, true); + registry.put(selectorPath, serverIdentify, true); return true; } - return electionIdentity.equals(registry.get(selectorPath)); + return serverIdentify.equals(registry.get(selectorPath)); } return false; } finally { diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-api/src/test/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServerTest.java b/dolphinscheduler-registry/dolphinscheduler-registry-api/src/test/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServerTest.java index da599399377d..cb7adb1ec95b 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-api/src/test/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServerTest.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-api/src/test/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServerTest.java @@ -120,22 +120,21 @@ void testDoesNotAdoptPreviousProcessWithSameAddress() { } @Test - void testSameAddressInstancesHaveDifferentOwnership() { + void testDoesNotAdoptPredecessorWithEarlierTimestamp() { + // Seed an earlier incarnation explicitly. Millisecond timestamps do not guarantee + // different identities for two same-address instances created in the same millisecond. + String previousOwner = ADDRESS + "#1"; + owner.set(previousOwner); server.start(); - String previousOwner = owner.get(); - AbstractHAServer replacement = newServer(); - AbstractServerStatusChangeListener replacementListener = - mock(AbstractServerStatusChangeListener.class, CALLS_REAL_METHODS); - replacement.addServerStatusChangeListener(replacementListener); - replacement.start(); - assertFalse(replacement.isActive()); - verify(replacementListener, never()).changeToActive(); + assertFalse(server.isActive()); + verify(statusListener, never()).changeToActive(); + verify(registry, never()).put(eq(SELECTOR_PATH), anyString(), eq(true)); owner.set(null); remove(previousOwner); - assertTrue(replacement.isActive()); + assertTrue(server.isActive()); assertNotEquals(previousOwner, owner.get()); - verify(replacementListener).changeToActive(); + verify(statusListener).changeToActive(); } @Test From cd9b51d9925b0ecf60c7104421dad0a0847ba28d Mon Sep 17 00:00:00 2001 From: michaellx1057 Date: Wed, 16 Sep 2026 18:32:14 +0800 Subject: [PATCH 3/5] [Fix-18644][Master] Prevent overlapping coordinator runs during restart --- .../master/engine/TaskGroupCoordinator.java | 131 +++++++++--- .../serial/WorkflowSerialCoordinator.java | 57 ++++-- .../engine/TaskGroupCoordinatorTest.java | 91 +++++++++ .../serial/WorkflowSerialCoordinatorTest.java | 193 ++++++++++++++++++ 4 files changed, 426 insertions(+), 46 deletions(-) diff --git a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/TaskGroupCoordinator.java b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/TaskGroupCoordinator.java index 3a6d81c24c6b..10a5fed0d58b 100644 --- a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/TaskGroupCoordinator.java +++ b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/TaskGroupCoordinator.java @@ -77,9 +77,10 @@ public class TaskGroupCoordinator implements ITaskGroupCoordinator, AutoCloseabl @Autowired private TransactionTemplate transactionTemplate; - private boolean flag = false; + // Desired lifecycle state; a canceled thread must finish before a requested restart. + private volatile boolean flag = false; - private Thread internalThread; + private CoordinatorThread internalThread; private static final int DEFAULT_LIMIT = 1000; @@ -88,35 +89,70 @@ public synchronized void start() { if (flag) { throw new IllegalStateException("TaskGroupCoordinator is already started"); } - if (internalThread != null) { - throw new IllegalStateException("InternalThread is already started"); - } flag = true; - internalThread = new BaseDaemonThread(this::doStart) { - }; - internalThread.setName("TaskGroupCoordinator-Thread"); + if (internalThread == null) { + startInternalThread(); + } + // A canceled run may still be inside JDBC. Its finally block starts the replacement. + } + + private void startInternalThread() { + internalThread = new CoordinatorThread(); internalThread.start(); log.info("TaskGroupCoordinator started..."); } + private final class CoordinatorThread extends BaseDaemonThread { + + private volatile boolean running = true; + + private CoordinatorThread() { + super("TaskGroupCoordinator-Thread"); + } + + @Override + public void run() { + try { + doStart(this); + } finally { + synchronized (TaskGroupCoordinator.this) { + internalThread = null; + if (flag) { + startInternalThread(); + } + } + } + } + } + @VisibleForTesting boolean isStarted() { return flag; } - private void doStart() { - // Sleep 1 minutes here to make sure the previous task group slot has been released. - // This step is not necessary, since the wakeup operation is idempotent, but we can avoid confusion warning. - ThreadUtils.sleep(TimeUnit.MINUTES.toMillis(1)); + private void doStart(CoordinatorThread run) { + pauseBeforeStart(); - while (flag) { + while (run.running) { try { final StopWatch taskGroupCoordinatorRoundCost = StopWatch.createStarted(); - amendTaskGroupUseSize(); - amendTaskGroupQueueStatus(); - dealWithForceStartTaskGroupQueue(); - dealWithWaitingTaskGroupQueue(); + if (!run.running) { + return; + } + amendTaskGroupUseSize(run); + if (!run.running) { + return; + } + amendTaskGroupQueueStatus(run); + if (!run.running) { + return; + } + dealWithForceStartTaskGroupQueue(run); + if (!run.running) { + return; + } + dealWithWaitingTaskGroupQueue(run); taskGroupCoordinatorRoundCost.stop(); log.debug("TaskGroupCoordinator round cost: {}/ms", taskGroupCoordinatorRoundCost.getTime()); @@ -124,24 +160,39 @@ private void doStart() { log.error("TaskGroupCoordinator error", e); } finally { // sleep 5s - ThreadUtils.sleep(Constants.SLEEP_TIME_MILLIS * 5); + if (run.running) { + ThreadUtils.sleep(Constants.SLEEP_TIME_MILLIS * 5); + } } } } + @VisibleForTesting + void pauseBeforeStart() { + // Sleep 1 minutes here to make sure the previous task group slot has been released. + // This step is not necessary, since the wakeup operation is idempotent, but we can avoid confusion warning. + ThreadUtils.sleep(TimeUnit.MINUTES.toMillis(1)); + } + /** * Make sure the TaskGroup useSize is equal to the TaskGroupQueue which status is {@link TaskGroupQueueStatus#ACQUIRE_SUCCESS} and forceStart is {@link org.apache.dolphinscheduler.common.enums.Flag#NO}. */ - private void amendTaskGroupUseSize() { + private void amendTaskGroupUseSize(CoordinatorThread run) { // The TaskGroup useSize should equal to the TaskGroupQueue which inQueue is YES and forceStart is NO List taskGroups = taskGroupDao.queryAllTaskGroups(); - if (CollectionUtils.isEmpty(taskGroups)) { + if (!run.running || CollectionUtils.isEmpty(taskGroups)) { return; } StopWatch taskGroupCoordinatorRoundTimeCost = StopWatch.createStarted(); for (TaskGroup taskGroup : taskGroups) { + if (!run.running) { + return; + } int actualUseSize = taskGroupQueueDao.countUsingTaskGroupQueueByGroupId(taskGroup.getId()); + if (!run.running) { + return; + } if (taskGroup.getUseSize() == actualUseSize) { continue; } @@ -157,17 +208,17 @@ private void amendTaskGroupUseSize() { /** * Clear the TaskGroupQueue when the related {@link TaskInstance} is not exist or status is finished. */ - private void amendTaskGroupQueueStatus() { + private void amendTaskGroupQueueStatus(CoordinatorThread run) { int minTaskGroupQueueId = -1; int limit = DEFAULT_LIMIT; StopWatch taskGroupCoordinatorRoundTimeCost = StopWatch.createStarted(); - while (true) { + while (run.running) { List taskGroupQueues = taskGroupQueueDao.queryInQueueTaskGroupQueue(minTaskGroupQueueId, limit); - if (CollectionUtils.isEmpty(taskGroupQueues)) { + if (!run.running || CollectionUtils.isEmpty(taskGroupQueues)) { break; } - amendTaskGroupQueueStatus(taskGroupQueues); + amendTaskGroupQueueStatus(taskGroupQueues, run); if (taskGroupQueues.size() < limit) { break; } @@ -179,7 +230,7 @@ private void amendTaskGroupQueueStatus() { /** * Clear the TaskGroupQueue when the related {@link TaskInstance} is not exist or status is finished. */ - private void amendTaskGroupQueueStatus(List taskGroupQueues) { + private void amendTaskGroupQueueStatus(List taskGroupQueues, CoordinatorThread run) { final List taskInstanceIds = taskGroupQueues.stream() .map(TaskGroupQueue::getTaskId) .collect(Collectors.toList()); @@ -188,6 +239,9 @@ private void amendTaskGroupQueueStatus(List taskGroupQueues) { .collect(Collectors.toMap(TaskInstance::getId, Function.identity())); for (TaskGroupQueue taskGroupQueue : taskGroupQueues) { + if (!run.running) { + return; + } int taskId = taskGroupQueue.getTaskId(); final TaskInstance taskInstance = taskInstanceMap.get(taskId); @@ -206,7 +260,7 @@ private void amendTaskGroupQueueStatus(List taskGroupQueues) { } } - private void dealWithForceStartTaskGroupQueue() { + private void dealWithForceStartTaskGroupQueue(CoordinatorThread run) { // Find the force start task group queue(Which is inQueue and forceStart is YES) // Notify the related waiting task instance // Set the taskGroupQueue status to RELEASE and remove it from queue @@ -214,13 +268,13 @@ private void dealWithForceStartTaskGroupQueue() { int minTaskGroupQueueId = -1; int limit = DEFAULT_LIMIT; StopWatch taskGroupCoordinatorRoundTimeCost = StopWatch.createStarted(); - while (true) { + while (run.running) { final List taskGroupQueues = taskGroupQueueDao.queryWaitNotifyForceStartTaskGroupQueue(minTaskGroupQueueId, limit); - if (CollectionUtils.isEmpty(taskGroupQueues)) { + if (!run.running || CollectionUtils.isEmpty(taskGroupQueues)) { break; } - dealWithForceStartTaskGroupQueue(taskGroupQueues); + dealWithForceStartTaskGroupQueue(taskGroupQueues, run); if (taskGroupQueues.size() < limit) { break; } @@ -230,11 +284,14 @@ private void dealWithForceStartTaskGroupQueue() { taskGroupCoordinatorRoundTimeCost.getTime()); } - private void dealWithForceStartTaskGroupQueue(List taskGroupQueues) { + private void dealWithForceStartTaskGroupQueue(List taskGroupQueues, CoordinatorThread run) { // Find the force start task group queue(Which is inQueue and forceStart is YES) // Notify the related waiting task instance // Set the taskGroupQueue status to RELEASE and remove it from queue for (final TaskGroupQueue taskGroupQueue : taskGroupQueues) { + if (!run.running) { + return; + } try { LogUtils.setTaskInstanceIdMDC(taskGroupQueue.getTaskId()); if (!notifyForceStartTaskGroupQueue(taskGroupQueue)) { @@ -279,17 +336,20 @@ private boolean notifyForceStartTaskGroupQueue(TaskGroupQueue taskGroupQueue) { return notified; } - private void dealWithWaitingTaskGroupQueue() { + private void dealWithWaitingTaskGroupQueue(CoordinatorThread run) { // Find the TaskGroup which usage < maxSize. // Find the highest priority inQueue task group queue(Which is inQueue and status is Waiting and force start is // NO) belong to the // task group. List taskGroups = taskGroupDao.queryAvailableTaskGroups(); - if (CollectionUtils.isEmpty(taskGroups)) { + if (!run.running || CollectionUtils.isEmpty(taskGroups)) { log.debug("There is no available task group"); return; } for (TaskGroup taskGroup : taskGroups) { + if (!run.running) { + return; + } int availableSize = taskGroup.getGroupSize() - taskGroup.getUseSize(); if (availableSize <= 0) { log.info("TaskGroup {} is full, available size is {}", taskGroup, availableSize); @@ -302,11 +362,14 @@ private void dealWithWaitingTaskGroupQueue() { .filter(taskGroupQueue -> TaskGroupQueueStatus.WAIT_QUEUE == taskGroupQueue.getStatus()) .limit(availableSize) .collect(Collectors.toList()); - if (CollectionUtils.isEmpty(taskGroupQueues)) { + if (!run.running || CollectionUtils.isEmpty(taskGroupQueues)) { log.debug("There is no waiting task group queue for task group {}", taskGroup.getName()); continue; } for (TaskGroupQueue taskGroupQueue : taskGroupQueues) { + if (!run.running) { + return; + } try { LogUtils.setTaskInstanceIdMDC(taskGroupQueue.getTaskId()); if (!acquireTaskGroupSlotAndNotify(taskGroupQueue)) { @@ -525,12 +588,12 @@ public synchronized void close() { flag = false; try { if (internalThread != null) { + internalThread.running = false; internalThread.interrupt(); } } catch (Exception ex) { log.error("Close internalThread failed", ex); } - internalThread = null; log.info("TaskGroupCoordinator closed"); } } diff --git a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/workflow/serial/WorkflowSerialCoordinator.java b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/workflow/serial/WorkflowSerialCoordinator.java index a11223c45b2e..64b498cb5a13 100644 --- a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/workflow/serial/WorkflowSerialCoordinator.java +++ b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/workflow/serial/WorkflowSerialCoordinator.java @@ -65,9 +65,10 @@ public class WorkflowSerialCoordinator implements IWorkflowSerialCoordinator { @Autowired private SerialCommandPriorityHandler serialCommandPriorityHandler; + // Desired lifecycle state; a canceled thread must finish before a requested restart. private volatile boolean flag = false; - private Thread internalThread; + private CoordinatorThread internalThread; private static final int DEFAULT_FETCH_SIZE = 1000; @@ -79,23 +80,53 @@ public synchronized void start() { if (flag) { throw new IllegalStateException("WorkflowSerialCoordinator is already started"); } - if (internalThread != null) { - throw new IllegalStateException("InternalThread is already started"); - } flag = true; - internalThread = new BaseDaemonThread(this::doStart) { - }; - internalThread.setName("WorkflowSerialCoordinator-Thread"); + if (internalThread == null) { + startInternalThread(); + } + // A canceled run may still be inside JDBC. Its finally block starts the replacement. + } + + private void startInternalThread() { + internalThread = new CoordinatorThread(); internalThread.start(); log.info("WorkflowSerialCoordinator started..."); } - private void doStart() { - while (flag) { + private final class CoordinatorThread extends BaseDaemonThread { + + private volatile boolean running = true; + + private CoordinatorThread() { + super("WorkflowSerialCoordinator-Thread"); + } + + @Override + public void run() { + try { + doStart(this); + } finally { + synchronized (WorkflowSerialCoordinator.this) { + internalThread = null; + if (flag) { + startInternalThread(); + } + } + } + } + } + + private void doStart(CoordinatorThread run) { + while (run.running) { try { final StopWatch workflowSerialCoordinatorRoundCost = StopWatch.createStarted(); final List serialCommandsGroups = fetchSerialCommands(); - serialCommandsGroups.forEach(this::handleSerialCommand); + for (SerialCommandsGroup serialCommandsGroup : serialCommandsGroups) { + if (!run.running) { + return; + } + handleSerialCommand(serialCommandsGroup); + } log.debug("WorkflowSerialCoordinator handled SerialCommandsGroup size: {}, cost: {}/ms ", serialCommandsGroups.size(), workflowSerialCoordinatorRoundCost.getDuration().toMillis()); @@ -103,7 +134,9 @@ private void doStart() { log.error("WorkflowSerialCoordinator error", e); } finally { // sleep 5s - ThreadUtils.sleep(TimeUnit.SECONDS.toMillis(DEFAULT_FETCH_INTERVAL_SECONDS)); + if (run.running) { + ThreadUtils.sleep(TimeUnit.SECONDS.toMillis(DEFAULT_FETCH_INTERVAL_SECONDS)); + } } } } @@ -177,12 +210,12 @@ public synchronized void close() { flag = false; try { if (internalThread != null) { + internalThread.running = false; internalThread.interrupt(); } } catch (Exception ex) { log.error("Close internalThread failed", ex); } - internalThread = null; log.info("WorkflowSerialCoordinator closed"); } } diff --git a/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/TaskGroupCoordinatorTest.java b/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/TaskGroupCoordinatorTest.java index 6f4b7d516ca4..7a50bfe8b804 100644 --- a/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/TaskGroupCoordinatorTest.java +++ b/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/TaskGroupCoordinatorTest.java @@ -35,7 +35,11 @@ import org.apache.dolphinscheduler.dao.repository.TaskInstanceDao; import org.apache.dolphinscheduler.dao.repository.WorkflowInstanceDao; +import java.util.Collections; import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -46,6 +50,7 @@ import org.mockito.junit.jupiter.MockitoExtension; import org.mockito.junit.jupiter.MockitoSettings; import org.mockito.quality.Strictness; +import org.springframework.test.util.ReflectionTestUtils; import com.google.common.collect.Lists; @@ -171,4 +176,90 @@ void releaseTaskGroupSlot() { verify(taskGroupQueueDao, Mockito.times(1)).deleteById(taskGroupQueue); } + @Test + void restartShouldWaitForPreviousFetchToReturn() throws Exception { + verifyRestartDuringFetch(false); + } + + @Test + void closeShouldCancelPendingRestart() throws Exception { + verifyRestartDuringFetch(true); + } + + private void verifyRestartDuringFetch(boolean cancelRestart) throws Exception { + taskGroupCoordinator = Mockito.spy(taskGroupCoordinator); + Mockito.doNothing().when(taskGroupCoordinator).pauseBeforeStart(); + CountDownLatch fetching = new CountDownLatch(1); + CountDownLatch releaseFetch = new CountDownLatch(1); + CountDownLatch restarted = new CountDownLatch(1); + AtomicReference workerFailure = new AtomicReference<>(); + AtomicReference firstThread = new AtomicReference<>(); + Mockito.when(taskGroupDao.queryAllTaskGroups()).thenAnswer(invocation -> { + if (firstThread.compareAndSet(null, Thread.currentThread())) { + fetching.countDown(); + // JDBC calls can finish after interrupt. Keep the previous run inside its DAO call. + boolean released = false; + while (!released) { + try { + released = releaseFetch.await(5, TimeUnit.SECONDS); + if (!released) { + workerFailure.set(new AssertionError("Test did not release the blocked DAO")); + return Collections.emptyList(); + } + } catch (InterruptedException ignored) { + // Model a driver that does not cancel its request on interrupt. + } + } + TaskGroup staleGroup = new TaskGroup(); + staleGroup.setId(1); + staleGroup.setUseSize(1); + return Collections.singletonList(staleGroup); + } else { + if (firstThread.get() == Thread.currentThread()) { + workerFailure.set(new AssertionError("Old polling loop resumed")); + } + restarted.countDown(); + } + return Collections.emptyList(); + }); + try { + taskGroupCoordinator.start(); + Assertions.assertTrue(fetching.await(5, TimeUnit.SECONDS)); + taskGroupCoordinator.close(); + taskGroupCoordinator.start(); + Assertions.assertSame(firstThread.get(), ReflectionTestUtils.getField(taskGroupCoordinator, + "internalThread"), "Keep the old run until its DAO call returns"); + if (cancelRestart) { + taskGroupCoordinator.close(); + } + releaseFetch.countDown(); + firstThread.get().join(5000); + Assertions.assertFalse(firstThread.get().isAlive()); + if (cancelRestart) { + Assertions.assertEquals(1L, restarted.getCount()); + Assertions.assertNull(ReflectionTestUtils.getField(taskGroupCoordinator, "internalThread")); + } else { + Assertions.assertTrue(restarted.await(5, TimeUnit.SECONDS)); + } + Assertions.assertNull(workerFailure.get()); + // The old fetch returned a nonempty batch, but cancellation must discard it. + Mockito.verify(taskGroupQueueDao, Mockito.never()).countUsingTaskGroupQueueByGroupId(Mockito.anyInt()); + } finally { + Thread latestThread; + synchronized (taskGroupCoordinator) { + latestThread = (Thread) ReflectionTestUtils.getField(taskGroupCoordinator, "internalThread"); + taskGroupCoordinator.close(); + } + releaseFetch.countDown(); + if (latestThread != null) { + latestThread.join(5000); + Assertions.assertFalse(latestThread.isAlive()); + } + if (firstThread.get() != null) { + firstThread.get().join(5000); + Assertions.assertFalse(firstThread.get().isAlive()); + } + } + } + } diff --git a/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/workflow/serial/WorkflowSerialCoordinatorTest.java b/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/workflow/serial/WorkflowSerialCoordinatorTest.java index cac58146efcd..c9bdacb62baa 100644 --- a/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/workflow/serial/WorkflowSerialCoordinatorTest.java +++ b/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/workflow/serial/WorkflowSerialCoordinatorTest.java @@ -20,16 +20,32 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertThrows; +import org.apache.dolphinscheduler.common.enums.WorkflowExecutionTypeEnum; +import org.apache.dolphinscheduler.dao.entity.WorkflowDefinitionLog; +import org.apache.dolphinscheduler.dao.model.SerialCommandDto; import org.apache.dolphinscheduler.dao.repository.SerialCommandDao; import org.apache.dolphinscheduler.dao.repository.WorkflowDefinitionLogDao; +import org.apache.dolphinscheduler.server.master.engine.ITaskGroupCoordinator; +import org.apache.dolphinscheduler.server.master.engine.MasterCoordinator; +import org.apache.dolphinscheduler.server.master.failover.IFailoverCoordinator; +import java.util.Arrays; +import java.util.Collections; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.InjectMocks; import org.mockito.Mock; +import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; import org.mockito.junit.jupiter.MockitoSettings; import org.mockito.quality.Strictness; +import org.springframework.test.util.ReflectionTestUtils; @ExtendWith(MockitoExtension.class) @MockitoSettings(strictness = Strictness.LENIENT) @@ -81,4 +97,181 @@ void closeShouldBeIdempotent() { assertDoesNotThrow(() -> workflowSerialCoordinator.close()); } + @Test + void restartShouldWaitForPreviousFetchToReturn() throws Exception { + verifyRestartDuringFetch(false); + } + + @Test + void closeShouldCancelPendingRestart() throws Exception { + verifyRestartDuringFetch(true); + } + + private void verifyRestartDuringFetch(boolean cancelRestart) throws Exception { + CountDownLatch fetching = new CountDownLatch(1); + CountDownLatch releaseFetch = new CountDownLatch(1); + CountDownLatch restarted = new CountDownLatch(1); + AtomicReference workerFailure = new AtomicReference<>(); + AtomicReference firstThread = new AtomicReference<>(); + Mockito.when(serialCommandDao.fetchSerialCommands(Mockito.anyInt())).thenAnswer(invocation -> { + if (firstThread.compareAndSet(null, Thread.currentThread())) { + fetching.countDown(); + // JDBC calls can finish after interrupt. Keep the previous run inside its DAO call. + boolean released = false; + while (!released) { + try { + released = releaseFetch.await(5, TimeUnit.SECONDS); + if (!released) { + workerFailure.set(new AssertionError("Test did not release the blocked DAO")); + return Collections.emptyList(); + } + } catch (InterruptedException ignored) { + // Model a driver that does not cancel its request on interrupt. + } + } + return Collections.singletonList(SerialCommandDto.builder() + .workflowDefinitionCode(1L).workflowDefinitionVersion(1).build()); + } else { + if (firstThread.get() == Thread.currentThread()) { + workerFailure.set(new AssertionError("Old polling loop resumed")); + } + restarted.countDown(); + } + return Collections.emptyList(); + }); + WorkflowDefinitionLog definition = + new WorkflowDefinitionLog(); + definition.setExecutionType(WorkflowExecutionTypeEnum.SERIAL_WAIT); + Mockito.when(workflowDefinitionLogDao.queryByDefinitionCodeAndVersion(1L, 1)).thenReturn(definition); + try { + workflowSerialCoordinator.start(); + Assertions.assertTrue(fetching.await(5, TimeUnit.SECONDS)); + workflowSerialCoordinator.close(); + workflowSerialCoordinator.start(); + Assertions.assertSame(firstThread.get(), ReflectionTestUtils.getField(workflowSerialCoordinator, + "internalThread"), "Keep the old run until its DAO call returns"); + if (cancelRestart) { + workflowSerialCoordinator.close(); + } + releaseFetch.countDown(); + firstThread.get().join(5000); + Assertions.assertFalse(firstThread.get().isAlive()); + if (cancelRestart) { + Assertions.assertEquals(1L, restarted.getCount()); + Assertions.assertNull(ReflectionTestUtils.getField(workflowSerialCoordinator, "internalThread")); + } else { + Assertions.assertTrue(restarted.await(5, TimeUnit.SECONDS)); + } + Assertions.assertNull(workerFailure.get()); + // The old fetch returned a nonempty batch, but cancellation must discard it. + Mockito.verifyNoInteractions(serialCommandWaitHandler); + } finally { + Thread latestThread; + synchronized (workflowSerialCoordinator) { + latestThread = (Thread) ReflectionTestUtils.getField(workflowSerialCoordinator, "internalThread"); + workflowSerialCoordinator.close(); + } + releaseFetch.countDown(); + if (latestThread != null) { + latestThread.join(5000); + Assertions.assertFalse(latestThread.isAlive()); + } + if (firstThread.get() != null) { + firstThread.get().join(5000); + Assertions.assertFalse(firstThread.get().isAlive()); + } + } + } + + @Test + void roleChangesShouldWaitForInFlightHandlerAndDiscardRemainingGroups() throws Exception { + MasterCoordinator.MasterCoordinatorListener listener = new MasterCoordinator.MasterCoordinatorListener( + Mockito.mock(ITaskGroupCoordinator.class), Mockito.mock(IFailoverCoordinator.class), + workflowSerialCoordinator); + CountDownLatch handling = new CountDownLatch(1); + CountDownLatch releaseHandler = new CountDownLatch(1); + CountDownLatch replacementHandled = new CountDownLatch(1); + CountDownLatch releaseReplacement = new CountDownLatch(1); + AtomicReference oldThread = new AtomicReference<>(); + AtomicReference workerFailure = new AtomicReference<>(); + AtomicInteger fetches = new AtomicInteger(); + AtomicInteger handled = new AtomicInteger(); + WorkflowDefinitionLog definition = new WorkflowDefinitionLog(); + definition.setExecutionType(WorkflowExecutionTypeEnum.SERIAL_WAIT); + Mockito.when(workflowDefinitionLogDao.queryByDefinitionCodeAndVersion(Mockito.anyLong(), Mockito.anyInt())) + .thenReturn(definition); + SerialCommandDto first = SerialCommandDto.builder().workflowDefinitionCode(1L) + .workflowDefinitionVersion(1).build(); + SerialCommandDto second = SerialCommandDto.builder().workflowDefinitionCode(2L) + .workflowDefinitionVersion(1).build(); + Mockito.when(serialCommandDao.fetchSerialCommands(Mockito.anyInt())).thenAnswer(invocation -> { + if (fetches.incrementAndGet() == 1) { + return Arrays.asList(first, second); + } + return Collections.singletonList(first); + }); + Mockito.doAnswer(invocation -> { + if (handled.incrementAndGet() == 1) { + oldThread.set(Thread.currentThread()); + handling.countDown(); + // A handler already executing cannot be forcibly canceled. Its successor must wait. + while (true) { + try { + if (!releaseHandler.await(5, TimeUnit.SECONDS)) { + workerFailure.set(new AssertionError("Handler was not released")); + } + break; + } catch (InterruptedException ignored) { + // Model an in-flight database request ignoring interruption. + } + } + } else { + if (Thread.currentThread() == oldThread.get()) { + workerFailure.set(new AssertionError("Canceled run handled another group")); + } + replacementHandled.countDown(); + // Keep the successor at the observation point instead of relying on the polling interval. + try { + releaseReplacement.await(); + } catch (InterruptedException ignored) { + // Test cleanup closes the successor before releasing this latch. + } + } + return null; + }).when(serialCommandWaitHandler).handle(Mockito.any()); + try { + listener.changeToActive(); + Assertions.assertTrue(handling.await(5, TimeUnit.SECONDS)); + listener.changeToStandBy(); + listener.changeToActive(); + listener.changeToStandBy(); + listener.changeToActive(); + Assertions.assertSame(oldThread.get(), ReflectionTestUtils.getField(workflowSerialCoordinator, + "internalThread")); + Assertions.assertEquals(1, fetches.get()); + releaseHandler.countDown(); + Assertions.assertTrue(replacementHandled.await(5, TimeUnit.SECONDS)); + oldThread.get().join(5000); + Assertions.assertFalse(oldThread.get().isAlive()); + Assertions.assertNull(workerFailure.get()); + Assertions.assertEquals(2, handled.get()); + } finally { + Thread latestThread; + synchronized (workflowSerialCoordinator) { + latestThread = (Thread) ReflectionTestUtils.getField(workflowSerialCoordinator, "internalThread"); + listener.changeToStandBy(); + } + releaseHandler.countDown(); + releaseReplacement.countDown(); + if (latestThread != null) { + latestThread.join(5000); + Assertions.assertFalse(latestThread.isAlive()); + } + if (oldThread.get() != null) { + oldThread.get().join(5000); + Assertions.assertFalse(oldThread.get().isAlive()); + } + } + } + } From 43ab835efc08cd56f02c0874eb53923609a419e2 Mon Sep 17 00:00:00 2001 From: michaellx1057 Date: Wed, 16 Sep 2026 22:28:23 +0800 Subject: [PATCH 4/5] [Fix-18644][Registry] Narrow HA repair to ownership and coordinator handoff --- .../alert/registry/AlertRegistryClient.java | 13 +- .../alert/service/AlertHAServer.java | 1 - .../alert/AlertServerHATest.java | 116 ------------------ .../master/engine/MasterCoordinator.java | 1 - .../registry/api/ha/AbstractHAServer.java | 39 ------ .../registry/api/ha/AbstractHAServerTest.java | 90 +++----------- 6 files changed, 20 insertions(+), 240 deletions(-) delete mode 100644 dolphinscheduler-alert/dolphinscheduler-alert-server/src/test/java/org/apache/dolphinscheduler/alert/AlertServerHATest.java diff --git a/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/registry/AlertRegistryClient.java b/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/registry/AlertRegistryClient.java index be91c96ebf26..1b7839d81628 100644 --- a/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/registry/AlertRegistryClient.java +++ b/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/registry/AlertRegistryClient.java @@ -22,8 +22,6 @@ import org.apache.dolphinscheduler.meter.metrics.MetricsProvider; import org.apache.dolphinscheduler.registry.api.RegistryClient; -import java.io.IOException; - import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Autowired; @@ -56,16 +54,9 @@ public void start() { } @Override - public void close() throws IOException { + public void close() { log.info("AlertRegistryClient closing..."); - try { - if (alertHeartbeatTask != null) { - alertHeartbeatTask.shutdown(); - } - } finally { - // Stop renewing the HA selector when the entire AlertServer shuts down. - registryClient.close(); - } + alertHeartbeatTask.shutdown(); log.info("AlertRegistryClient closed..."); } diff --git a/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/service/AlertHAServer.java b/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/service/AlertHAServer.java index cc5a920cd1c1..67382ac470a7 100644 --- a/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/service/AlertHAServer.java +++ b/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/service/AlertHAServer.java @@ -42,7 +42,6 @@ public void start() { @Override public void close() { - super.close(); log.info("AlertHAServer shutdown..."); } } diff --git a/dolphinscheduler-alert/dolphinscheduler-alert-server/src/test/java/org/apache/dolphinscheduler/alert/AlertServerHATest.java b/dolphinscheduler-alert/dolphinscheduler-alert-server/src/test/java/org/apache/dolphinscheduler/alert/AlertServerHATest.java deleted file mode 100644 index 199385d86e47..000000000000 --- a/dolphinscheduler-alert/dolphinscheduler-alert-server/src/test/java/org/apache/dolphinscheduler/alert/AlertServerHATest.java +++ /dev/null @@ -1,116 +0,0 @@ -/* - * 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.dolphinscheduler.alert; - -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.doNothing; -import static org.mockito.Mockito.doThrow; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.mockStatic; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - -import org.apache.dolphinscheduler.alert.config.AlertConfig; -import org.apache.dolphinscheduler.alert.plugin.AlertPluginManager; -import org.apache.dolphinscheduler.alert.registry.AlertHeartbeatTask; -import org.apache.dolphinscheduler.alert.registry.AlertRegistryClient; -import org.apache.dolphinscheduler.alert.rpc.AlertRpcServer; -import org.apache.dolphinscheduler.alert.service.AlertBootstrapService; -import org.apache.dolphinscheduler.alert.service.AlertHAServer; -import org.apache.dolphinscheduler.common.lifecycle.ServerLifeCycleManager; -import org.apache.dolphinscheduler.common.thread.ThreadUtils; -import org.apache.dolphinscheduler.registry.api.Event; -import org.apache.dolphinscheduler.registry.api.Registry; -import org.apache.dolphinscheduler.registry.api.RegistryClient; -import org.apache.dolphinscheduler.registry.api.SubscribeListener; -import org.apache.dolphinscheduler.registry.api.enums.RegistryNodeType; - -import java.util.concurrent.atomic.AtomicReference; - -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; -import org.mockito.MockedStatic; -import org.springframework.test.util.ReflectionTestUtils; - -class AlertServerHATest { - - @ParameterizedTest - @ValueSource(booleans = {false, true}) - void testDemotionClosesAlertServerWithoutReactivatingClosedServices(boolean heartbeatShutdownFails) throws Exception { - Registry registry = mock(Registry.class); - AlertConfig config = mock(AlertConfig.class); - when(config.getAlertServerAddress()).thenReturn("alert-0:50052"); - AlertHAServer haServer = new AlertHAServer(registry, config); - AlertBootstrapService bootstrapService = mock(AlertBootstrapService.class); - AlertRpcServer rpcServer = mock(AlertRpcServer.class); - AlertRegistryClient registryClient = spy(new AlertRegistryClient()); - AlertHeartbeatTask heartbeatTask = mock(AlertHeartbeatTask.class); - if (heartbeatShutdownFails) { - doThrow(new IllegalStateException("heartbeat shutdown failed")).when(heartbeatTask).shutdown(); - } - ReflectionTestUtils.setField(registryClient, "registryClient", new RegistryClient(registry)); - ReflectionTestUtils.setField(registryClient, "alertHeartbeatTask", heartbeatTask); - doNothing().when(registryClient).start(); - AlertServer alertServer = new AlertServer(); - ReflectionTestUtils.setField(alertServer, "alertHAServer", haServer); - ReflectionTestUtils.setField(alertServer, "alertBootstrapService", bootstrapService); - ReflectionTestUtils.setField(alertServer, "alertRpcServer", rpcServer); - ReflectionTestUtils.setField(alertServer, "alertRegistryClient", registryClient); - ReflectionTestUtils.setField(alertServer, "alertPluginManager", mock(AlertPluginManager.class)); - - String selectorPath = RegistryNodeType.ALERT_HA_LEADER.getRegistryPath(); - AtomicReference subscriber = new AtomicReference<>(); - doAnswer(invocation -> { - subscriber.set(invocation.getArgument(1)); - return null; - }).when(registry).subscribe(eq(selectorPath), org.mockito.ArgumentMatchers.any()); - when(registry.acquireLock(anyString())).thenReturn(true); - try ( - MockedStatic lifecycle = mockStatic(ServerLifeCycleManager.class); - MockedStatic ignored = mockStatic(ThreadUtils.class)) { - lifecycle.when(ServerLifeCycleManager::toStopped).thenReturn(true); - alertServer.run(); - assertTrue(haServer.isActive()); - verify(bootstrapService).start(); - - // AlertServer's real demotion listener closes the whole server, rather than pausing it. - // A transient election error must not retry and restart these closed services. - when(registry.exists(selectorPath)) - .thenThrow(new IllegalStateException("temporary registry failure")) - .thenReturn(false); - Event removal = new Event(selectorPath, selectorPath, "", Event.Type.REMOVE); - subscriber.get().notify(removal); - subscriber.get().notify(removal); - haServer.start(); - assertFalse(haServer.isActive()); - verify(bootstrapService, times(1)).start(); - verify(bootstrapService, times(1)).close(); - verify(rpcServer).close(); - verify(registryClient).close(); - verify(heartbeatTask).shutdown(); - verify(registry).close(); - verify(registry, times(2)).acquireLock(anyString()); - } - } -} diff --git a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/MasterCoordinator.java b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/MasterCoordinator.java index 8a374acfa8dd..eb2a9dda02cd 100644 --- a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/MasterCoordinator.java +++ b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/MasterCoordinator.java @@ -71,7 +71,6 @@ public void start() { @Override public void close() { - super.close(); taskGroupCoordinator.close(); log.info("MasterCoordinator shutdown..."); } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServer.java b/dolphinscheduler-registry/dolphinscheduler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServer.java index a984eebaaeba..4846d87e0316 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServer.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServer.java @@ -41,8 +41,6 @@ public abstract class AbstractHAServer implements HAServer { private volatile ServerStatus serverStatus; - private volatile boolean closed; - private final List serverStatusChangeListeners; private static final long DEFAULT_RETRY_INTERVAL = 5_000; @@ -60,9 +58,6 @@ public AbstractHAServer(final Registry registry, final String selectorPath, fina @Override public void start() { - if (closed) { - return; - } registry.subscribe(selectorPath, new SubscribeListener() { @Override @@ -82,16 +77,9 @@ public SubscribeScope getSubscribeScope() { } private synchronized void reconcileElection() { - if (closed) { - return; - } // Serialize election and publication with callbacks, including callbacks during startup. // REMOVE may be delayed or have no previous value, so consult current ownership instead. boolean elected = participateElection(); - // A demotion listener may close the entire server (for example, AlertServer). - if (closed) { - return; - } if (elected) { statusChange(ServerStatus.ACTIVE); } else { @@ -111,21 +99,12 @@ public synchronized boolean participateElection() { // If meet exception during participate election, will retry. // This can avoid the situation that the server is not elected as leader due to network jitter. for (int i = 0; i < DEFAULT_MAX_RETRY_TIMES; i++) { - if (closed) { - return false; - } boolean lockAcquired = false; try { try { lockAcquired = registry.acquireLock(electionLock); if (lockAcquired) { - if (closed) { - return false; - } if (!registry.exists(selectorPath)) { - if (closed) { - return false; - } registry.put(selectorPath, serverIdentify, true); return true; } @@ -138,11 +117,6 @@ public synchronized boolean participateElection() { } } } catch (Exception e) { - // Do not keep coordinator services active while ownership cannot be verified. - statusChange(ServerStatus.STAND_BY); - if (closed) { - return false; - } log.error("Participate election error, meet an exception, will retry after {}ms", DEFAULT_RETRY_INTERVAL, e); ThreadUtils.sleep(DEFAULT_RETRY_INTERVAL); @@ -152,16 +126,6 @@ public synchronized boolean participateElection() { "Participate election failed after retry " + DEFAULT_MAX_RETRY_TIMES + " times"); } - @Override - public void close() { - // Publish shutdown before waiting for an in-flight election to release the monitor. - closed = true; - synchronized (this) { - // Consumers close their own services; notifying listeners here could recurse. - serverStatus = ServerStatus.STAND_BY; - } - } - @Override public void addServerStatusChangeListener(ServerStatusChangeListener listener) { serverStatusChangeListeners.add(listener); @@ -173,9 +137,6 @@ public ServerStatus getServerStatus() { } private synchronized void statusChange(ServerStatus targetStatus) { - if (closed) { - return; - } final ServerStatus originStatus = serverStatus; serverStatus = targetStatus; try { diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-api/src/test/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServerTest.java b/dolphinscheduler-registry/dolphinscheduler-registry-api/src/test/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServerTest.java index cb7adb1ec95b..6f22a006f071 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-api/src/test/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServerTest.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-api/src/test/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServerTest.java @@ -201,43 +201,6 @@ void testActiveServerDemotesWhenLockCannotBeAcquired() { verify(registry, times(1)).releaseLock(ELECTION_LOCK); } - @Test - void testElectionErrorDemotesBeforeRetryAndReleasesAcquiredLock() { - server.start(); - when(registry.exists(SELECTOR_PATH)).thenThrow(new IllegalStateException("registry unavailable")); - try (MockedStatic threadUtils = mockStatic(ThreadUtils.class)) { - // Verify safety at each retry boundary, without spending real time on retry sleeps. - threadUtils.when(() -> ThreadUtils.sleep(5_000)).thenAnswer(invocation -> { - assertFalse(server.isActive()); - verify(statusListener, times(1)).changeToStandBy(); - return null; - }); - assertThrows(IllegalStateException.class, () -> remove("")); - } - assertFalse(server.isActive()); - verify(statusListener, times(1)).changeToStandBy(); - verify(registry, times(21)).releaseLock(ELECTION_LOCK); - } - - @Test - void testTransientElectionErrorStopsAndRestartsCoordinator() { - server.start(); - when(registry.exists(SELECTOR_PATH)) - .thenThrow(new IllegalStateException("temporary registry failure")) - .thenReturn(true); - try (MockedStatic threadUtils = mockStatic(ThreadUtils.class)) { - threadUtils.when(() -> ThreadUtils.sleep(5_000)).thenAnswer(invocation -> { - assertFalse(server.isActive()); - verify(statusListener).changeToStandBy(); - return null; - }); - remove(""); - } - assertTrue(server.isActive()); - verify(statusListener, times(2)).changeToActive(); - verify(statusListener).changeToStandBy(); - } - @Test void testAcquisitionErrorDoesNotReleaseUnacquiredLock() { when(registry.acquireLock(ELECTION_LOCK)).thenThrow(new IllegalStateException("lock unavailable")); @@ -250,52 +213,35 @@ void testAcquisitionErrorDoesNotReleaseUnacquiredLock() { } @Test - void testShutdownDuringLockAcquisitionDoesNotClaimSelector() { - when(registry.acquireLock(ELECTION_LOCK)).thenAnswer(invocation -> { - // Shutdown is requested before the blocking acquisition returns to the election. - server.close(); - return true; - }); - server.start(); - assertFalse(server.isActive()); - verify(registry, never()).put(eq(SELECTOR_PATH), anyString(), eq(true)); - verify(registry).releaseLock(ELECTION_LOCK); - verify(statusListener, never()).changeToActive(); - } - - @Test - void testTerminalDemotionListenerPreventsRetryAndReactivation() { - doAnswer(invocation -> { - server.close(); - return null; - }).when(statusListener).changeToStandBy(); + void testTransientElectionErrorKeepsRoleUntilOwnershipDecision() { server.start(); when(registry.exists(SELECTOR_PATH)) .thenThrow(new IllegalStateException("temporary registry failure")) .thenReturn(true); try (MockedStatic threadUtils = mockStatic(ThreadUtils.class)) { + threadUtils.when(() -> ThreadUtils.sleep(5_000)).thenAnswer(invocation -> { + assertTrue(server.isActive()); + verify(statusListener, never()).changeToStandBy(); + return null; + }); remove(""); - // AlertServer shuts down on demotion, unlike restartable Master coordinators. - threadUtils.verifyNoInteractions(); } - remove(""); - assertFalse(server.isActive()); + assertTrue(server.isActive()); verify(statusListener, times(1)).changeToActive(); - verify(statusListener, times(1)).changeToStandBy(); - verify(registry, times(2)).acquireLock(ELECTION_LOCK); + verify(statusListener, never()).changeToStandBy(); } @Test - void testClosePreventsRestartAndFurtherElection() { - server.start(); - server.close(); - remove(""); + void testExhaustedRetriesPreserveOriginalRole() { server.start(); - assertFalse(server.participateElection()); - assertFalse(server.isActive()); - verify(statusListener, times(1)).changeToActive(); - verify(registry, times(1)).acquireLock(ELECTION_LOCK); - verify(registry, times(1)).subscribe(eq(SELECTOR_PATH), org.mockito.ArgumentMatchers.any()); + when(registry.exists(SELECTOR_PATH)).thenThrow(new IllegalStateException("registry unavailable")); + try (MockedStatic ignored = mockStatic(ThreadUtils.class)) { + assertThrows(IllegalStateException.class, () -> remove("")); + } + // Exception-driven demotion is deliberately outside this minimal candidate. + assertTrue(server.isActive()); + verify(statusListener, never()).changeToStandBy(); + verify(registry, times(21)).releaseLock(ELECTION_LOCK); } @Test @@ -368,7 +314,7 @@ private AbstractHAServer newServer(String address) { @Override public void close() { - super.close(); + } }; } From 50f8ad92ce556882b3994a2f81b88217023c573a Mon Sep 17 00:00:00 2001 From: michaellx1057 Date: Thu, 17 Sep 2026 16:23:48 +0800 Subject: [PATCH 5/5] [Fix-18644][Registry] Serialize HA transitions and coordinator shutdown Run ownership reconciliation on a single election worker. Request both coordinator workers to stop before joining either on demotion and normal Master shutdown. Add deterministic lifecycle and shutdown regression coverage. --- .../alert/service/AlertHAServer.java | 1 + .../master/engine/ITaskGroupCoordinator.java | 5 + .../engine/IWorkflowSerialCoordinator.java | 5 + .../master/engine/MasterCoordinator.java | 24 +- .../master/engine/TaskGroupCoordinator.java | 155 +++---- .../serial/WorkflowSerialCoordinator.java | 91 ++-- .../master/engine/MasterCoordinatorTest.java | 206 +++++++++ .../engine/TaskGroupCoordinatorTest.java | 153 ++++--- .../serial/WorkflowSerialCoordinatorTest.java | 420 +++++++++++++----- .../registry/api/ha/AbstractHAServer.java | 133 ++++-- .../registry/api/ha/AbstractHAServerTest.java | 340 +++++++++++--- 11 files changed, 1133 insertions(+), 400 deletions(-) create mode 100644 dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/MasterCoordinatorTest.java diff --git a/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/service/AlertHAServer.java b/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/service/AlertHAServer.java index 67382ac470a7..cc5a920cd1c1 100644 --- a/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/service/AlertHAServer.java +++ b/dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/service/AlertHAServer.java @@ -42,6 +42,7 @@ public void start() { @Override public void close() { + super.close(); log.info("AlertHAServer shutdown..."); } } diff --git a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/ITaskGroupCoordinator.java b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/ITaskGroupCoordinator.java index 854097b02124..7d841aa15d9b 100644 --- a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/ITaskGroupCoordinator.java +++ b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/ITaskGroupCoordinator.java @@ -52,6 +52,11 @@ public interface ITaskGroupCoordinator extends AutoCloseable { */ void start(); + /** + * Request the worker to stop without joining the worker. Call {@link #close()} to wait before restarting. + */ + void requestStop(); + /** * If the {@link TaskInstance#getTaskGroupId()} > 0, and the TaskGroup flag is {@link Flag#YES} then the task instance need to use task group. * diff --git a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/IWorkflowSerialCoordinator.java b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/IWorkflowSerialCoordinator.java index bbd9c248c6de..12d57143fc65 100644 --- a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/IWorkflowSerialCoordinator.java +++ b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/IWorkflowSerialCoordinator.java @@ -21,6 +21,11 @@ public interface IWorkflowSerialCoordinator extends AutoCloseable { void start(); + /** + * Request the worker to stop without joining the worker. Call {@link #close()} to wait before restarting. + */ + void requestStop(); + @Override void close(); diff --git a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/MasterCoordinator.java b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/MasterCoordinator.java index eb2a9dda02cd..81d5533b8edd 100644 --- a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/MasterCoordinator.java +++ b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/MasterCoordinator.java @@ -41,11 +41,7 @@ @Component public class MasterCoordinator extends AbstractHAServer { - private final ITaskGroupCoordinator taskGroupCoordinator; - - private final IFailoverCoordinator failoverCoordinator; - - private final IWorkflowSerialCoordinator workflowSerialCoordinator; + private final MasterCoordinatorListener masterCoordinatorListener; public MasterCoordinator(final Registry registry, final MasterConfig masterConfig, @@ -56,11 +52,9 @@ public MasterCoordinator(final Registry registry, registry, RegistryNodeType.MASTER_COORDINATOR.getRegistryPath(), masterConfig.getMasterAddress()); - this.taskGroupCoordinator = taskGroupCoordinator; - this.failoverCoordinator = failoverCoordinator; - this.workflowSerialCoordinator = workflowSerialCoordinator; - addServerStatusChangeListener( - new MasterCoordinatorListener(taskGroupCoordinator, failoverCoordinator, workflowSerialCoordinator)); + this.masterCoordinatorListener = + new MasterCoordinatorListener(taskGroupCoordinator, failoverCoordinator, workflowSerialCoordinator); + addServerStatusChangeListener(masterCoordinatorListener); } @Override @@ -71,7 +65,8 @@ public void start() { @Override public void close() { - taskGroupCoordinator.close(); + super.close(); + masterCoordinatorListener.changeToStandBy(); log.info("MasterCoordinator shutdown..."); } @@ -108,11 +103,14 @@ public void changeToActive() { @Override public void changeToStandBy() { - taskGroupCoordinator.close(); - workflowSerialCoordinator.close(); + // Stop both workers before waiting: either may be blocked in a database call. + taskGroupCoordinator.requestStop(); + workflowSerialCoordinator.requestStop(); if (failoverCoordinatorFuture != null) { failoverCoordinatorFuture.cancel(true); } + taskGroupCoordinator.close(); + workflowSerialCoordinator.close(); } } diff --git a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/TaskGroupCoordinator.java b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/TaskGroupCoordinator.java index 10a5fed0d58b..8fc25a300029 100644 --- a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/TaskGroupCoordinator.java +++ b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/TaskGroupCoordinator.java @@ -77,10 +77,9 @@ public class TaskGroupCoordinator implements ITaskGroupCoordinator, AutoCloseabl @Autowired private TransactionTemplate transactionTemplate; - // Desired lifecycle state; a canceled thread must finish before a requested restart. private volatile boolean flag = false; - private CoordinatorThread internalThread; + private Thread internalThread; private static final int DEFAULT_LIMIT = 1000; @@ -89,70 +88,45 @@ public synchronized void start() { if (flag) { throw new IllegalStateException("TaskGroupCoordinator is already started"); } - flag = true; - if (internalThread == null) { - startInternalThread(); + if (internalThread != null) { + throw new IllegalStateException("InternalThread is already started"); } - // A canceled run may still be inside JDBC. Its finally block starts the replacement. - } - - private void startInternalThread() { - internalThread = new CoordinatorThread(); + flag = true; + internalThread = new BaseDaemonThread(this::doStart) { + }; + internalThread.setName("TaskGroupCoordinator-Thread"); internalThread.start(); log.info("TaskGroupCoordinator started..."); } - private final class CoordinatorThread extends BaseDaemonThread { - - private volatile boolean running = true; - - private CoordinatorThread() { - super("TaskGroupCoordinator-Thread"); - } - - @Override - public void run() { - try { - doStart(this); - } finally { - synchronized (TaskGroupCoordinator.this) { - internalThread = null; - if (flag) { - startInternalThread(); - } - } - } - } - } - @VisibleForTesting boolean isStarted() { return flag; } - private void doStart(CoordinatorThread run) { - pauseBeforeStart(); + private void doStart() { + // Sleep 1 minutes here to make sure the previous task group slot has been released. + // This step is not necessary, since the wakeup operation is idempotent, but we can avoid confusion warning. + ThreadUtils.sleep(TimeUnit.MINUTES.toMillis(1)); - while (run.running) { + while (flag) { try { final StopWatch taskGroupCoordinatorRoundCost = StopWatch.createStarted(); - if (!run.running) { + // A database call may outlive a stop request; check flag before starting the next phase. + amendTaskGroupUseSize(); + if (!flag) { return; } - amendTaskGroupUseSize(run); - if (!run.running) { + amendTaskGroupQueueStatus(); + if (!flag) { return; } - amendTaskGroupQueueStatus(run); - if (!run.running) { + dealWithForceStartTaskGroupQueue(); + if (!flag) { return; } - dealWithForceStartTaskGroupQueue(run); - if (!run.running) { - return; - } - dealWithWaitingTaskGroupQueue(run); + dealWithWaitingTaskGroupQueue(); taskGroupCoordinatorRoundCost.stop(); log.debug("TaskGroupCoordinator round cost: {}/ms", taskGroupCoordinatorRoundCost.getTime()); @@ -160,37 +134,31 @@ private void doStart(CoordinatorThread run) { log.error("TaskGroupCoordinator error", e); } finally { // sleep 5s - if (run.running) { + if (flag) { ThreadUtils.sleep(Constants.SLEEP_TIME_MILLIS * 5); } } } } - @VisibleForTesting - void pauseBeforeStart() { - // Sleep 1 minutes here to make sure the previous task group slot has been released. - // This step is not necessary, since the wakeup operation is idempotent, but we can avoid confusion warning. - ThreadUtils.sleep(TimeUnit.MINUTES.toMillis(1)); - } - /** * Make sure the TaskGroup useSize is equal to the TaskGroupQueue which status is {@link TaskGroupQueueStatus#ACQUIRE_SUCCESS} and forceStart is {@link org.apache.dolphinscheduler.common.enums.Flag#NO}. */ - private void amendTaskGroupUseSize(CoordinatorThread run) { + private void amendTaskGroupUseSize() { // The TaskGroup useSize should equal to the TaskGroupQueue which inQueue is YES and forceStart is NO List taskGroups = taskGroupDao.queryAllTaskGroups(); - if (!run.running || CollectionUtils.isEmpty(taskGroups)) { + if (CollectionUtils.isEmpty(taskGroups)) { return; } StopWatch taskGroupCoordinatorRoundTimeCost = StopWatch.createStarted(); for (TaskGroup taskGroup : taskGroups) { - if (!run.running) { + if (!flag) { return; } int actualUseSize = taskGroupQueueDao.countUsingTaskGroupQueueByGroupId(taskGroup.getId()); - if (!run.running) { + // The query may finish after a stop request; do not start the update in that case. + if (!flag) { return; } if (taskGroup.getUseSize() == actualUseSize) { @@ -208,17 +176,17 @@ private void amendTaskGroupUseSize(CoordinatorThread run) { /** * Clear the TaskGroupQueue when the related {@link TaskInstance} is not exist or status is finished. */ - private void amendTaskGroupQueueStatus(CoordinatorThread run) { + private void amendTaskGroupQueueStatus() { int minTaskGroupQueueId = -1; int limit = DEFAULT_LIMIT; StopWatch taskGroupCoordinatorRoundTimeCost = StopWatch.createStarted(); - while (run.running) { + while (flag) { List taskGroupQueues = taskGroupQueueDao.queryInQueueTaskGroupQueue(minTaskGroupQueueId, limit); - if (!run.running || CollectionUtils.isEmpty(taskGroupQueues)) { + if (!flag || CollectionUtils.isEmpty(taskGroupQueues)) { break; } - amendTaskGroupQueueStatus(taskGroupQueues, run); + amendTaskGroupQueueStatus(taskGroupQueues); if (taskGroupQueues.size() < limit) { break; } @@ -230,7 +198,7 @@ private void amendTaskGroupQueueStatus(CoordinatorThread run) { /** * Clear the TaskGroupQueue when the related {@link TaskInstance} is not exist or status is finished. */ - private void amendTaskGroupQueueStatus(List taskGroupQueues, CoordinatorThread run) { + private void amendTaskGroupQueueStatus(List taskGroupQueues) { final List taskInstanceIds = taskGroupQueues.stream() .map(TaskGroupQueue::getTaskId) .collect(Collectors.toList()); @@ -239,7 +207,7 @@ private void amendTaskGroupQueueStatus(List taskGroupQueues, Coo .collect(Collectors.toMap(TaskInstance::getId, Function.identity())); for (TaskGroupQueue taskGroupQueue : taskGroupQueues) { - if (!run.running) { + if (!flag) { return; } int taskId = taskGroupQueue.getTaskId(); @@ -260,7 +228,7 @@ private void amendTaskGroupQueueStatus(List taskGroupQueues, Coo } } - private void dealWithForceStartTaskGroupQueue(CoordinatorThread run) { + private void dealWithForceStartTaskGroupQueue() { // Find the force start task group queue(Which is inQueue and forceStart is YES) // Notify the related waiting task instance // Set the taskGroupQueue status to RELEASE and remove it from queue @@ -268,13 +236,13 @@ private void dealWithForceStartTaskGroupQueue(CoordinatorThread run) { int minTaskGroupQueueId = -1; int limit = DEFAULT_LIMIT; StopWatch taskGroupCoordinatorRoundTimeCost = StopWatch.createStarted(); - while (run.running) { + while (flag) { final List taskGroupQueues = taskGroupQueueDao.queryWaitNotifyForceStartTaskGroupQueue(minTaskGroupQueueId, limit); - if (!run.running || CollectionUtils.isEmpty(taskGroupQueues)) { + if (!flag || CollectionUtils.isEmpty(taskGroupQueues)) { break; } - dealWithForceStartTaskGroupQueue(taskGroupQueues, run); + dealWithForceStartTaskGroupQueue(taskGroupQueues); if (taskGroupQueues.size() < limit) { break; } @@ -284,12 +252,12 @@ private void dealWithForceStartTaskGroupQueue(CoordinatorThread run) { taskGroupCoordinatorRoundTimeCost.getTime()); } - private void dealWithForceStartTaskGroupQueue(List taskGroupQueues, CoordinatorThread run) { + private void dealWithForceStartTaskGroupQueue(List taskGroupQueues) { // Find the force start task group queue(Which is inQueue and forceStart is YES) // Notify the related waiting task instance // Set the taskGroupQueue status to RELEASE and remove it from queue for (final TaskGroupQueue taskGroupQueue : taskGroupQueues) { - if (!run.running) { + if (!flag) { return; } try { @@ -336,18 +304,18 @@ private boolean notifyForceStartTaskGroupQueue(TaskGroupQueue taskGroupQueue) { return notified; } - private void dealWithWaitingTaskGroupQueue(CoordinatorThread run) { + private void dealWithWaitingTaskGroupQueue() { // Find the TaskGroup which usage < maxSize. // Find the highest priority inQueue task group queue(Which is inQueue and status is Waiting and force start is // NO) belong to the // task group. List taskGroups = taskGroupDao.queryAvailableTaskGroups(); - if (!run.running || CollectionUtils.isEmpty(taskGroups)) { + if (CollectionUtils.isEmpty(taskGroups)) { log.debug("There is no available task group"); return; } for (TaskGroup taskGroup : taskGroups) { - if (!run.running) { + if (!flag) { return; } int availableSize = taskGroup.getGroupSize() - taskGroup.getUseSize(); @@ -362,12 +330,12 @@ private void dealWithWaitingTaskGroupQueue(CoordinatorThread run) { .filter(taskGroupQueue -> TaskGroupQueueStatus.WAIT_QUEUE == taskGroupQueue.getStatus()) .limit(availableSize) .collect(Collectors.toList()); - if (!run.running || CollectionUtils.isEmpty(taskGroupQueues)) { + if (CollectionUtils.isEmpty(taskGroupQueues)) { log.debug("There is no waiting task group queue for task group {}", taskGroup.getName()); continue; } for (TaskGroupQueue taskGroupQueue : taskGroupQueues) { - if (!run.running) { + if (!flag) { return; } try { @@ -580,19 +548,34 @@ private void deleteTaskGroupQueueSlot(TaskGroupQueue taskGroupQueue) { } @Override - public synchronized void close() { - if (!flag) { - log.warn("TaskGroupCoordinator is already closed"); - return; - } + public synchronized void requestStop() { flag = false; - try { - if (internalThread != null) { - internalThread.running = false; - internalThread.interrupt(); + if (internalThread != null) { + internalThread.interrupt(); + } + } + + @Override + public synchronized void close() { + if (Thread.currentThread() == internalThread) { + throw new IllegalStateException("TaskGroupCoordinator cannot close its own worker thread"); + } + // A prior stop request does not mean the worker has finished. + requestStop(); + boolean interrupted = false; + if (internalThread != null) { + // Keep start() waiting until the old worker has finished, including any in-flight JDBC call. + while (internalThread.isAlive()) { + try { + internalThread.join(); + } catch (InterruptedException ex) { + interrupted = true; + } } - } catch (Exception ex) { - log.error("Close internalThread failed", ex); + internalThread = null; + } + if (interrupted) { + Thread.currentThread().interrupt(); } log.info("TaskGroupCoordinator closed"); } diff --git a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/workflow/serial/WorkflowSerialCoordinator.java b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/workflow/serial/WorkflowSerialCoordinator.java index 64b498cb5a13..f743c7b0e06c 100644 --- a/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/workflow/serial/WorkflowSerialCoordinator.java +++ b/dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/workflow/serial/WorkflowSerialCoordinator.java @@ -65,10 +65,9 @@ public class WorkflowSerialCoordinator implements IWorkflowSerialCoordinator { @Autowired private SerialCommandPriorityHandler serialCommandPriorityHandler; - // Desired lifecycle state; a canceled thread must finish before a requested restart. private volatile boolean flag = false; - private CoordinatorThread internalThread; + private Thread internalThread; private static final int DEFAULT_FETCH_SIZE = 1000; @@ -80,53 +79,28 @@ public synchronized void start() { if (flag) { throw new IllegalStateException("WorkflowSerialCoordinator is already started"); } - flag = true; - if (internalThread == null) { - startInternalThread(); + if (internalThread != null) { + throw new IllegalStateException("InternalThread is already started"); } - // A canceled run may still be inside JDBC. Its finally block starts the replacement. - } - - private void startInternalThread() { - internalThread = new CoordinatorThread(); + flag = true; + internalThread = new BaseDaemonThread(this::doStart) { + }; + internalThread.setName("WorkflowSerialCoordinator-Thread"); internalThread.start(); log.info("WorkflowSerialCoordinator started..."); } - private final class CoordinatorThread extends BaseDaemonThread { - - private volatile boolean running = true; - - private CoordinatorThread() { - super("WorkflowSerialCoordinator-Thread"); - } - - @Override - public void run() { - try { - doStart(this); - } finally { - synchronized (WorkflowSerialCoordinator.this) { - internalThread = null; - if (flag) { - startInternalThread(); - } - } - } - } - } - - private void doStart(CoordinatorThread run) { - while (run.running) { + private void doStart() { + while (flag) { try { final StopWatch workflowSerialCoordinatorRoundCost = StopWatch.createStarted(); final List serialCommandsGroups = fetchSerialCommands(); - for (SerialCommandsGroup serialCommandsGroup : serialCommandsGroups) { - if (!run.running) { - return; + // Fetching or handling a group may outlive a stop request; check before handling the next group. + serialCommandsGroups.forEach(serialCommandsGroup -> { + if (flag) { + handleSerialCommand(serialCommandsGroup); } - handleSerialCommand(serialCommandsGroup); - } + }); log.debug("WorkflowSerialCoordinator handled SerialCommandsGroup size: {}, cost: {}/ms ", serialCommandsGroups.size(), workflowSerialCoordinatorRoundCost.getDuration().toMillis()); @@ -134,7 +108,7 @@ private void doStart(CoordinatorThread run) { log.error("WorkflowSerialCoordinator error", e); } finally { // sleep 5s - if (run.running) { + if (flag) { ThreadUtils.sleep(TimeUnit.SECONDS.toMillis(DEFAULT_FETCH_INTERVAL_SECONDS)); } } @@ -201,20 +175,35 @@ private SerialCommandsGroup createSerialCommandsGroup(SerialCommandDto serialCom .build(); } + @Override + public synchronized void requestStop() { + flag = false; + if (internalThread != null) { + internalThread.interrupt(); + } + } + @Override public synchronized void close() { - if (!flag) { - log.warn("WorkflowSerialCoordinator is already closed"); - return; + if (Thread.currentThread() == internalThread) { + throw new IllegalStateException("WorkflowSerialCoordinator cannot close its own worker thread"); } - flag = false; - try { - if (internalThread != null) { - internalThread.running = false; - internalThread.interrupt(); + // A prior stop request does not mean the worker has finished. + requestStop(); + boolean interrupted = false; + if (internalThread != null) { + // Keep start() waiting until the old worker has finished, including any in-flight JDBC call. + while (internalThread.isAlive()) { + try { + internalThread.join(); + } catch (InterruptedException ex) { + interrupted = true; + } } - } catch (Exception ex) { - log.error("Close internalThread failed", ex); + internalThread = null; + } + if (interrupted) { + Thread.currentThread().interrupt(); } log.info("WorkflowSerialCoordinator closed"); } diff --git a/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/MasterCoordinatorTest.java b/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/MasterCoordinatorTest.java new file mode 100644 index 000000000000..a4c3dd7b5dd3 --- /dev/null +++ b/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/MasterCoordinatorTest.java @@ -0,0 +1,206 @@ +/* + * 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.dolphinscheduler.server.master.engine; + +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.apache.dolphinscheduler.common.enums.WorkflowExecutionTypeEnum; +import org.apache.dolphinscheduler.common.thread.ThreadUtils; +import org.apache.dolphinscheduler.dao.entity.WorkflowDefinitionLog; +import org.apache.dolphinscheduler.dao.model.SerialCommandDto; +import org.apache.dolphinscheduler.dao.repository.SerialCommandDao; +import org.apache.dolphinscheduler.dao.repository.TaskGroupDao; +import org.apache.dolphinscheduler.dao.repository.WorkflowDefinitionLogDao; +import org.apache.dolphinscheduler.registry.api.Registry; +import org.apache.dolphinscheduler.server.master.config.MasterConfig; +import org.apache.dolphinscheduler.server.master.engine.workflow.serial.SerialCommandDiscardHandler; +import org.apache.dolphinscheduler.server.master.engine.workflow.serial.SerialCommandPriorityHandler; +import org.apache.dolphinscheduler.server.master.engine.workflow.serial.SerialCommandWaitHandler; +import org.apache.dolphinscheduler.server.master.engine.workflow.serial.WorkflowSerialCoordinator; +import org.apache.dolphinscheduler.server.master.failover.IFailoverCoordinator; +import org.apache.dolphinscheduler.server.master.utils.MasterThreadFactory; + +import java.util.Collections; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.MockedStatic; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; +import org.springframework.test.util.ReflectionTestUtils; + +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.LENIENT) +class MasterCoordinatorTest { + + @InjectMocks + private WorkflowSerialCoordinator workflowSerialCoordinator; + + @Mock + private SerialCommandDao serialCommandDao; + + @Mock + private WorkflowDefinitionLogDao workflowDefinitionLogDao; + + @Mock + private SerialCommandWaitHandler serialCommandWaitHandler; + + @Mock + private SerialCommandDiscardHandler serialCommandDiscardHandler; + + @Mock + private SerialCommandPriorityHandler serialCommandPriorityHandler; + + @Test + void closeShouldStopBothWorkersBeforeWaitingForEither() throws Exception { + TaskGroupCoordinator taskGroupCoordinator = new TaskGroupCoordinator(); + TaskGroupDao taskGroupDao = Mockito.mock(TaskGroupDao.class); + ReflectionTestUtils.setField(taskGroupCoordinator, "taskGroupDao", taskGroupDao); + ScheduledExecutorService scheduler = Mockito.mock(ScheduledExecutorService.class); + ScheduledFuture scheduled = Mockito.mock(ScheduledFuture.class); + Mockito.doReturn(scheduled).when(scheduler).scheduleWithFixedDelay( + Mockito.any(Runnable.class), Mockito.anyLong(), Mockito.anyLong(), Mockito.any(TimeUnit.class)); + ExecutorService electionExecutor = Executors.newFixedThreadPool(1, runnable -> { + Thread worker = new Thread(() -> { + // The listener schedules failover work on the election worker, not the test thread. + try (MockedStatic factory = Mockito.mockStatic(MasterThreadFactory.class)) { + factory.when(MasterThreadFactory::getDefaultSchedulerThreadExecutor).thenReturn(scheduler); + runnable.run(); + } + }, "test-master-election"); + worker.setDaemon(true); + return worker; + }); + Registry registry = Mockito.mock(Registry.class); + Mockito.when(registry.acquireLock(Mockito.anyString())).thenReturn(true); + MasterConfig config = new MasterConfig(); + config.setMasterAddress("master-0:5678"); + MasterCoordinator masterCoordinator; + try ( + MockedStatic threadUtils = + Mockito.mockStatic(ThreadUtils.class, Mockito.CALLS_REAL_METHODS)) { + threadUtils.when(() -> ThreadUtils.newDaemonFixedThreadExecutor("HA-Election-%d", 1)) + .thenReturn(electionExecutor); + masterCoordinator = new MasterCoordinator(registry, config, taskGroupCoordinator, + Mockito.mock(IFailoverCoordinator.class), workflowSerialCoordinator); + } + CountDownLatch taskGroupFetching = new CountDownLatch(1); + CountDownLatch serialFetching = new CountDownLatch(1); + CountDownLatch taskGroupCanceled = new CountDownLatch(1); + CountDownLatch serialCanceled = new CountDownLatch(1); + CountDownLatch releaseTaskGroup = new CountDownLatch(1); + CountDownLatch releaseSerial = new CountDownLatch(1); + CountDownLatch closed = new CountDownLatch(1); + CountDownLatch failoverCanceled = new CountDownLatch(1); + Mockito.when(scheduled.cancel(true)).thenAnswer(invocation -> { + failoverCanceled.countDown(); + return true; + }); + AtomicReference taskGroupWorker = new AtomicReference<>(); + AtomicReference serialWorker = new AtomicReference<>(); + AtomicReference failure = new AtomicReference<>(); + Mockito.when(taskGroupDao.queryAllTaskGroups()).thenAnswer(invocation -> { + taskGroupWorker.set(Thread.currentThread()); + taskGroupFetching.countDown(); + awaitDatabaseResponse(releaseTaskGroup, taskGroupCanceled, failure); + return Collections.emptyList(); + }); + Mockito.when(serialCommandDao.fetchSerialCommands(Mockito.anyInt())).thenAnswer(invocation -> { + serialWorker.set(Thread.currentThread()); + serialFetching.countDown(); + awaitDatabaseResponse(releaseSerial, serialCanceled, failure); + return Collections.singletonList(SerialCommandDto.builder() + .workflowDefinitionCode(1L).workflowDefinitionVersion(1).build()); + }); + WorkflowDefinitionLog definition = new WorkflowDefinitionLog(); + definition.setExecutionType(WorkflowExecutionTypeEnum.SERIAL_WAIT); + Mockito.when(workflowDefinitionLogDao.queryByDefinitionCodeAndVersion(1L, 1)).thenReturn(definition); + Thread closer = new Thread(() -> { + try { + masterCoordinator.close(); + } catch (Throwable ex) { + failure.compareAndSet(null, ex); + } finally { + closed.countDown(); + } + }); + closer.setDaemon(true); + try { + masterCoordinator.start(); + Assertions.assertTrue(serialFetching.await(5, TimeUnit.SECONDS)); + // Keep the real one-minute TaskGroup startup delay; both lifecycle implementations participate. + Assertions.assertTrue(taskGroupFetching.await(90, TimeUnit.SECONDS)); + closer.start(); + Assertions.assertTrue(taskGroupCanceled.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(serialCanceled.await(5, TimeUnit.SECONDS)); + releaseSerial.countDown(); + serialWorker.get().join(5000); + Assertions.assertFalse(serialWorker.get().isAlive()); + // A stop request alone must not make a new generation eligible to start. + assertThrows(IllegalStateException.class, workflowSerialCoordinator::start); + Mockito.verifyNoInteractions(serialCommandWaitHandler); + // Serial has stopped while TaskGroup is still blocked; close must still join TaskGroup. + Assertions.assertTrue(taskGroupWorker.get().isAlive()); + Assertions.assertEquals(1L, closed.getCount()); + Assertions.assertTrue(failoverCanceled.await(5, TimeUnit.SECONDS)); + Mockito.verify(scheduled).cancel(true); + releaseTaskGroup.countDown(); + Assertions.assertTrue(closed.await(5, TimeUnit.SECONDS)); + Assertions.assertFalse(taskGroupWorker.get().isAlive()); + Assertions.assertNull(failure.get()); + } finally { + releaseSerial.countDown(); + releaseTaskGroup.countDown(); + closer.join(5000); + masterCoordinator.close(); + // Also release Serial if a regression in the Master entry point omitted it. + workflowSerialCoordinator.close(); + Assertions.assertTrue(electionExecutor.awaitTermination(5, TimeUnit.SECONDS)); + } + } + + private static void awaitDatabaseResponse(CountDownLatch release, CountDownLatch canceled, + AtomicReference failure) { + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(100); + while (release.getCount() != 0) { + try { + long remaining = deadline - System.nanoTime(); + if (remaining <= 0 || !release.await(remaining, TimeUnit.NANOSECONDS)) { + failure.compareAndSet(null, new AssertionError("Blocked DAO was not released")); + return; + } + } catch (InterruptedException ignored) { + // JDBC may ignore cancellation; only the test-controlled response releases the call. + canceled.countDown(); + } + } + } + +} diff --git a/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/TaskGroupCoordinatorTest.java b/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/TaskGroupCoordinatorTest.java index 7a50bfe8b804..878742853e46 100644 --- a/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/TaskGroupCoordinatorTest.java +++ b/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/TaskGroupCoordinatorTest.java @@ -50,7 +50,6 @@ import org.mockito.junit.jupiter.MockitoExtension; import org.mockito.junit.jupiter.MockitoSettings; import org.mockito.quality.Strictness; -import org.springframework.test.util.ReflectionTestUtils; import com.google.common.collect.Lists; @@ -177,89 +176,143 @@ void releaseTaskGroupSlot() { } @Test - void restartShouldWaitForPreviousFetchToReturn() throws Exception { - verifyRestartDuringFetch(false); + void closeShouldWaitForPreviousFetchBeforeRestart() throws Exception { + verifyCloseDuringFetch(false); } @Test - void closeShouldCancelPendingRestart() throws Exception { - verifyRestartDuringFetch(true); + void interruptedCloseShouldFinishWaitingAndRestoreInterrupt() throws Exception { + verifyCloseDuringFetch(true); } - private void verifyRestartDuringFetch(boolean cancelRestart) throws Exception { - taskGroupCoordinator = Mockito.spy(taskGroupCoordinator); - Mockito.doNothing().when(taskGroupCoordinator).pauseBeforeStart(); + private void verifyCloseDuringFetch(boolean interruptCloser) throws Exception { CountDownLatch fetching = new CountDownLatch(1); + CountDownLatch canceled = new CountDownLatch(1); CountDownLatch releaseFetch = new CountDownLatch(1); CountDownLatch restarted = new CountDownLatch(1); - AtomicReference workerFailure = new AtomicReference<>(); + CountDownLatch closed = new CountDownLatch(1); + AtomicReference failure = new AtomicReference<>(); AtomicReference firstThread = new AtomicReference<>(); Mockito.when(taskGroupDao.queryAllTaskGroups()).thenAnswer(invocation -> { if (firstThread.compareAndSet(null, Thread.currentThread())) { fetching.countDown(); - // JDBC calls can finish after interrupt. Keep the previous run inside its DAO call. - boolean released = false; - while (!released) { + // Model a JDBC request that returns only after the server responds, despite interruption. + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(10); + while (releaseFetch.getCount() != 0) { try { - released = releaseFetch.await(5, TimeUnit.SECONDS); - if (!released) { - workerFailure.set(new AssertionError("Test did not release the blocked DAO")); + long remaining = deadline - System.nanoTime(); + if (remaining <= 0 || !releaseFetch.await(remaining, TimeUnit.NANOSECONDS)) { + failure.compareAndSet(null, new AssertionError("Blocked DAO was not released")); return Collections.emptyList(); } } catch (InterruptedException ignored) { - // Model a driver that does not cancel its request on interrupt. + canceled.countDown(); } } TaskGroup staleGroup = new TaskGroup(); staleGroup.setId(1); staleGroup.setUseSize(1); return Collections.singletonList(staleGroup); - } else { - if (firstThread.get() == Thread.currentThread()) { - workerFailure.set(new AssertionError("Old polling loop resumed")); - } - restarted.countDown(); } + if (Thread.currentThread() == firstThread.get()) { + failure.compareAndSet(null, new AssertionError("Canceled worker resumed polling")); + } + restarted.countDown(); return Collections.emptyList(); }); + Thread closer = new Thread(() -> { + try { + taskGroupCoordinator.close(); + Assertions.assertFalse(firstThread.get().isAlive(), "close must finish the old worker"); + Assertions.assertEquals(interruptCloser, Thread.currentThread().isInterrupted()); + } catch (Throwable ex) { + failure.compareAndSet(null, ex); + } finally { + closed.countDown(); + } + }); + Thread starter = new Thread(() -> { + try { + taskGroupCoordinator.start(); + } catch (Throwable ex) { + failure.compareAndSet(null, ex); + } + }); + closer.setDaemon(true); + starter.setDaemon(true); try { taskGroupCoordinator.start(); - Assertions.assertTrue(fetching.await(5, TimeUnit.SECONDS)); - taskGroupCoordinator.close(); - taskGroupCoordinator.start(); - Assertions.assertSame(firstThread.get(), ReflectionTestUtils.getField(taskGroupCoordinator, - "internalThread"), "Keep the old run until its DAO call returns"); - if (cancelRestart) { - taskGroupCoordinator.close(); + // Allow the coordinator's normal one-minute startup delay before observing the DAO call. + Assertions.assertTrue(fetching.await(90, TimeUnit.SECONDS)); + closer.start(); + Assertions.assertTrue(canceled.await(5, TimeUnit.SECONDS)); + if (interruptCloser) { + closer.interrupt(); } + starter.start(); + // The start call must wait on the lifecycle monitor while close drains the old request. + awaitBlocked(starter); + Assertions.assertEquals(1L, closed.getCount()); + Assertions.assertEquals(1L, restarted.getCount()); releaseFetch.countDown(); - firstThread.get().join(5000); - Assertions.assertFalse(firstThread.get().isAlive()); - if (cancelRestart) { - Assertions.assertEquals(1L, restarted.getCount()); - Assertions.assertNull(ReflectionTestUtils.getField(taskGroupCoordinator, "internalThread")); - } else { - Assertions.assertTrue(restarted.await(5, TimeUnit.SECONDS)); - } - Assertions.assertNull(workerFailure.get()); - // The old fetch returned a nonempty batch, but cancellation must discard it. + Assertions.assertTrue(closed.await(5, TimeUnit.SECONDS)); + // A new worker also observes the normal startup delay. + Assertions.assertTrue(restarted.await(90, TimeUnit.SECONDS)); + closer.join(5000); + starter.join(5000); + Assertions.assertFalse(closer.isAlive()); + Assertions.assertFalse(starter.isAlive()); + Assertions.assertNull(failure.get()); + // A nonempty batch fetched before cancellation must not be processed after it returns. Mockito.verify(taskGroupQueueDao, Mockito.never()).countUsingTaskGroupQueueByGroupId(Mockito.anyInt()); + taskGroupCoordinator.close(); + taskGroupCoordinator.close(); + taskGroupCoordinator.start(); + taskGroupCoordinator.close(); } finally { - Thread latestThread; - synchronized (taskGroupCoordinator) { - latestThread = (Thread) ReflectionTestUtils.getField(taskGroupCoordinator, "internalThread"); - taskGroupCoordinator.close(); - } + // Release external work before waiting for close; otherwise cleanup itself would deadlock. releaseFetch.countDown(); - if (latestThread != null) { - latestThread.join(5000); - Assertions.assertFalse(latestThread.isAlive()); - } - if (firstThread.get() != null) { - firstThread.get().join(5000); - Assertions.assertFalse(firstThread.get().isAlive()); + closer.join(5000); + starter.join(5000); + Assertions.assertFalse(closer.isAlive()); + Assertions.assertFalse(starter.isAlive()); + taskGroupCoordinator.close(); + } + } + + @Test + void workerShouldNotCloseItself() throws Exception { + CountDownLatch checked = new CountDownLatch(1); + AtomicReference failure = new AtomicReference<>(); + Mockito.when(taskGroupDao.queryAllTaskGroups()).thenAnswer(invocation -> { + try { + Assertions.assertThrows(IllegalStateException.class, () -> taskGroupCoordinator.close()); + } catch (Throwable ex) { + failure.compareAndSet(null, ex); + } finally { + checked.countDown(); } + return Collections.emptyList(); + }); + try { + taskGroupCoordinator.start(); + // Exercise the actual worker after its normal one-minute startup delay. + Assertions.assertTrue(checked.await(90, TimeUnit.SECONDS)); + Assertions.assertNull(failure.get()); + // Rejection must leave lifecycle state unchanged so the owner can still close the worker. + Assertions.assertThrows(IllegalStateException.class, () -> taskGroupCoordinator.start()); + } finally { + taskGroupCoordinator.close(); + } + } + + private static void awaitBlocked(Thread thread) { + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5); + while (thread.isAlive() && thread.getState() != Thread.State.BLOCKED + && System.nanoTime() < deadline) { + Thread.yield(); } + Assertions.assertEquals(Thread.State.BLOCKED, thread.getState()); } } diff --git a/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/workflow/serial/WorkflowSerialCoordinatorTest.java b/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/workflow/serial/WorkflowSerialCoordinatorTest.java index c9bdacb62baa..86119a4f4447 100644 --- a/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/workflow/serial/WorkflowSerialCoordinatorTest.java +++ b/dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/workflow/serial/WorkflowSerialCoordinatorTest.java @@ -24,16 +24,23 @@ import org.apache.dolphinscheduler.dao.entity.WorkflowDefinitionLog; import org.apache.dolphinscheduler.dao.model.SerialCommandDto; import org.apache.dolphinscheduler.dao.repository.SerialCommandDao; +import org.apache.dolphinscheduler.dao.repository.TaskGroupDao; import org.apache.dolphinscheduler.dao.repository.WorkflowDefinitionLogDao; +import org.apache.dolphinscheduler.registry.api.Event; +import org.apache.dolphinscheduler.registry.api.Registry; +import org.apache.dolphinscheduler.registry.api.SubscribeListener; +import org.apache.dolphinscheduler.registry.api.ha.AbstractHAServer; import org.apache.dolphinscheduler.server.master.engine.ITaskGroupCoordinator; import org.apache.dolphinscheduler.server.master.engine.MasterCoordinator; +import org.apache.dolphinscheduler.server.master.engine.TaskGroupCoordinator; import org.apache.dolphinscheduler.server.master.failover.IFailoverCoordinator; +import org.apache.dolphinscheduler.server.master.utils.MasterThreadFactory; -import java.util.Arrays; import java.util.Collections; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import org.junit.jupiter.api.Assertions; @@ -41,6 +48,7 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.InjectMocks; import org.mockito.Mock; +import org.mockito.MockedStatic; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; import org.mockito.junit.jupiter.MockitoSettings; @@ -98,180 +106,350 @@ void closeShouldBeIdempotent() { assertDoesNotThrow(() -> workflowSerialCoordinator.close()); } @Test - void restartShouldWaitForPreviousFetchToReturn() throws Exception { - verifyRestartDuringFetch(false); + void closeShouldWaitForPreviousFetchBeforeRestart() throws Exception { + verifyCloseDuringFetch(false); } @Test - void closeShouldCancelPendingRestart() throws Exception { - verifyRestartDuringFetch(true); + void interruptedCloseShouldFinishWaitingAndRestoreInterrupt() throws Exception { + verifyCloseDuringFetch(true); } - private void verifyRestartDuringFetch(boolean cancelRestart) throws Exception { + private void verifyCloseDuringFetch(boolean interruptCloser) throws Exception { CountDownLatch fetching = new CountDownLatch(1); + CountDownLatch canceled = new CountDownLatch(1); CountDownLatch releaseFetch = new CountDownLatch(1); CountDownLatch restarted = new CountDownLatch(1); - AtomicReference workerFailure = new AtomicReference<>(); + CountDownLatch closed = new CountDownLatch(1); + AtomicReference failure = new AtomicReference<>(); AtomicReference firstThread = new AtomicReference<>(); Mockito.when(serialCommandDao.fetchSerialCommands(Mockito.anyInt())).thenAnswer(invocation -> { if (firstThread.compareAndSet(null, Thread.currentThread())) { fetching.countDown(); - // JDBC calls can finish after interrupt. Keep the previous run inside its DAO call. - boolean released = false; - while (!released) { + // Model a JDBC request that returns only after the server responds, despite interruption. + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(10); + while (releaseFetch.getCount() != 0) { try { - released = releaseFetch.await(5, TimeUnit.SECONDS); - if (!released) { - workerFailure.set(new AssertionError("Test did not release the blocked DAO")); + long remaining = deadline - System.nanoTime(); + if (remaining <= 0 || !releaseFetch.await(remaining, TimeUnit.NANOSECONDS)) { + failure.compareAndSet(null, new AssertionError("Blocked DAO was not released")); return Collections.emptyList(); } } catch (InterruptedException ignored) { - // Model a driver that does not cancel its request on interrupt. + canceled.countDown(); } } return Collections.singletonList(SerialCommandDto.builder() .workflowDefinitionCode(1L).workflowDefinitionVersion(1).build()); - } else { - if (firstThread.get() == Thread.currentThread()) { - workerFailure.set(new AssertionError("Old polling loop resumed")); - } - restarted.countDown(); } + if (Thread.currentThread() == firstThread.get()) { + failure.compareAndSet(null, new AssertionError("Canceled worker resumed polling")); + } + restarted.countDown(); return Collections.emptyList(); }); - WorkflowDefinitionLog definition = - new WorkflowDefinitionLog(); + WorkflowDefinitionLog definition = new WorkflowDefinitionLog(); definition.setExecutionType(WorkflowExecutionTypeEnum.SERIAL_WAIT); Mockito.when(workflowDefinitionLogDao.queryByDefinitionCodeAndVersion(1L, 1)).thenReturn(definition); + Thread closer = new Thread(() -> { + try { + workflowSerialCoordinator.close(); + Assertions.assertFalse(firstThread.get().isAlive(), "close must finish the old worker"); + Assertions.assertEquals(interruptCloser, Thread.currentThread().isInterrupted()); + } catch (Throwable ex) { + failure.compareAndSet(null, ex); + } finally { + closed.countDown(); + } + }); + Thread starter = new Thread(() -> { + try { + workflowSerialCoordinator.start(); + } catch (Throwable ex) { + failure.compareAndSet(null, ex); + } + }); + closer.setDaemon(true); + starter.setDaemon(true); try { workflowSerialCoordinator.start(); Assertions.assertTrue(fetching.await(5, TimeUnit.SECONDS)); - workflowSerialCoordinator.close(); - workflowSerialCoordinator.start(); - Assertions.assertSame(firstThread.get(), ReflectionTestUtils.getField(workflowSerialCoordinator, - "internalThread"), "Keep the old run until its DAO call returns"); - if (cancelRestart) { - workflowSerialCoordinator.close(); + closer.start(); + Assertions.assertTrue(canceled.await(5, TimeUnit.SECONDS)); + if (interruptCloser) { + closer.interrupt(); } + starter.start(); + // The start call must wait on the lifecycle monitor while close drains the old request. + awaitBlocked(starter); + Assertions.assertEquals(1L, closed.getCount()); + Assertions.assertEquals(1L, restarted.getCount()); releaseFetch.countDown(); - firstThread.get().join(5000); - Assertions.assertFalse(firstThread.get().isAlive()); - if (cancelRestart) { - Assertions.assertEquals(1L, restarted.getCount()); - Assertions.assertNull(ReflectionTestUtils.getField(workflowSerialCoordinator, "internalThread")); - } else { - Assertions.assertTrue(restarted.await(5, TimeUnit.SECONDS)); - } - Assertions.assertNull(workerFailure.get()); - // The old fetch returned a nonempty batch, but cancellation must discard it. + Assertions.assertTrue(closed.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(restarted.await(5, TimeUnit.SECONDS)); + closer.join(5000); + starter.join(5000); + Assertions.assertFalse(closer.isAlive()); + Assertions.assertFalse(starter.isAlive()); + Assertions.assertNull(failure.get()); + // A nonempty batch fetched before cancellation must not be processed after it returns. Mockito.verifyNoInteractions(serialCommandWaitHandler); + workflowSerialCoordinator.close(); + workflowSerialCoordinator.close(); + workflowSerialCoordinator.start(); + workflowSerialCoordinator.close(); } finally { - Thread latestThread; - synchronized (workflowSerialCoordinator) { - latestThread = (Thread) ReflectionTestUtils.getField(workflowSerialCoordinator, "internalThread"); - workflowSerialCoordinator.close(); - } + // Release external work before waiting for close; otherwise cleanup itself would deadlock. releaseFetch.countDown(); - if (latestThread != null) { - latestThread.join(5000); - Assertions.assertFalse(latestThread.isAlive()); - } - if (firstThread.get() != null) { - firstThread.get().join(5000); - Assertions.assertFalse(firstThread.get().isAlive()); + closer.join(5000); + starter.join(5000); + Assertions.assertFalse(closer.isAlive()); + Assertions.assertFalse(starter.isAlive()); + workflowSerialCoordinator.close(); + } + } + + @Test + void workerShouldNotCloseItself() throws Exception { + CountDownLatch checked = new CountDownLatch(1); + AtomicReference failure = new AtomicReference<>(); + Mockito.when(serialCommandDao.fetchSerialCommands(Mockito.anyInt())).thenAnswer(invocation -> { + try { + Assertions.assertThrows(IllegalStateException.class, () -> workflowSerialCoordinator.close()); + } catch (Throwable ex) { + failure.compareAndSet(null, ex); + } finally { + checked.countDown(); } + return Collections.emptyList(); + }); + try { + workflowSerialCoordinator.start(); + Assertions.assertTrue(checked.await(5, TimeUnit.SECONDS)); + Assertions.assertNull(failure.get()); + // Rejection must leave lifecycle state unchanged so the owner can still close the worker. + Assertions.assertThrows(IllegalStateException.class, () -> workflowSerialCoordinator.start()); + } finally { + workflowSerialCoordinator.close(); } } @Test - void roleChangesShouldWaitForInFlightHandlerAndDiscardRemainingGroups() throws Exception { + void queuedRoleChangesShouldDrainOldWorkerBeforeReactivation() throws Exception { + Registry registry = Mockito.mock(Registry.class); + AtomicReference owner = new AtomicReference<>(); + AtomicReference subscriber = new AtomicReference<>(); + AbstractHAServer server = new AbstractHAServer(registry, "/coordinator", "master-0:5678") { + }; MasterCoordinator.MasterCoordinatorListener listener = new MasterCoordinator.MasterCoordinatorListener( Mockito.mock(ITaskGroupCoordinator.class), Mockito.mock(IFailoverCoordinator.class), - workflowSerialCoordinator); - CountDownLatch handling = new CountDownLatch(1); - CountDownLatch releaseHandler = new CountDownLatch(1); - CountDownLatch replacementHandled = new CountDownLatch(1); - CountDownLatch releaseReplacement = new CountDownLatch(1); - AtomicReference oldThread = new AtomicReference<>(); - AtomicReference workerFailure = new AtomicReference<>(); - AtomicInteger fetches = new AtomicInteger(); - AtomicInteger handled = new AtomicInteger(); + workflowSerialCoordinator) { + + @Override + public void changeToActive() { + // Static mocks are thread-local: intercept the factory on the HA worker itself. + try (MockedStatic factory = Mockito.mockStatic(MasterThreadFactory.class)) { + factory.when(MasterThreadFactory::getDefaultSchedulerThreadExecutor) + .thenReturn(Mockito.mock(ScheduledExecutorService.class)); + super.changeToActive(); + } + } + }; + server.addServerStatusChangeListener(listener); + Mockito.when(registry.acquireLock("/coordinator-lock")).thenReturn(true); + Mockito.when(registry.exists("/coordinator")).thenAnswer(invocation -> owner.get() != null); + Mockito.when(registry.get("/coordinator")).thenAnswer(invocation -> owner.get()); + Mockito.doAnswer(invocation -> { + owner.set(invocation.getArgument(1)); + return null; + }).when(registry).put(Mockito.eq("/coordinator"), Mockito.anyString(), Mockito.eq(true)); + Mockito.doAnswer(invocation -> { + subscriber.set(invocation.getArgument(1)); + return null; + }).when(registry).subscribe(Mockito.eq("/coordinator"), Mockito.any()); + + CountDownLatch fetching = new CountDownLatch(1); + CountDownLatch canceled = new CountDownLatch(1); + CountDownLatch releaseFetch = new CountDownLatch(1); + CountDownLatch restarted = new CountDownLatch(1); + CountDownLatch callbacksReturned = new CountDownLatch(1); + AtomicReference oldWorker = new AtomicReference<>(); + AtomicReference failure = new AtomicReference<>(); WorkflowDefinitionLog definition = new WorkflowDefinitionLog(); definition.setExecutionType(WorkflowExecutionTypeEnum.SERIAL_WAIT); - Mockito.when(workflowDefinitionLogDao.queryByDefinitionCodeAndVersion(Mockito.anyLong(), Mockito.anyInt())) - .thenReturn(definition); - SerialCommandDto first = SerialCommandDto.builder().workflowDefinitionCode(1L) - .workflowDefinitionVersion(1).build(); - SerialCommandDto second = SerialCommandDto.builder().workflowDefinitionCode(2L) - .workflowDefinitionVersion(1).build(); + Mockito.when(workflowDefinitionLogDao.queryByDefinitionCodeAndVersion(1L, 1)).thenReturn(definition); Mockito.when(serialCommandDao.fetchSerialCommands(Mockito.anyInt())).thenAnswer(invocation -> { - if (fetches.incrementAndGet() == 1) { - return Arrays.asList(first, second); - } - return Collections.singletonList(first); - }); - Mockito.doAnswer(invocation -> { - if (handled.incrementAndGet() == 1) { - oldThread.set(Thread.currentThread()); - handling.countDown(); - // A handler already executing cannot be forcibly canceled. Its successor must wait. - while (true) { + if (oldWorker.compareAndSet(null, Thread.currentThread())) { + fetching.countDown(); + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(10); + while (releaseFetch.getCount() != 0) { try { - if (!releaseHandler.await(5, TimeUnit.SECONDS)) { - workerFailure.set(new AssertionError("Handler was not released")); + long remaining = deadline - System.nanoTime(); + if (remaining <= 0 || !releaseFetch.await(remaining, TimeUnit.NANOSECONDS)) { + failure.compareAndSet(null, new AssertionError("Blocked DAO was not released")); + return Collections.emptyList(); } - break; } catch (InterruptedException ignored) { - // Model an in-flight database request ignoring interruption. + canceled.countDown(); } } - } else { - if (Thread.currentThread() == oldThread.get()) { - workerFailure.set(new AssertionError("Canceled run handled another group")); - } - replacementHandled.countDown(); - // Keep the successor at the observation point instead of relying on the polling interval. - try { - releaseReplacement.await(); - } catch (InterruptedException ignored) { - // Test cleanup closes the successor before releasing this latch. - } + return Collections.singletonList(SerialCommandDto.builder() + .workflowDefinitionCode(1L).workflowDefinitionVersion(1).build()); } - return null; - }).when(serialCommandWaitHandler).handle(Mockito.any()); + if (Thread.currentThread() == oldWorker.get()) { + failure.compareAndSet(null, new AssertionError("Canceled worker resumed polling")); + } + restarted.countDown(); + return Collections.emptyList(); + }); + Thread callbacks = new Thread(() -> { + try { + owner.set("master-1:5678#peer"); + subscriber.get().notify(new Event("/coordinator", "/coordinator", "", Event.Type.REMOVE)); + Assertions.assertTrue(canceled.await(5, TimeUnit.SECONDS)); + // The HA worker is draining the old DAO call. A second notification must still return. + owner.set(null); + subscriber.get().notify(new Event("/coordinator", "/coordinator", "", Event.Type.REMOVE)); + } catch (Throwable ex) { + failure.compareAndSet(null, ex); + } finally { + callbacksReturned.countDown(); + } + }); + callbacks.setDaemon(true); try { - listener.changeToActive(); - Assertions.assertTrue(handling.await(5, TimeUnit.SECONDS)); - listener.changeToStandBy(); - listener.changeToActive(); - listener.changeToStandBy(); - listener.changeToActive(); - Assertions.assertSame(oldThread.get(), ReflectionTestUtils.getField(workflowSerialCoordinator, - "internalThread")); - Assertions.assertEquals(1, fetches.get()); - releaseHandler.countDown(); - Assertions.assertTrue(replacementHandled.await(5, TimeUnit.SECONDS)); - oldThread.get().join(5000); - Assertions.assertFalse(oldThread.get().isAlive()); - Assertions.assertNull(workerFailure.get()); - Assertions.assertEquals(2, handled.get()); + server.start(); + Assertions.assertTrue(fetching.await(5, TimeUnit.SECONDS)); + callbacks.start(); + Assertions.assertTrue(callbacksReturned.await(5, TimeUnit.SECONDS)); + Assertions.assertNull(failure.get()); + Assertions.assertFalse(server.isActive()); + Assertions.assertTrue(oldWorker.get().isAlive()); + Assertions.assertEquals(1L, restarted.getCount()); + releaseFetch.countDown(); + Assertions.assertTrue(restarted.await(5, TimeUnit.SECONDS)); + Assertions.assertFalse(oldWorker.get().isAlive()); + Assertions.assertTrue(server.isActive()); + Assertions.assertNull(failure.get()); + Mockito.verifyNoInteractions(serialCommandWaitHandler); } finally { - Thread latestThread; - synchronized (workflowSerialCoordinator) { - latestThread = (Thread) ReflectionTestUtils.getField(workflowSerialCoordinator, "internalThread"); + releaseFetch.countDown(); + callbacks.join(5000); + Assertions.assertFalse(callbacks.isAlive()); + server.close(); + // Also cancel the listener's scheduled failover task, including when an assertion fails. + listener.changeToStandBy(); + } + } + + @Test + void demotionShouldStopBothWorkersBeforeWaitingForEither() throws Exception { + TaskGroupCoordinator taskGroupCoordinator = new TaskGroupCoordinator(); + TaskGroupDao taskGroupDao = Mockito.mock(TaskGroupDao.class); + ReflectionTestUtils.setField(taskGroupCoordinator, "taskGroupDao", taskGroupDao); + MasterCoordinator.MasterCoordinatorListener listener = new MasterCoordinator.MasterCoordinatorListener( + taskGroupCoordinator, Mockito.mock(IFailoverCoordinator.class), workflowSerialCoordinator); + ScheduledExecutorService scheduler = Mockito.mock(ScheduledExecutorService.class); + ScheduledFuture scheduled = Mockito.mock(ScheduledFuture.class); + Mockito.doReturn(scheduled).when(scheduler).scheduleWithFixedDelay( + Mockito.any(Runnable.class), Mockito.anyLong(), Mockito.anyLong(), Mockito.any(TimeUnit.class)); + CountDownLatch taskGroupFetching = new CountDownLatch(1); + CountDownLatch serialFetching = new CountDownLatch(1); + CountDownLatch taskGroupCanceled = new CountDownLatch(1); + CountDownLatch serialCanceled = new CountDownLatch(1); + CountDownLatch releaseTaskGroup = new CountDownLatch(1); + CountDownLatch releaseSerial = new CountDownLatch(1); + CountDownLatch closed = new CountDownLatch(1); + CountDownLatch failoverCanceled = new CountDownLatch(1); + Mockito.when(scheduled.cancel(true)).thenAnswer(invocation -> { + failoverCanceled.countDown(); + return true; + }); + AtomicReference taskGroupWorker = new AtomicReference<>(); + AtomicReference serialWorker = new AtomicReference<>(); + AtomicReference failure = new AtomicReference<>(); + Mockito.when(taskGroupDao.queryAllTaskGroups()).thenAnswer(invocation -> { + taskGroupWorker.set(Thread.currentThread()); + taskGroupFetching.countDown(); + awaitDatabaseResponse(releaseTaskGroup, taskGroupCanceled, failure); + return Collections.emptyList(); + }); + Mockito.when(serialCommandDao.fetchSerialCommands(Mockito.anyInt())).thenAnswer(invocation -> { + serialWorker.set(Thread.currentThread()); + serialFetching.countDown(); + awaitDatabaseResponse(releaseSerial, serialCanceled, failure); + return Collections.singletonList(SerialCommandDto.builder() + .workflowDefinitionCode(1L).workflowDefinitionVersion(1).build()); + }); + WorkflowDefinitionLog definition = new WorkflowDefinitionLog(); + definition.setExecutionType(WorkflowExecutionTypeEnum.SERIAL_WAIT); + Mockito.when(workflowDefinitionLogDao.queryByDefinitionCodeAndVersion(1L, 1)).thenReturn(definition); + Thread closer = new Thread(() -> { + try { listener.changeToStandBy(); + } catch (Throwable ex) { + failure.compareAndSet(null, ex); + } finally { + closed.countDown(); } - releaseHandler.countDown(); - releaseReplacement.countDown(); - if (latestThread != null) { - latestThread.join(5000); - Assertions.assertFalse(latestThread.isAlive()); - } - if (oldThread.get() != null) { - oldThread.get().join(5000); - Assertions.assertFalse(oldThread.get().isAlive()); + }); + closer.setDaemon(true); + try (MockedStatic factory = Mockito.mockStatic(MasterThreadFactory.class)) { + factory.when(MasterThreadFactory::getDefaultSchedulerThreadExecutor).thenReturn(scheduler); + listener.changeToActive(); + Assertions.assertTrue(serialFetching.await(5, TimeUnit.SECONDS)); + // Keep the real one-minute TaskGroup startup delay; both lifecycle implementations participate. + Assertions.assertTrue(taskGroupFetching.await(90, TimeUnit.SECONDS)); + closer.start(); + Assertions.assertTrue(taskGroupCanceled.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(serialCanceled.await(5, TimeUnit.SECONDS)); + releaseSerial.countDown(); + serialWorker.get().join(5000); + Assertions.assertFalse(serialWorker.get().isAlive()); + // A stop request alone must not make a new generation eligible to start. + assertThrows(IllegalStateException.class, workflowSerialCoordinator::start); + Mockito.verifyNoInteractions(serialCommandWaitHandler); + // Serial has stopped while TaskGroup is still blocked; close must still join TaskGroup. + Assertions.assertTrue(taskGroupWorker.get().isAlive()); + Assertions.assertEquals(1L, closed.getCount()); + Assertions.assertTrue(failoverCanceled.await(5, TimeUnit.SECONDS)); + Mockito.verify(scheduled).cancel(true); + releaseTaskGroup.countDown(); + Assertions.assertTrue(closed.await(5, TimeUnit.SECONDS)); + Assertions.assertFalse(taskGroupWorker.get().isAlive()); + Assertions.assertNull(failure.get()); + } finally { + releaseSerial.countDown(); + releaseTaskGroup.countDown(); + closer.join(5000); + listener.changeToStandBy(); + } + } + + private static void awaitDatabaseResponse(CountDownLatch release, CountDownLatch canceled, + AtomicReference failure) { + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(100); + while (release.getCount() != 0) { + try { + long remaining = deadline - System.nanoTime(); + if (remaining <= 0 || !release.await(remaining, TimeUnit.NANOSECONDS)) { + failure.compareAndSet(null, new AssertionError("Blocked DAO was not released")); + return; + } + } catch (InterruptedException ignored) { + // JDBC may ignore cancellation; only the test-controlled response releases the call. + canceled.countDown(); } } } + private static void awaitBlocked(Thread thread) { + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5); + while (thread.isAlive() && thread.getState() != Thread.State.BLOCKED + && System.nanoTime() < deadline) { + Thread.yield(); + } + Assertions.assertEquals(Thread.State.BLOCKED, thread.getState()); + } + } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServer.java b/dolphinscheduler-registry/dolphinscheduler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServer.java index 4846d87e0316..9cb762fcf70f 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServer.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServer.java @@ -25,9 +25,13 @@ import org.apache.dolphinscheduler.registry.api.SubscribeListener; import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.RejectedExecutionException; import lombok.extern.slf4j.Slf4j; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.Lists; @Slf4j @@ -41,6 +45,8 @@ public abstract class AbstractHAServer implements HAServer { private volatile ServerStatus serverStatus; + private final ExecutorService electionExecutor; + private final List serverStatusChangeListeners; private static final long DEFAULT_RETRY_INTERVAL = 5_000; @@ -48,6 +54,14 @@ public abstract class AbstractHAServer implements HAServer { private static final int DEFAULT_MAX_RETRY_TIMES = 20; public AbstractHAServer(final Registry registry, final String selectorPath, final String serverIdentify) { + this(registry, selectorPath, serverIdentify, + ThreadUtils.newDaemonFixedThreadExecutor("HA-Election-%d", 1)); + } + + @VisibleForTesting + AbstractHAServer(final Registry registry, final String selectorPath, final String serverIdentify, + final ExecutorService electionExecutor) { + this.electionExecutor = electionExecutor; this.registry = registry; this.selectorPath = checkNotNull(selectorPath); // Include the creation time to distinguish restarts at the same address. @@ -58,28 +72,65 @@ public AbstractHAServer(final Registry registry, final String selectorPath, fina @Override public void start() { - registry.subscribe(selectorPath, new SubscribeListener() { + try { + registry.subscribe(selectorPath, new SubscribeListener() { - @Override - public void notify(Event event) { - if (Event.Type.REMOVE.equals(event.getType())) { - reconcileElection(); + @Override + public void notify(Event event) { + if (Event.Type.REMOVE.equals(event.getType())) { + enqueueElection(); + } } - } - @Override - public SubscribeScope getSubscribeScope() { - return SubscribeScope.PATH_ONLY; + @Override + public SubscribeScope getSubscribeScope() { + return SubscribeScope.PATH_ONLY; + } + }); + // Preserve startup completion and failure semantics; callbacks only enqueue work. + electionExecutor.submit(this::reconcileElection).get(); + } catch (InterruptedException e) { + close(); + Thread.currentThread().interrupt(); + throw new IllegalStateException("Interrupted while starting HA server", e); + } catch (ExecutionException e) { + close(); + if (e.getCause() instanceof RuntimeException) { + throw (RuntimeException) e.getCause(); } - }); + throw new IllegalStateException("Failed to start HA server", e.getCause()); + } catch (RuntimeException e) { + close(); + throw e; + } + } - reconcileElection(); + private void enqueueElection() { + if (electionExecutor.isShutdown()) { + return; + } + try { + electionExecutor.execute(() -> { + try { + reconcileElection(); + } catch (Exception e) { + log.error("Failed to reconcile HA ownership for {}", serverIdentify, e); + } + }); + } catch (RejectedExecutionException e) { + // The Registry can deliver callbacks concurrently with close(). + if (!electionExecutor.isShutdown()) { + throw e; + } + } } - private synchronized void reconcileElection() { - // Serialize election and publication with callbacks, including callbacks during startup. - // REMOVE may be delayed or have no previous value, so consult current ownership instead. + private void reconcileElection() { + // REMOVE is a request to recheck ownership, not a role decision to replay later. boolean elected = participateElection(); + if (electionExecutor.isShutdown()) { + return; + } if (elected) { statusChange(ServerStatus.ACTIVE); } else { @@ -88,37 +139,56 @@ private synchronized void reconcileElection() { } } + @Override + public void close() { + // Let queued startup futures finish as no-ops. shutdownNow would strand their callers. + electionExecutor.shutdown(); + synchronized (this) { + // Wait for any publication already in progress, without joining the event worker: + // an Alert listener can close this server from that worker itself. + } + } + @Override public boolean isActive() { return ServerStatus.ACTIVE.equals(getServerStatus()); } @Override - public synchronized boolean participateElection() { + public boolean participateElection() { final String electionLock = selectorPath + "-lock"; // If meet exception during participate election, will retry. // This can avoid the situation that the server is not elected as leader due to network jitter. for (int i = 0; i < DEFAULT_MAX_RETRY_TIMES; i++) { - boolean lockAcquired = false; + if (electionExecutor.isShutdown()) { + return false; + } try { + if (!registry.acquireLock(electionLock)) { + return false; + } try { - lockAcquired = registry.acquireLock(electionLock); - if (lockAcquired) { - if (!registry.exists(selectorPath)) { - registry.put(selectorPath, serverIdentify, true); - return true; - } + if (electionExecutor.isShutdown()) { + return false; + } + boolean selectorExists = registry.exists(selectorPath); + if (electionExecutor.isShutdown()) { + return false; + } + if (selectorExists) { return serverIdentify.equals(registry.get(selectorPath)); } - return false; + registry.put(selectorPath, serverIdentify, true); + return true; } finally { - if (lockAcquired) { - registry.releaseLock(electionLock); - } + registry.releaseLock(electionLock); } } catch (Exception e) { log.error("Participate election error, meet an exception, will retry after {}ms", DEFAULT_RETRY_INTERVAL, e); + if (electionExecutor.isShutdown()) { + return false; + } ThreadUtils.sleep(DEFAULT_RETRY_INTERVAL); } } @@ -136,11 +206,20 @@ public ServerStatus getServerStatus() { return serverStatus; } + // Use the same monitor as external close() so it waits for ongoing status updates and listener calls. private synchronized void statusChange(ServerStatus targetStatus) { + if (electionExecutor.isShutdown()) { + return; + } final ServerStatus originStatus = serverStatus; serverStatus = targetStatus; try { - serverStatusChangeListeners.forEach(listener -> listener.change(originStatus, targetStatus)); + serverStatusChangeListeners.forEach(listener -> { + // A listener may close this server; do not invoke subsequent listeners after that. + if (!electionExecutor.isShutdown()) { + listener.change(originStatus, targetStatus); + } + }); } catch (Exception ex) { log.error("Trigger ServerStatusChangeListener from {} -> {} error", originStatus, targetStatus, ex); } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-api/src/test/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServerTest.java b/dolphinscheduler-registry/dolphinscheduler-registry-api/src/test/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServerTest.java index 6f22a006f071..bb39e4ec857e 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-api/src/test/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServerTest.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-api/src/test/java/org/apache/dolphinscheduler/registry/api/ha/AbstractHAServerTest.java @@ -20,8 +20,10 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.CALLS_REAL_METHODS; @@ -38,17 +40,19 @@ import org.apache.dolphinscheduler.registry.api.Registry; import org.apache.dolphinscheduler.registry.api.SubscribeListener; -import java.lang.management.LockInfo; -import java.lang.management.ManagementFactory; -import java.lang.management.ThreadInfo; +import java.util.ArrayList; +import java.util.List; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.MockedStatic; @@ -59,6 +63,13 @@ class AbstractHAServerTest { private static final String ELECTION_LOCK = SELECTOR_PATH + "-lock"; private static final String ADDRESS = "master-0:5678"; + private final List servers = new ArrayList<>(); + private final List electionExecutors = new ArrayList<>(); + private final AtomicReference retryAction = new AtomicReference<>(() -> { + }); + private ExecutorService electionExecutor; + private final AtomicReference workerFailure = new AtomicReference<>(); + private Registry registry; private AtomicReference owner; private AtomicReference subscriber; @@ -72,6 +83,7 @@ void setUp() { subscriber = new AtomicReference<>(); statusListener = mock(AbstractServerStatusChangeListener.class, CALLS_REAL_METHODS); server = newServer(); + electionExecutor = electionExecutors.get(0); server.addServerStatusChangeListener(statusListener); when(registry.acquireLock(ELECTION_LOCK)).thenReturn(true); when(registry.exists(SELECTOR_PATH)).thenAnswer(invocation -> owner.get() != null); @@ -86,6 +98,15 @@ void setUp() { }).when(registry).subscribe(eq(SELECTOR_PATH), org.mockito.ArgumentMatchers.any()); } + @AfterEach + void tearDown() throws Exception { + servers.forEach(AbstractHAServer::close); + for (ExecutorService executor : electionExecutors) { + assertTrue(executor.awaitTermination(5, TimeUnit.SECONDS)); + } + assertNoWorkerFailure(); + } + @Test void testInitialLeaderAndFollower() { server.start(); @@ -177,6 +198,7 @@ void testAddAndUpdateDoNotTriggerElection() { owner.set(null); subscriber.get().notify(new Event(SELECTOR_PATH, SELECTOR_PATH, "", Event.Type.ADD)); subscriber.get().notify(new Event(SELECTOR_PATH, SELECTOR_PATH, "", Event.Type.UPDATE)); + drain(); assertFalse(server.isActive()); verify(registry, times(1)).acquireLock(ELECTION_LOCK); verify(statusListener, never()).changeToActive(); @@ -204,9 +226,8 @@ void testActiveServerDemotesWhenLockCannotBeAcquired() { @Test void testAcquisitionErrorDoesNotReleaseUnacquiredLock() { when(registry.acquireLock(ELECTION_LOCK)).thenThrow(new IllegalStateException("lock unavailable")); - try (MockedStatic ignored = mockStatic(ThreadUtils.class)) { - assertThrows(IllegalStateException.class, server::start); - } + assertThrows(IllegalStateException.class, server::start); + assertTrue(electionExecutor.isShutdown()); assertFalse(server.isActive()); verify(registry, never()).releaseLock(ELECTION_LOCK); verify(statusListener, never()).changeToActive(); @@ -218,14 +239,11 @@ void testTransientElectionErrorKeepsRoleUntilOwnershipDecision() { when(registry.exists(SELECTOR_PATH)) .thenThrow(new IllegalStateException("temporary registry failure")) .thenReturn(true); - try (MockedStatic threadUtils = mockStatic(ThreadUtils.class)) { - threadUtils.when(() -> ThreadUtils.sleep(5_000)).thenAnswer(invocation -> { - assertTrue(server.isActive()); - verify(statusListener, never()).changeToStandBy(); - return null; - }); - remove(""); - } + retryAction.set(() -> { + assertTrue(server.isActive()); + verify(statusListener, never()).changeToStandBy(); + }); + remove(""); assertTrue(server.isActive()); verify(statusListener, times(1)).changeToActive(); verify(statusListener, never()).changeToStandBy(); @@ -235,9 +253,8 @@ void testTransientElectionErrorKeepsRoleUntilOwnershipDecision() { void testExhaustedRetriesPreserveOriginalRole() { server.start(); when(registry.exists(SELECTOR_PATH)).thenThrow(new IllegalStateException("registry unavailable")); - try (MockedStatic ignored = mockStatic(ThreadUtils.class)) { - assertThrows(IllegalStateException.class, () -> remove("")); - } + // Asynchronous callback failures are logged by the worker, not thrown on the Registry thread. + remove(""); // Exception-driven demotion is deliberately outside this minimal candidate. assertTrue(server.isActive()); verify(statusListener, never()).changeToStandBy(); @@ -248,74 +265,293 @@ void testExhaustedRetriesPreserveOriginalRole() { void testRemoveCannotBeOverwrittenByEarlierStartupElection() throws Exception { CountDownLatch startupElectionFinished = new CountDownLatch(1); CountDownLatch allowStartupToReturn = new CountDownLatch(1); - CountDownLatch callbackStarted = new CountDownLatch(1); AtomicBoolean firstRelease = new AtomicBoolean(true); - AtomicReference callbackThread = new AtomicReference<>(); when(registry.releaseLock(ELECTION_LOCK)).thenAnswer(invocation -> { if (firstRelease.getAndSet(false)) { - // Pause after the successful election but before startup publishes ACTIVE. + // Pause after election, before publication; the notification must queue behind it. startupElectionFinished.countDown(); assertTrue(allowStartupToReturn.await(5, TimeUnit.SECONDS)); } return true; }); - ExecutorService executor = Executors.newFixedThreadPool(2); + ExecutorService caller = Executors.newSingleThreadExecutor(); try { - Future startup = executor.submit(server::start); + Future startup = caller.submit(server::start); assertTrue(startupElectionFinished.await(5, TimeUnit.SECONDS)); - String previousOwner = owner.get(); owner.set("master-1:5678#peer-instance"); - Future callback = executor.submit(() -> { - callbackThread.set(Thread.currentThread()); - callbackStarted.countDown(); - remove(previousOwner); - }); - assertTrue(callbackStarted.await(5, TimeUnit.SECONDS)); - // Wait for actual monitor contention (fixed code), or completion (old code). - // This forces the relevant ordering rather than relying on a sleep or scheduler luck. - long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5); - while (!callback.isDone() && !isBlockedOnServer(callbackThread.get()) - && System.nanoTime() < deadline) { - Thread.yield(); - } - assertTrue(callback.isDone() || isBlockedOnServer(callbackThread.get())); + notifyRemove(""); allowStartupToReturn.countDown(); startup.get(5, TimeUnit.SECONDS); - callback.get(5, TimeUnit.SECONDS); + drain(); assertFalse(server.isActive()); verify(statusListener).changeToActive(); verify(statusListener).changeToStandBy(); } finally { allowStartupToReturn.countDown(); - executor.shutdownNow(); - assertTrue(executor.awaitTermination(5, TimeUnit.SECONDS)); + caller.shutdownNow(); + assertTrue(caller.awaitTermination(5, TimeUnit.SECONDS)); + } + } + + @Test + void testCallbacksReturnWhileRoleListenerIsBlockedAndRecheckCurrentOwner() throws Exception { + server.start(); + String ownIdentity = owner.get(); + CountDownLatch stopping = new CountDownLatch(1); + CountDownLatch allowStop = new CountDownLatch(1); + AtomicReference listenerThread = new AtomicReference<>(); + server.addServerStatusChangeListener(new AbstractServerStatusChangeListener() { + + @Override + public void changeToActive() { + } + + @Override + public void changeToStandBy() { + listenerThread.set(Thread.currentThread()); + stopping.countDown(); + await(allowStop); + } + }); + try { + owner.set("master-1:5678#peer-instance"); + notifyRemove(""); + assertTrue(stopping.await(5, TimeUnit.SECONDS)); + assertNotEquals(Thread.currentThread(), listenerThread.get()); + // Queue while this instance appears to own the key, then change the owner again. + // The queued request must re-read the Registry, not publish a captured ACTIVE decision. + owner.set(ownIdentity); + notifyRemove(""); + owner.set("master-2:5678#peer-instance"); + allowStop.countDown(); + drain(); + assertFalse(server.isActive()); + verify(statusListener, times(1)).changeToActive(); + } finally { + allowStop.countDown(); } } - private boolean isBlockedOnServer(Thread thread) { - ThreadInfo threadInfo = ManagementFactory.getThreadMXBean().getThreadInfo(thread.getId()); - if (threadInfo == null || threadInfo.getThreadState() != Thread.State.BLOCKED) { - return false; + @Test + void testElectionAndPublicationRunOnSameWorker() { + AtomicReference electionThread = new AtomicReference<>(); + AtomicReference publicationThread = new AtomicReference<>(); + when(registry.acquireLock(ELECTION_LOCK)).thenAnswer(invocation -> { + electionThread.set(Thread.currentThread()); + return true; + }); + server.addServerStatusChangeListener((origin, target) -> publicationThread.set(Thread.currentThread())); + server.start(); + assertNotEquals(Thread.currentThread(), electionThread.get()); + assertEquals(electionThread.get(), publicationThread.get()); + owner.set("master-1:5678#peer-instance"); + remove(""); + assertEquals(electionThread.get(), publicationThread.get()); + } + + @Test + void testCloseDiscardsPendingRequestAndLateElectionResult() throws Exception { + CountDownLatch acquired = new CountDownLatch(1); + CountDownLatch allowElection = new CountDownLatch(1); + when(registry.acquireLock(ELECTION_LOCK)).thenAnswer(invocation -> { + acquired.countDown(); + assertTrue(allowElection.await(5, TimeUnit.SECONDS)); + return true; + }); + ExecutorService caller = Executors.newSingleThreadExecutor(); + try { + Future startup = caller.submit(server::start); + assertTrue(acquired.await(5, TimeUnit.SECONDS)); + notifyRemove(""); + server.close(); + // A notification after shutdown is ignored, including the submit/shutdown race. + notifyRemove(""); + allowElection.countDown(); + startup.get(5, TimeUnit.SECONDS); + assertTrue(electionExecutor.awaitTermination(5, TimeUnit.SECONDS)); + assertFalse(server.isActive()); + verify(statusListener, never()).changeToActive(); + verify(registry, times(1)).acquireLock(ELECTION_LOCK); + } finally { + allowElection.countDown(); + caller.shutdownNow(); + assertTrue(caller.awaitTermination(5, TimeUnit.SECONDS)); + } + } + + @Test + void testCloseFromListenerDoesNotDeadlockOrActivateQueuedWork() throws Exception { + server.start(); + CountDownLatch closed = new CountDownLatch(1); + server.addServerStatusChangeListener(new AbstractServerStatusChangeListener() { + + @Override + public void changeToActive() { + } + + @Override + public void changeToStandBy() { + // Alert closes its HA server from the demotion listener on the election worker. + server.close(); + owner.set(null); + notifyRemove(""); + closed.countDown(); + } + }); + AbstractServerStatusChangeListener subsequentListener = mock(AbstractServerStatusChangeListener.class); + server.addServerStatusChangeListener(subsequentListener); + owner.set("master-1:5678#peer-instance"); + notifyRemove(""); + assertTrue(closed.await(5, TimeUnit.SECONDS)); + assertTrue(electionExecutor.awaitTermination(5, TimeUnit.SECONDS)); + assertFalse(server.isActive()); + verify(statusListener, times(1)).changeToActive(); + org.mockito.Mockito.verifyNoInteractions(subsequentListener); + } + + @Test + void testCloseDoesNotStrandQueuedStartupFuture() throws Exception { + CountDownLatch workerBlocked = new CountDownLatch(1); + CountDownLatch releaseWorker = new CountDownLatch(1); + electionExecutor.execute(() -> { + workerBlocked.countDown(); + await(releaseWorker); + }); + ExecutorService caller = Executors.newSingleThreadExecutor(); + try { + assertTrue(workerBlocked.await(5, TimeUnit.SECONDS)); + Future startup = caller.submit(server::start); + // Observe the submitted Future in the queue, not just subscribe() before submission. + ThreadPoolExecutor executor = (ThreadPoolExecutor) electionExecutor; + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5); + while (executor.getQueue().isEmpty() && System.nanoTime() < deadline) { + Thread.yield(); + } + assertEquals(1, executor.getQueue().size()); + server.close(); + releaseWorker.countDown(); + startup.get(5, TimeUnit.SECONDS); + assertFalse(server.isActive()); + verify(registry, never()).acquireLock(ELECTION_LOCK); + } finally { + releaseWorker.countDown(); + caller.shutdownNow(); + assertTrue(caller.awaitTermination(5, TimeUnit.SECONDS)); + } + } + + @Test + void testCloseBeforeStartupSubmissionRejectsWithoutWaiting() { + doAnswer(invocation -> { + // Close after subscription but before the startup Future can be submitted. + server.close(); + return null; + }).when(registry).subscribe(eq(SELECTOR_PATH), org.mockito.ArgumentMatchers.any()); + assertThrows(java.util.concurrent.RejectedExecutionException.class, server::start); + verify(registry, never()).acquireLock(ELECTION_LOCK); + } + + @Test + void testExternalCloseWaitsForEnteredListener() throws Exception { + server.start(); + CountDownLatch entered = new CountDownLatch(1); + CountDownLatch releaseListener = new CountDownLatch(1); + AtomicReference failure = new AtomicReference<>(); + server.addServerStatusChangeListener((origin, target) -> { + entered.countDown(); + await(releaseListener); + }); + Thread closer = new Thread(() -> { + try { + server.close(); + } catch (Throwable ex) { + failure.set(ex); + } + }); + closer.setDaemon(true); + try { + owner.set("master-1:5678#peer-instance"); + notifyRemove(""); + assertTrue(entered.await(5, TimeUnit.SECONDS)); + closer.start(); + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5); + while (closer.isAlive() && closer.getState() != Thread.State.BLOCKED + && System.nanoTime() < deadline) { + Thread.yield(); + } + // The listener holds the publication monitor; external close must wait for it. + assertEquals(Thread.State.BLOCKED, closer.getState()); + releaseListener.countDown(); + closer.join(5000); + assertFalse(closer.isAlive()); + assertNull(failure.get()); + } finally { + releaseListener.countDown(); + closer.join(5000); } - LockInfo lockInfo = threadInfo.getLockInfo(); - return lockInfo != null && lockInfo.getIdentityHashCode() == System.identityHashCode(server); } private void remove(String previousOwner) { + notifyRemove(previousOwner); + drain(); + } + + private void notifyRemove(String previousOwner) { subscriber.get().notify(new Event(SELECTOR_PATH, SELECTOR_PATH, previousOwner, Event.Type.REMOVE)); } + private void drain() { + try { + electionExecutor.submit(() -> { + }).get(5, TimeUnit.SECONDS); + assertNoWorkerFailure(); + } catch (Exception e) { + throw new AssertionError("Election worker did not finish", e); + } + } + + private void assertNoWorkerFailure() { + if (workerFailure.get() != null) { + throw new AssertionError("Election worker failed", workerFailure.get()); + } + } + + private void await(CountDownLatch latch) { + try { + assertTrue(latch.await(5, TimeUnit.SECONDS)); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new AssertionError(e); + } + } + private AbstractHAServer newServer() { return newServer(ADDRESS); } private AbstractHAServer newServer(String address) { - return new AbstractHAServer(registry, SELECTOR_PATH, address) { - - @Override - public void close() { - - } + ExecutorService executor = new ThreadPoolExecutor(1, 1, 0L, TimeUnit.MILLISECONDS, + new LinkedBlockingQueue<>(), runnable -> { + Thread thread = new Thread(() -> { + // Static mocks are thread-local, so install the retry delay stub on the election worker. + try ( + MockedStatic threadUtils = + mockStatic(ThreadUtils.class, CALLS_REAL_METHODS)) { + threadUtils.when(() -> ThreadUtils.sleep(anyLong())).thenAnswer(invocation -> { + retryAction.get().run(); + return null; + }); + runnable.run(); + } + }, "test-ha-election"); + thread.setUncaughtExceptionHandler( + (failedThread, failure) -> workerFailure.compareAndSet(null, failure)); + return thread; + }); + electionExecutors.add(executor); + AbstractHAServer result = new AbstractHAServer(registry, SELECTOR_PATH, address, executor) { }; + servers.add(result); + return result; } }