From 0a3173302f874deba765415a84d78c5155c5e538 Mon Sep 17 00:00:00 2001 From: Yanjun Qiu <153984347+qiuyanjun888@users.noreply.github.com> Date: Wed, 15 Jul 2026 12:29:39 +0800 Subject: [PATCH 01/16] [Fix-18274][Registry] Handle expired JDBC heartbeat sessions --- .../jdbc/server/JdbcRegistryServer.java | 11 +- .../jdbc/server/JdbcRegistryServerTest.java | 135 ++++++++++++++++++ 2 files changed, 143 insertions(+), 3 deletions(-) create mode 100644 dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java index 61a934bd148e..23c0d60e28c9 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java @@ -321,8 +321,10 @@ private void refreshClientsHeartbeat() { if (CollectionUtils.isEmpty(jdbcRegistryClients)) { return; } - if (jdbcRegistryServerState == JdbcRegistryServerState.STOPPED) { - log.warn("The JdbcRegistryServer is STOPPED, will not refresh clients: {} heartbeat.", + if (jdbcRegistryServerState == JdbcRegistryServerState.STOPPED + || jdbcRegistryServerState == JdbcRegistryServerState.DISCONNECTED) { + log.warn("The JdbcRegistryServer is {}, will not refresh clients: {} heartbeat.", + jdbcRegistryServerState, CollectionUtils.collect(jdbcRegistryClients, IJdbcRegistryClient::getJdbcRegistryClientIdentify)); return; } @@ -341,7 +343,10 @@ private void refreshClientsHeartbeat() { } JdbcRegistryClientHeartbeatDTO clone = jdbcRegistryClientHeartbeatDTO.clone(); clone.setLastHeartbeatTime(now); - jdbcRegistryClientRepository.updateById(jdbcRegistryClientHeartbeatDTO); + if (!jdbcRegistryClientRepository.updateById(clone)) { + throw new RegistryException( + "The client heartbeat has expired: " + jdbcRegistryClientHeartbeatDTO.getId()); + } jdbcRegistryClientHeartbeatDTO.setLastHeartbeatTime(clone.getLastHeartbeatTime()); } if (jdbcRegistryServerState == JdbcRegistryServerState.SUSPENDED) { diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java new file mode 100644 index 000000000000..605082499eb0 --- /dev/null +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java @@ -0,0 +1,135 @@ +/* + * 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.plugin.registry.jdbc.server; + +import org.apache.dolphinscheduler.plugin.registry.jdbc.JdbcRegistryProperties; +import org.apache.dolphinscheduler.plugin.registry.jdbc.client.IJdbcRegistryClient; +import org.apache.dolphinscheduler.plugin.registry.jdbc.client.JdbcRegistryClientIdentify; +import org.apache.dolphinscheduler.plugin.registry.jdbc.model.DTO.JdbcRegistryClientHeartbeatDTO; +import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryClientRepository; +import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryDataChangeEventRepository; +import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryDataRepository; +import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryLockRepository; + +import java.time.Duration; +import java.util.concurrent.atomic.AtomicLong; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.test.util.ReflectionTestUtils; +import org.springframework.transaction.support.TransactionTemplate; + +import com.google.common.truth.Truth; + +@ExtendWith(MockitoExtension.class) +class JdbcRegistryServerTest { + + private static final JdbcRegistryClientIdentify CLIENT_IDENTIFY = + new JdbcRegistryClientIdentify(1L, "test-client"); + + @Mock + private JdbcRegistryDataRepository jdbcRegistryDataRepository; + + @Mock + private JdbcRegistryLockRepository jdbcRegistryLockRepository; + + @Mock + private JdbcRegistryClientRepository jdbcRegistryClientRepository; + + @Mock + private JdbcRegistryDataChangeEventRepository jdbcRegistryDataChangeEventRepository; + + @Mock + private TransactionTemplate transactionTemplate; + + @Mock + private IJdbcRegistryClient jdbcRegistryClient; + + @Mock + private ConnectionStateListener connectionStateListener; + + private JdbcRegistryServer jdbcRegistryServer; + + @BeforeEach + void setUp() { + JdbcRegistryProperties jdbcRegistryProperties = new JdbcRegistryProperties(); + jdbcRegistryProperties.setSessionTimeout(Duration.ofSeconds(1)); + jdbcRegistryServer = new JdbcRegistryServer( + jdbcRegistryDataRepository, + jdbcRegistryLockRepository, + jdbcRegistryClientRepository, + jdbcRegistryDataChangeEventRepository, + jdbcRegistryProperties, + transactionTemplate); + Mockito.when(jdbcRegistryClient.getJdbcRegistryClientIdentify()).thenReturn(CLIENT_IDENTIFY); + jdbcRegistryServer.registerClient(jdbcRegistryClient); + jdbcRegistryServer.subscribeConnectionStateChange(connectionStateListener); + } + + @AfterEach + void tearDown() { + jdbcRegistryServer.close(); + } + + @Test + void refreshClientsHeartbeat_shouldDisconnectWhenHeartbeatRecordWasPurged() { + ReflectionTestUtils.setField(jdbcRegistryServer, "jdbcRegistryServerState", + JdbcRegistryServerState.SUSPENDED); + ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", 0L); + Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenReturn(false); + + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.DISCONNECTED); + Mockito.verify(connectionStateListener).onDisConnected(); + } + + @Test + void refreshClientsHeartbeat_shouldPersistCurrentHeartbeatTimestamp() { + ArgumentCaptor registeredHeartbeat = + ArgumentCaptor.forClass(JdbcRegistryClientHeartbeatDTO.class); + Mockito.verify(jdbcRegistryClientRepository).insert(registeredHeartbeat.capture()); + registeredHeartbeat.getValue().setLastHeartbeatTime(0L); + AtomicLong persistedHeartbeatTimestamp = new AtomicLong(-1L); + Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenAnswer(invocation -> { + JdbcRegistryClientHeartbeatDTO heartbeat = invocation.getArgument(0); + persistedHeartbeatTimestamp.set(heartbeat.getLastHeartbeatTime()); + return true; + }); + + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + + Truth.assertThat(persistedHeartbeatTimestamp.get()).isGreaterThan(0L); + } + + @Test + void refreshClientsHeartbeat_shouldNotRefreshAfterDisconnected() { + ReflectionTestUtils.setField(jdbcRegistryServer, "jdbcRegistryServerState", + JdbcRegistryServerState.DISCONNECTED); + + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + + Mockito.verify(jdbcRegistryClientRepository, Mockito.never()).updateById(Mockito.any()); + } +} From 2cf868b064b052f8ab4507a167768bd2e786a8e6 Mon Sep 17 00:00:00 2001 From: qiuyanjun Date: Thu, 16 Jul 2026 08:42:41 +0800 Subject: [PATCH 02/16] [Fix-18274][Registry] Disconnect expired JDBC sessions immediately --- .../registry/jdbc/server/JdbcRegistryServer.java | 6 ++++-- .../jdbc/server/JdbcRegistryServerTest.java | 13 +++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java index 23c0d60e28c9..287746e5f5b9 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java @@ -344,8 +344,10 @@ private void refreshClientsHeartbeat() { JdbcRegistryClientHeartbeatDTO clone = jdbcRegistryClientHeartbeatDTO.clone(); clone.setLastHeartbeatTime(now); if (!jdbcRegistryClientRepository.updateById(clone)) { - throw new RegistryException( - "The client heartbeat has expired: " + jdbcRegistryClientHeartbeatDTO.getId()); + log.error("The client heartbeat has expired: {}", jdbcRegistryClientHeartbeatDTO.getId()); + jdbcRegistryServerState = JdbcRegistryServerState.DISCONNECTED; + doTriggerOnDisConnectedListener(); + return; } jdbcRegistryClientHeartbeatDTO.setLastHeartbeatTime(clone.getLastHeartbeatTime()); } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java index 605082499eb0..5a449425f3ab 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java @@ -105,6 +105,19 @@ void refreshClientsHeartbeat_shouldDisconnectWhenHeartbeatRecordWasPurged() { Mockito.verify(connectionStateListener).onDisConnected(); } + @Test + void refreshClientsHeartbeat_shouldDisconnectImmediatelyWhenStartedHeartbeatRecordWasPurged() { + ReflectionTestUtils.setField(jdbcRegistryServer, "jdbcRegistryServerState", JdbcRegistryServerState.STARTED); + Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenReturn(false); + + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.DISCONNECTED); + Mockito.verify(jdbcRegistryClientRepository).updateById(Mockito.any()); + Mockito.verify(connectionStateListener).onDisConnected(); + } + @Test void refreshClientsHeartbeat_shouldPersistCurrentHeartbeatTimestamp() { ArgumentCaptor registeredHeartbeat = From cbdfd1d4670ef07e54f08d63a53543f1dc9a156b Mon Sep 17 00:00:00 2001 From: Yanjun Qiu <153984347+qiuyanjun888@users.noreply.github.com> Date: Mon, 20 Jul 2026 11:53:09 +0800 Subject: [PATCH 03/16] [Fix-18274][Registry] Preserve stopped state during heartbeat --- .../jdbc/server/JdbcRegistryServer.java | 21 +++++++++--- .../jdbc/server/JdbcRegistryServerTest.java | 34 +++++++++++++++++++ 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java index 287746e5f5b9..c97653ba2691 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java @@ -256,7 +256,9 @@ public void releaseJdbcRegistryLock(Long clientId, String lockKey) { @Override public void close() { - jdbcRegistryServerState = JdbcRegistryServerState.STOPPED; + synchronized (this) { + jdbcRegistryServerState = JdbcRegistryServerState.STOPPED; + } schedulerThreadExecutor.shutdown(); List clientIds = jdbcRegistryClients.stream() .map(IJdbcRegistryClient::getJdbcRegistryClientIdentify) @@ -345,8 +347,9 @@ private void refreshClientsHeartbeat() { clone.setLastHeartbeatTime(now); if (!jdbcRegistryClientRepository.updateById(clone)) { log.error("The client heartbeat has expired: {}", jdbcRegistryClientHeartbeatDTO.getId()); - jdbcRegistryServerState = JdbcRegistryServerState.DISCONNECTED; - doTriggerOnDisConnectedListener(); + if (transitionToDisconnected()) { + doTriggerOnDisConnectedListener(); + } return; } jdbcRegistryClientHeartbeatDTO.setLastHeartbeatTime(clone.getLastHeartbeatTime()); @@ -366,8 +369,7 @@ private void refreshClientsHeartbeat() { break; case SUSPENDED: if (System.currentTimeMillis() - lastSuccessHeartbeat > jdbcRegistryProperties.getSessionTimeout() - .toMillis()) { - jdbcRegistryServerState = JdbcRegistryServerState.DISCONNECTED; + .toMillis() && transitionToDisconnected()) { doTriggerOnDisConnectedListener(); } break; @@ -377,6 +379,15 @@ private void refreshClientsHeartbeat() { } } + private synchronized boolean transitionToDisconnected() { + if (jdbcRegistryServerState != JdbcRegistryServerState.STARTED + && jdbcRegistryServerState != JdbcRegistryServerState.SUSPENDED) { + return false; + } + jdbcRegistryServerState = JdbcRegistryServerState.DISCONNECTED; + return true; + } + private void doTriggerReconnectedListener() { log.info("Trigger:onReconnected listener."); connectionStateListeners.forEach(listener -> { diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java index 5a449425f3ab..e019115d191b 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java @@ -27,6 +27,11 @@ import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryLockRepository; import java.time.Duration; +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.AtomicLong; import org.junit.jupiter.api.AfterEach; @@ -118,6 +123,35 @@ void refreshClientsHeartbeat_shouldDisconnectImmediatelyWhenStartedHeartbeatReco Mockito.verify(connectionStateListener).onDisConnected(); } + @Test + void refreshClientsHeartbeat_shouldNotDisconnectWhenCloseWinsRace() throws Exception { + ReflectionTestUtils.setField(jdbcRegistryServer, "jdbcRegistryServerState", JdbcRegistryServerState.STARTED); + CountDownLatch heartbeatUpdateStarted = new CountDownLatch(1); + CountDownLatch allowHeartbeatUpdateToFinish = new CountDownLatch(1); + Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenAnswer(invocation -> { + heartbeatUpdateStarted.countDown(); + allowHeartbeatUpdateToFinish.await(5, TimeUnit.SECONDS); + return false; + }); + ExecutorService heartbeatExecutor = Executors.newSingleThreadExecutor(); + Future heartbeatFuture = heartbeatExecutor.submit(() -> { + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + }); + + try { + Truth.assertThat(heartbeatUpdateStarted.await(5, TimeUnit.SECONDS)).isTrue(); + jdbcRegistryServer.close(); + allowHeartbeatUpdateToFinish.countDown(); + heartbeatFuture.get(5, TimeUnit.SECONDS); + } finally { + allowHeartbeatUpdateToFinish.countDown(); + heartbeatExecutor.shutdownNow(); + } + + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.STOPPED); + Mockito.verify(connectionStateListener, Mockito.never()).onDisConnected(); + } + @Test void refreshClientsHeartbeat_shouldPersistCurrentHeartbeatTimestamp() { ArgumentCaptor registeredHeartbeat = From 79cc4a0cde8ebf20cf96c3d1a901a8b095c3b59d Mon Sep 17 00:00:00 2001 From: qiuyanjun Date: Thu, 30 Jul 2026 13:05:47 +0800 Subject: [PATCH 04/16] [Fix-18274][Registry] Make heartbeat state transitions atomic --- .../jdbc/server/JdbcRegistryServer.java | 51 ++++++++++------ .../jdbc/server/JdbcRegistryServerTest.java | 61 +++++++++++++++++++ 2 files changed, 92 insertions(+), 20 deletions(-) diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java index c97653ba2691..28a68d1561b1 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java @@ -105,7 +105,7 @@ public JdbcRegistryServer(JdbcRegistryDataRepository jdbcRegistryDataRepository, } @Override - public void start() { + public synchronized void start() { if (jdbcRegistryServerState != JdbcRegistryServerState.INIT) { // The server is already started or stopped, will not start again. return; @@ -167,7 +167,7 @@ public void deregisterClient(IJdbcRegistryClient jdbcRegistryClient) { } @Override - public JdbcRegistryServerState getServerState() { + public synchronized JdbcRegistryServerState getServerState() { return jdbcRegistryServerState; } @@ -271,7 +271,7 @@ public void close() { private void purgeInvalidJdbcRegistryMetadata() { final StopWatch stopWatch = StopWatch.createStarted(); - if (jdbcRegistryServerState == JdbcRegistryServerState.STOPPED) { + if (getServerState() == JdbcRegistryServerState.STOPPED) { return; } // remove the client which is already dead from the registry, and remove it's related data and lock. @@ -323,10 +323,11 @@ private void refreshClientsHeartbeat() { if (CollectionUtils.isEmpty(jdbcRegistryClients)) { return; } - if (jdbcRegistryServerState == JdbcRegistryServerState.STOPPED - || jdbcRegistryServerState == JdbcRegistryServerState.DISCONNECTED) { + JdbcRegistryServerState currentState = getServerState(); + if (currentState == JdbcRegistryServerState.STOPPED + || currentState == JdbcRegistryServerState.DISCONNECTED) { log.warn("The JdbcRegistryServer is {}, will not refresh clients: {} heartbeat.", - jdbcRegistryServerState, + currentState, CollectionUtils.collect(jdbcRegistryClients, IJdbcRegistryClient::getJdbcRegistryClientIdentify)); return; } @@ -347,30 +348,25 @@ private void refreshClientsHeartbeat() { clone.setLastHeartbeatTime(now); if (!jdbcRegistryClientRepository.updateById(clone)) { log.error("The client heartbeat has expired: {}", jdbcRegistryClientHeartbeatDTO.getId()); - if (transitionToDisconnected()) { - doTriggerOnDisConnectedListener(); - } + transitionToDisconnected(); return; } jdbcRegistryClientHeartbeatDTO.setLastHeartbeatTime(clone.getLastHeartbeatTime()); } - if (jdbcRegistryServerState == JdbcRegistryServerState.SUSPENDED) { - jdbcRegistryServerState = JdbcRegistryServerState.STARTED; - doTriggerReconnectedListener(); - } + transitionToStarted(); lastSuccessHeartbeat = now; log.debug("Success refresh clients: {} heartbeat.", CollectionUtils.collect(jdbcRegistryClients, IJdbcRegistryClient::getJdbcRegistryClientIdentify)); } catch (Exception ex) { log.error("Failed to refresh the client's term", ex); - switch (jdbcRegistryServerState) { + switch (getServerState()) { case STARTED: - jdbcRegistryServerState = JdbcRegistryServerState.SUSPENDED; + transitionToSuspended(); break; case SUSPENDED: if (System.currentTimeMillis() - lastSuccessHeartbeat > jdbcRegistryProperties.getSessionTimeout() - .toMillis() && transitionToDisconnected()) { - doTriggerOnDisConnectedListener(); + .toMillis()) { + transitionToDisconnected(); } break; default: @@ -379,13 +375,28 @@ private void refreshClientsHeartbeat() { } } - private synchronized boolean transitionToDisconnected() { + private synchronized void transitionToStarted() { + if (jdbcRegistryServerState != JdbcRegistryServerState.SUSPENDED) { + return; + } + jdbcRegistryServerState = JdbcRegistryServerState.STARTED; + doTriggerReconnectedListener(); + } + + private synchronized void transitionToSuspended() { + if (jdbcRegistryServerState != JdbcRegistryServerState.STARTED) { + return; + } + jdbcRegistryServerState = JdbcRegistryServerState.SUSPENDED; + } + + private synchronized void transitionToDisconnected() { if (jdbcRegistryServerState != JdbcRegistryServerState.STARTED && jdbcRegistryServerState != JdbcRegistryServerState.SUSPENDED) { - return false; + return; } jdbcRegistryServerState = JdbcRegistryServerState.DISCONNECTED; - return true; + doTriggerOnDisConnectedListener(); } private void doTriggerReconnectedListener() { diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java index e019115d191b..ff736ac07d6b 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java @@ -152,6 +152,67 @@ void refreshClientsHeartbeat_shouldNotDisconnectWhenCloseWinsRace() throws Excep Mockito.verify(connectionStateListener, Mockito.never()).onDisConnected(); } + @Test + void refreshClientsHeartbeat_shouldNotReconnectWhenCloseWinsSuccessfulHeartbeatRace() throws Exception { + ReflectionTestUtils.setField(jdbcRegistryServer, "jdbcRegistryServerState", + JdbcRegistryServerState.SUSPENDED); + CountDownLatch heartbeatUpdateStarted = new CountDownLatch(1); + CountDownLatch allowHeartbeatUpdateToFinish = new CountDownLatch(1); + Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenAnswer(invocation -> { + heartbeatUpdateStarted.countDown(); + allowHeartbeatUpdateToFinish.await(5, TimeUnit.SECONDS); + return true; + }); + ExecutorService heartbeatExecutor = Executors.newSingleThreadExecutor(); + Future heartbeatFuture = heartbeatExecutor.submit(() -> { + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + }); + + try { + Truth.assertThat(heartbeatUpdateStarted.await(5, TimeUnit.SECONDS)).isTrue(); + jdbcRegistryServer.close(); + allowHeartbeatUpdateToFinish.countDown(); + heartbeatFuture.get(5, TimeUnit.SECONDS); + } finally { + allowHeartbeatUpdateToFinish.countDown(); + heartbeatExecutor.shutdownNow(); + } + + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.STOPPED); + Mockito.verify(connectionStateListener, Mockito.never()).onReconnected(); + Mockito.verify(connectionStateListener, Mockito.never()).onDisConnected(); + } + + @Test + void refreshClientsHeartbeat_shouldNotSuspendWhenCloseWinsFailedHeartbeatRace() throws Exception { + ReflectionTestUtils.setField(jdbcRegistryServer, "jdbcRegistryServerState", JdbcRegistryServerState.STARTED); + CountDownLatch heartbeatUpdateStarted = new CountDownLatch(1); + CountDownLatch allowHeartbeatUpdateToFail = new CountDownLatch(1); + Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenAnswer(invocation -> { + heartbeatUpdateStarted.countDown(); + allowHeartbeatUpdateToFail.await(5, TimeUnit.SECONDS); + throw new RuntimeException("Heartbeat update failed"); + }); + ExecutorService heartbeatExecutor = Executors.newSingleThreadExecutor(); + Future heartbeatFuture = heartbeatExecutor.submit(() -> { + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + }); + + try { + Truth.assertThat(heartbeatUpdateStarted.await(5, TimeUnit.SECONDS)).isTrue(); + jdbcRegistryServer.close(); + allowHeartbeatUpdateToFail.countDown(); + heartbeatFuture.get(5, TimeUnit.SECONDS); + } finally { + allowHeartbeatUpdateToFail.countDown(); + heartbeatExecutor.shutdownNow(); + } + + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.STOPPED); + Mockito.verify(connectionStateListener, Mockito.never()).onReconnected(); + Mockito.verify(connectionStateListener, Mockito.never()).onDisConnected(); + } + @Test void refreshClientsHeartbeat_shouldPersistCurrentHeartbeatTimestamp() { ArgumentCaptor registeredHeartbeat = From fddfbe1a81f985452d235ba20eee9a0feff0cf66 Mon Sep 17 00:00:00 2001 From: qiuyanjun Date: Sat, 12 Sep 2026 20:51:40 +0800 Subject: [PATCH 05/16] [Fix-18274][Registry] Address JDBC registry state review --- .../jdbc/server/JdbcRegistryServer.java | 50 ++++++++++++------- .../jdbc/server/JdbcRegistryServerTest.java | 33 ++++++++++++ 2 files changed, 64 insertions(+), 19 deletions(-) diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java index 28a68d1561b1..ac039897028e 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java @@ -45,6 +45,7 @@ import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReferenceFieldUpdater; import java.util.stream.Collectors; import lombok.SneakyThrows; @@ -60,6 +61,10 @@ @Slf4j public class JdbcRegistryServer implements IJdbcRegistryServer { + private static final AtomicReferenceFieldUpdater SERVER_STATE_UPDATER = + AtomicReferenceFieldUpdater.newUpdater( + JdbcRegistryServer.class, JdbcRegistryServerState.class, "jdbcRegistryServerState"); + private final JdbcRegistryProperties jdbcRegistryProperties; private final JdbcRegistryLockRepository jdbcRegistryLockRepository; @@ -70,7 +75,7 @@ public class JdbcRegistryServer implements IJdbcRegistryServer { private final JdbcRegistryLockManager jdbcRegistryLockManager; - private JdbcRegistryServerState jdbcRegistryServerState; + private volatile JdbcRegistryServerState jdbcRegistryServerState; private final List jdbcRegistryClients = new CopyOnWriteArrayList<>(); @@ -167,7 +172,7 @@ public void deregisterClient(IJdbcRegistryClient jdbcRegistryClient) { } @Override - public synchronized JdbcRegistryServerState getServerState() { + public JdbcRegistryServerState getServerState() { return jdbcRegistryServerState; } @@ -257,7 +262,11 @@ public void releaseJdbcRegistryLock(Long clientId, String lockKey) { @Override public void close() { synchronized (this) { - jdbcRegistryServerState = JdbcRegistryServerState.STOPPED; + if (SERVER_STATE_UPDATER.getAndSet(this, + JdbcRegistryServerState.STOPPED) == JdbcRegistryServerState.STOPPED) { + log.warn("The JdbcRegistryServer is already STOPPED."); + return; + } } schedulerThreadExecutor.shutdown(); List clientIds = jdbcRegistryClients.stream() @@ -375,28 +384,31 @@ private void refreshClientsHeartbeat() { } } - private synchronized void transitionToStarted() { - if (jdbcRegistryServerState != JdbcRegistryServerState.SUSPENDED) { - return; + private void transitionToStarted() { + if (SERVER_STATE_UPDATER.compareAndSet( + this, JdbcRegistryServerState.SUSPENDED, JdbcRegistryServerState.STARTED)) { + doTriggerReconnectedListener(); } - jdbcRegistryServerState = JdbcRegistryServerState.STARTED; - doTriggerReconnectedListener(); } - private synchronized void transitionToSuspended() { - if (jdbcRegistryServerState != JdbcRegistryServerState.STARTED) { - return; - } - jdbcRegistryServerState = JdbcRegistryServerState.SUSPENDED; + private void transitionToSuspended() { + SERVER_STATE_UPDATER.compareAndSet( + this, JdbcRegistryServerState.STARTED, JdbcRegistryServerState.SUSPENDED); } - private synchronized void transitionToDisconnected() { - if (jdbcRegistryServerState != JdbcRegistryServerState.STARTED - && jdbcRegistryServerState != JdbcRegistryServerState.SUSPENDED) { - return; + private void transitionToDisconnected() { + while (true) { + JdbcRegistryServerState currentState = jdbcRegistryServerState; + if (currentState != JdbcRegistryServerState.STARTED + && currentState != JdbcRegistryServerState.SUSPENDED) { + return; + } + if (SERVER_STATE_UPDATER.compareAndSet( + this, currentState, JdbcRegistryServerState.DISCONNECTED)) { + doTriggerOnDisConnectedListener(); + return; + } } - jdbcRegistryServerState = JdbcRegistryServerState.DISCONNECTED; - doTriggerOnDisConnectedListener(); } private void doTriggerReconnectedListener() { diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java index ff736ac07d6b..daca1fc0b31e 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java @@ -32,6 +32,7 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import org.junit.jupiter.api.AfterEach; @@ -97,6 +98,38 @@ void tearDown() { jdbcRegistryServer.close(); } + @Test + void close_shouldOnlyPurgeClientsOnceWhenCalledConcurrently() throws Exception { + CountDownLatch firstPurgeStarted = new CountDownLatch(1); + CountDownLatch allowFirstPurgeToFinish = new CountDownLatch(1); + AtomicInteger purgeInvocations = new AtomicInteger(); + Mockito.doAnswer(invocation -> { + if (purgeInvocations.incrementAndGet() == 1) { + firstPurgeStarted.countDown(); + allowFirstPurgeToFinish.await(5, TimeUnit.SECONDS); + } + return null; + }).when(jdbcRegistryClientRepository).deleteByIds(Mockito.any()); + ExecutorService closeExecutor = Executors.newFixedThreadPool(2); + Future firstClose = closeExecutor.submit(jdbcRegistryServer::close); + Future secondClose = null; + + try { + Truth.assertThat(firstPurgeStarted.await(5, TimeUnit.SECONDS)).isTrue(); + secondClose = closeExecutor.submit(jdbcRegistryServer::close); + secondClose.get(5, TimeUnit.SECONDS); + + Truth.assertThat(purgeInvocations.get()).isEqualTo(1); + } finally { + allowFirstPurgeToFinish.countDown(); + firstClose.get(5, TimeUnit.SECONDS); + if (secondClose != null) { + secondClose.get(5, TimeUnit.SECONDS); + } + closeExecutor.shutdownNow(); + } + } + @Test void refreshClientsHeartbeat_shouldDisconnectWhenHeartbeatRecordWasPurged() { ReflectionTestUtils.setField(jdbcRegistryServer, "jdbcRegistryServerState", From 71e8a14eb2848fcafd1221b423f323c85b293bb5 Mon Sep 17 00:00:00 2001 From: qiuyanjun Date: Mon, 14 Sep 2026 12:32:40 +0800 Subject: [PATCH 06/16] [Fix-18274][Registry] Address JDBC registry review comments --- .../jdbc/server/JdbcRegistryServer.java | 37 ++++++++----------- .../jdbc/server/JdbcRegistryServerTest.java | 29 +++++++++------ 2 files changed, 34 insertions(+), 32 deletions(-) diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java index ac039897028e..ae0464468f15 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java @@ -45,7 +45,7 @@ import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicReferenceFieldUpdater; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; import lombok.SneakyThrows; @@ -61,10 +61,6 @@ @Slf4j public class JdbcRegistryServer implements IJdbcRegistryServer { - private static final AtomicReferenceFieldUpdater SERVER_STATE_UPDATER = - AtomicReferenceFieldUpdater.newUpdater( - JdbcRegistryServer.class, JdbcRegistryServerState.class, "jdbcRegistryServerState"); - private final JdbcRegistryProperties jdbcRegistryProperties; private final JdbcRegistryLockRepository jdbcRegistryLockRepository; @@ -75,7 +71,8 @@ public class JdbcRegistryServer implements IJdbcRegistryServer { private final JdbcRegistryLockManager jdbcRegistryLockManager; - private volatile JdbcRegistryServerState jdbcRegistryServerState; + private final AtomicReference serverState = + new AtomicReference<>(JdbcRegistryServerState.INIT); private final List jdbcRegistryClients = new CopyOnWriteArrayList<>(); @@ -105,13 +102,12 @@ public JdbcRegistryServer(JdbcRegistryDataRepository jdbcRegistryDataRepository, transactionTemplate, schedulerThreadExecutor); this.jdbcRegistryLockManager = new JdbcRegistryLockManager( jdbcRegistryProperties, jdbcRegistryLockRepository); - this.jdbcRegistryServerState = JdbcRegistryServerState.INIT; lastSuccessHeartbeat = System.currentTimeMillis(); } @Override public synchronized void start() { - if (jdbcRegistryServerState != JdbcRegistryServerState.INIT) { + if (serverState.get() != JdbcRegistryServerState.INIT) { // The server is already started or stopped, will not start again. return; } @@ -124,7 +120,7 @@ public synchronized void start() { jdbcRegistryProperties.getSessionTimeout().toMillis(), TimeUnit.MILLISECONDS); jdbcRegistryDataManager.start(); - jdbcRegistryServerState = JdbcRegistryServerState.STARTED; + serverState.set(JdbcRegistryServerState.STARTED); doTriggerOnConnectedListener(); schedulerThreadExecutor.scheduleWithFixedDelay( this::refreshClientsHeartbeat, @@ -173,7 +169,7 @@ public void deregisterClient(IJdbcRegistryClient jdbcRegistryClient) { @Override public JdbcRegistryServerState getServerState() { - return jdbcRegistryServerState; + return serverState.get(); } @Override @@ -262,8 +258,7 @@ public void releaseJdbcRegistryLock(Long clientId, String lockKey) { @Override public void close() { synchronized (this) { - if (SERVER_STATE_UPDATER.getAndSet(this, - JdbcRegistryServerState.STOPPED) == JdbcRegistryServerState.STOPPED) { + if (serverState.getAndSet(JdbcRegistryServerState.STOPPED) == JdbcRegistryServerState.STOPPED) { log.warn("The JdbcRegistryServer is already STOPPED."); return; } @@ -357,8 +352,8 @@ private void refreshClientsHeartbeat() { clone.setLastHeartbeatTime(now); if (!jdbcRegistryClientRepository.updateById(clone)) { log.error("The client heartbeat has expired: {}", jdbcRegistryClientHeartbeatDTO.getId()); - transitionToDisconnected(); - return; + throw new IllegalStateException( + "The client heartbeat record no longer exists: " + jdbcRegistryClientHeartbeatDTO.getId()); } jdbcRegistryClientHeartbeatDTO.setLastHeartbeatTime(clone.getLastHeartbeatTime()); } @@ -385,29 +380,29 @@ private void refreshClientsHeartbeat() { } private void transitionToStarted() { - if (SERVER_STATE_UPDATER.compareAndSet( - this, JdbcRegistryServerState.SUSPENDED, JdbcRegistryServerState.STARTED)) { + if (serverState.compareAndSet(JdbcRegistryServerState.SUSPENDED, JdbcRegistryServerState.STARTED)) { doTriggerReconnectedListener(); } } private void transitionToSuspended() { - SERVER_STATE_UPDATER.compareAndSet( - this, JdbcRegistryServerState.STARTED, JdbcRegistryServerState.SUSPENDED); + if (!serverState.compareAndSet(JdbcRegistryServerState.STARTED, JdbcRegistryServerState.SUSPENDED)) { + log.debug("Failed to transition JdbcRegistryServer to SUSPENDED; current state is {}", serverState.get()); + } } private void transitionToDisconnected() { while (true) { - JdbcRegistryServerState currentState = jdbcRegistryServerState; + JdbcRegistryServerState currentState = serverState.get(); if (currentState != JdbcRegistryServerState.STARTED && currentState != JdbcRegistryServerState.SUSPENDED) { return; } - if (SERVER_STATE_UPDATER.compareAndSet( - this, currentState, JdbcRegistryServerState.DISCONNECTED)) { + if (serverState.compareAndSet(currentState, JdbcRegistryServerState.DISCONNECTED)) { doTriggerOnDisConnectedListener(); return; } + log.debug("Failed to transition JdbcRegistryServer from {} to DISCONNECTED; retrying", currentState); } } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java index daca1fc0b31e..4645043653e0 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java @@ -34,6 +34,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -132,8 +133,7 @@ void close_shouldOnlyPurgeClientsOnceWhenCalledConcurrently() throws Exception { @Test void refreshClientsHeartbeat_shouldDisconnectWhenHeartbeatRecordWasPurged() { - ReflectionTestUtils.setField(jdbcRegistryServer, "jdbcRegistryServerState", - JdbcRegistryServerState.SUSPENDED); + setServerState(JdbcRegistryServerState.SUSPENDED); ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", 0L); Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenReturn(false); @@ -144,21 +144,24 @@ void refreshClientsHeartbeat_shouldDisconnectWhenHeartbeatRecordWasPurged() { } @Test - void refreshClientsHeartbeat_shouldDisconnectImmediatelyWhenStartedHeartbeatRecordWasPurged() { - ReflectionTestUtils.setField(jdbcRegistryServer, "jdbcRegistryServerState", JdbcRegistryServerState.STARTED); + void refreshClientsHeartbeat_shouldDisconnectAfterSessionTimeoutWhenStartedHeartbeatRecordWasPurged() { + setServerState(JdbcRegistryServerState.STARTED); Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenReturn(false); ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.SUSPENDED); + ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", 0L); ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.DISCONNECTED); - Mockito.verify(jdbcRegistryClientRepository).updateById(Mockito.any()); + Mockito.verify(jdbcRegistryClientRepository, Mockito.times(2)).updateById(Mockito.any()); Mockito.verify(connectionStateListener).onDisConnected(); } @Test void refreshClientsHeartbeat_shouldNotDisconnectWhenCloseWinsRace() throws Exception { - ReflectionTestUtils.setField(jdbcRegistryServer, "jdbcRegistryServerState", JdbcRegistryServerState.STARTED); + setServerState(JdbcRegistryServerState.STARTED); CountDownLatch heartbeatUpdateStarted = new CountDownLatch(1); CountDownLatch allowHeartbeatUpdateToFinish = new CountDownLatch(1); Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenAnswer(invocation -> { @@ -187,8 +190,7 @@ void refreshClientsHeartbeat_shouldNotDisconnectWhenCloseWinsRace() throws Excep @Test void refreshClientsHeartbeat_shouldNotReconnectWhenCloseWinsSuccessfulHeartbeatRace() throws Exception { - ReflectionTestUtils.setField(jdbcRegistryServer, "jdbcRegistryServerState", - JdbcRegistryServerState.SUSPENDED); + setServerState(JdbcRegistryServerState.SUSPENDED); CountDownLatch heartbeatUpdateStarted = new CountDownLatch(1); CountDownLatch allowHeartbeatUpdateToFinish = new CountDownLatch(1); Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenAnswer(invocation -> { @@ -218,7 +220,7 @@ void refreshClientsHeartbeat_shouldNotReconnectWhenCloseWinsSuccessfulHeartbeatR @Test void refreshClientsHeartbeat_shouldNotSuspendWhenCloseWinsFailedHeartbeatRace() throws Exception { - ReflectionTestUtils.setField(jdbcRegistryServer, "jdbcRegistryServerState", JdbcRegistryServerState.STARTED); + setServerState(JdbcRegistryServerState.STARTED); CountDownLatch heartbeatUpdateStarted = new CountDownLatch(1); CountDownLatch allowHeartbeatUpdateToFail = new CountDownLatch(1); Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenAnswer(invocation -> { @@ -266,11 +268,16 @@ void refreshClientsHeartbeat_shouldPersistCurrentHeartbeatTimestamp() { @Test void refreshClientsHeartbeat_shouldNotRefreshAfterDisconnected() { - ReflectionTestUtils.setField(jdbcRegistryServer, "jdbcRegistryServerState", - JdbcRegistryServerState.DISCONNECTED); + setServerState(JdbcRegistryServerState.DISCONNECTED); ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); Mockito.verify(jdbcRegistryClientRepository, Mockito.never()).updateById(Mockito.any()); } + + @SuppressWarnings("unchecked") + private void setServerState(JdbcRegistryServerState state) { + ((AtomicReference) ReflectionTestUtils.getField(jdbcRegistryServer, "serverState")) + .set(state); + } } From 21b406752c3192cf245ddf5c010c439a3b644a28 Mon Sep 17 00:00:00 2001 From: qiuyanjun Date: Sat, 26 Sep 2026 09:05:49 +0800 Subject: [PATCH 07/16] docs: add JDBC registry heartbeat state design --- ...26-jdbc-registry-heartbeat-state-design.md | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 docs/superpowers/specs/2026-09-26-jdbc-registry-heartbeat-state-design.md diff --git a/docs/superpowers/specs/2026-09-26-jdbc-registry-heartbeat-state-design.md b/docs/superpowers/specs/2026-09-26-jdbc-registry-heartbeat-state-design.md new file mode 100644 index 000000000000..eee330b32344 --- /dev/null +++ b/docs/superpowers/specs/2026-09-26-jdbc-registry-heartbeat-state-design.md @@ -0,0 +1,35 @@ +# JDBC Registry 心跳状态机修复设计 + +## 背景 + +JDBC Registry 客户端的 heartbeat 记录可能在数据库不可用超过 session timeout 后被其他服务清理。数据库恢复后，原服务继续尝试更新已经不存在的记录；如果忽略 `updateById` 的 0 行结果，服务会继续运行并保留过期身份。 + +本次修复需要同时处理 heartbeat 失效、服务关闭竞态和状态转换的 CAS 失败，避免服务在关闭后被恢复为工作状态，也避免回调与实际状态不一致。 + +## 状态约束 + +- `INIT` 只能转换为 `STARTED`。 +- heartbeat 失败时，`STARTED` 进入 `SUSPENDED`；如果从上次成功 heartbeat 起已经超过 session timeout，则直接进入 `DISCONNECTED`。 +- `SUSPENDED` 在 heartbeat 成功后进入 `STARTED`，并只在 CAS 成功时触发重连回调。 +- `SUSPENDED` 或 `STARTED` 在 session timeout 后进入 `DISCONNECTED`，并只在 CAS 成功时触发断连回调。 +- `STOPPED` 是关闭终态，任何未完成的 heartbeat 结果都不能覆盖它。 +- `close()` 只有成功将当前状态转换为 `STOPPED` 的线程执行调度器关闭、数据库清理和本地集合清理。 + +## 实现方案 + +使用现有的 `AtomicReference` 作为唯一状态存储和可见性边界。`close()` 使用 CAS 循环处理重复调用和并发状态变化；heartbeat 的成功、失败和断连路径在调用点直接处理 CAS 结果，删除只包装单个 CAS 的转换方法。断连路径只尝试允许的源状态，CAS 失败时根据当前状态结束本次事件，不通过无限重试把过期事件应用到新的状态上。 + +heartbeat 成功后的本地时间戳和成功日志只在当前服务仍可工作时更新。heartbeat 失败首先区分瞬时失败和已经超过 session timeout 的失败；记录不存在属于更新失败，不能通过 upsert 恢复旧身份。 + +## 测试方案 + +增加或调整以下回归覆盖： + +- heartbeat 记录被清理后的 `STARTED -> SUSPENDED -> DISCONNECTED` 路径。 +- 首次失败时已经超过 session timeout 的直接断连路径。 +- `close()` 与失败、成功 heartbeat 结果的双向竞态，确保 `STOPPED` 最终状态不被覆盖且不会发出错误回调。 +- 每个 CAS 失败分支都不会修改终态、重复触发回调或记录虚假的成功 heartbeat。 +- 多客户端刷新时前一个客户端成功、后一个客户端失败的状态和时间戳行为。 +- 已断连服务不再刷新 heartbeat。 + +验证使用 JDBC registry 模块的目标单元测试、Spotless 检查和必要的编译检查。 From 177451947e21f88b7669c5ff0ae8d1678fb7dc85 Mon Sep 17 00:00:00 2001 From: qiuyanjun Date: Sat, 26 Sep 2026 09:10:33 +0800 Subject: [PATCH 08/16] docs: add JDBC registry heartbeat implementation plan --- ...026-09-26-jdbc-registry-heartbeat-state.md | 127 ++++++++++++++++++ 1 file changed, 127 insertions(+) create mode 100644 docs/superpowers/plans/2026-09-26-jdbc-registry-heartbeat-state.md diff --git a/docs/superpowers/plans/2026-09-26-jdbc-registry-heartbeat-state.md b/docs/superpowers/plans/2026-09-26-jdbc-registry-heartbeat-state.md new file mode 100644 index 000000000000..3a87796e8334 --- /dev/null +++ b/docs/superpowers/plans/2026-09-26-jdbc-registry-heartbeat-state.md @@ -0,0 +1,127 @@ +# JDBC Registry 心跳状态机修复实施计划 + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**目标：** 修复 JDBC Registry heartbeat 失效后的状态转换、关闭竞态和 CAS 失败处理，保证 `STOPPED` 终态、回调和 session timeout 语义一致。 + +**架构：** 继续使用 `AtomicReference` 作为唯一状态存储。所有状态变化使用明确的 CAS；heartbeat 调用点处理 CAS 成功和失败，`close()` 只有成功进入 `STOPPED` 的线程执行清理。 + +**技术栈：** Java、JUnit 5、Mockito、Maven、Spotless。 + +## 全局约束 + +- `STOPPED` 是关闭终态，任何未完成的 heartbeat 结果都不能覆盖它。 +- 只有成功的状态转换才触发连接回调。 +- heartbeat 记录不存在时不能 upsert 或恢复旧身份。 +- `DISCONNECTED` 只能由 session timeout 触发。 +- 不引入新的生产依赖。 + +### 任务 1：补充状态转换和竞态回归测试 + +**文件：** + +- 修改：`dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java` + +**接口：** + +- 使用现有的 `refreshClientsHeartbeat()`、`close()`、`serverState` 和 `lastSuccessHeartbeat` 测试行为，不暴露新的生产接口。 + +- [ ] **步骤 1：添加首次失败已超时就断连的测试** + +设置服务为 `STARTED`、`lastSuccessHeartbeat` 为超时之前，并让 `updateById()` 返回 `false`；断言一次 heartbeat 后状态为 `DISCONNECTED`，且断连回调只触发一次。 + +- [ ] **步骤 2：添加 heartbeat 成功结果晚于 close 的测试** + +阻塞数据库更新，先完成 `close()`，再返回成功；断言最终状态为 `STOPPED`、不触发重连或断连回调，并断言 `lastSuccessHeartbeat` 未被关闭后的结果改写。 + +- [ ] **步骤 3：添加 heartbeat 失败结果晚于 close 的测试** + +阻塞数据库更新，先完成 `close()`，再抛出异常；断言最终状态保持 `STOPPED`，不触发任何连接状态回调。 + +- [ ] **步骤 4：添加 CAS 失败的终态保护测试** + +覆盖服务已经处于 `DISCONNECTED` 或 `STOPPED` 时收到成功、失败 heartbeat 结果的情况，断言状态不变、回调不重复、不会记录虚假的成功 heartbeat。 + +- [ ] **步骤 5：运行新增测试确认当前实现失败** + +运行： + +```bash +./mvnw -pl dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc -am -DskipITs -Dtest=JdbcRegistryServerTest -Dsurefire.failIfNoSpecifiedTests=false test +``` + +预期：新增的首次超时断连或关闭后时间戳断言至少失败一项，失败原因必须来自当前状态处理逻辑，而不是测试编译错误。 + +### 任务 2：统一 heartbeat 状态转换和关闭语义 + +**文件：** + +- 修改：`dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java` + +**接口：** + +- 保持 `IJdbcRegistryServer` 和 `JdbcRegistryServerState` 不变。 +- 删除只包装单个 CAS 的 `transitionToStarted()` 和 `transitionToSuspended()`。 +- 保持 `refreshClientsHeartbeat()`、`close()` 和监听器接口签名不变。 + +- [ ] **步骤 1：将 close 改为 CAS 终态转换** + +在现有 `synchronized (this)` 生命周期临界区内循环读取状态并执行 `compareAndSet(current, STOPPED)`；读取到 `STOPPED` 时直接返回。只有 CAS 成功的调用继续关闭调度器、清理数据库和清空本地集合。 + +- [ ] **步骤 2：将 start 的状态写入改为明确的 INIT 到 STARTED 转换** + +保留启动方法的生命周期锁，在初始化完成后使用 `compareAndSet(INIT, STARTED)`，失败时不触发 `onConnected()`。 + +- [ ] **步骤 3：在 heartbeat 成功路径直接处理 CAS 结果** + +成功更新数据库后，仅当服务仍处于 `STARTED` 或成功完成 `SUSPENDED -> STARTED` 时更新 `lastSuccessHeartbeat` 和成功日志；CAS 失败且当前状态为 `STOPPED` 或 `DISCONNECTED` 时结束本次处理。 + +- [ ] **步骤 4：在 heartbeat 失败路径直接处理允许的转换** + +先计算距离上次成功 heartbeat 的时间；超时后依次尝试 `STARTED -> DISCONNECTED` 和 `SUSPENDED -> DISCONNECTED`，只有一次 CAS 成功才触发断连回调。未超时时只尝试 `STARTED -> SUSPENDED`，CAS 失败时根据当前终态结束本次事件。 + +- [ ] **步骤 5：明确时间戳的跨线程可见性** + +将 `lastSuccessHeartbeat` 改为 `volatile long`，保持构造函数初始化并避免空值自动拆箱。 + +- [ ] **步骤 6：停止 DISCONNECTED 状态下的清理调度** + +让 `purgeInvalidJdbcRegistryMetadata()` 在 `DISCONNECTED` 和 `STOPPED` 状态都直接返回，避免失去数据库 session 的服务继续清理其他客户端元数据。 + +- [ ] **步骤 7：运行任务 1 的测试确认通过** + +运行同一 Maven 测试命令，预期 `JdbcRegistryServerTest` 全部通过且无失败、错误。 + +### 任务 3：补充多客户端和格式验证 + +**文件：** + +- 修改：`dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java` + +- [ ] **步骤 1：添加多客户端部分更新失败测试** + +注册两个客户端，让第一个更新成功、第二个更新返回 `false`；断言第一个本地 DTO 时间戳已更新，服务按当前 timeout 规则进入 `SUSPENDED` 或 `DISCONNECTED`，且不会触发重连回调。 + +- [ ] **步骤 2：运行完整目标测试** + +运行： + +```bash +./mvnw clean -pl dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc -am -DskipITs -Dtest=JdbcRegistryServerTest,JdbcRegistryDataChangeListenerAdapterTest -Dsurefire.failIfNoSpecifiedTests=false test +``` + +预期：选定测试全部通过，Maven reactor 构建成功。 + +- [ ] **步骤 3：运行格式检查** + +运行： + +```bash +./mvnw -pl dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc spotless:check +``` + +预期：Spotless 检查成功。 + +- [ ] **步骤 4：检查差异和工作区** + +运行 `git diff --check`、`git status --short` 和目标文件差异审查，确认只包含本次状态机修复、测试和设计文档，不包含工作区原有未跟踪文件。 From ae7f23e3176c5165b48a19cefac5da25f3c6f2d0 Mon Sep 17 00:00:00 2001 From: qiuyanjun Date: Sat, 26 Sep 2026 09:29:42 +0800 Subject: [PATCH 09/16] [Fix-18274][Registry] Make JDBC heartbeat state transitions safe --- ...026-09-26-jdbc-registry-heartbeat-state.md | 1 - .../jdbc/server/JdbcRegistryServer.java | 102 ++++++++++-------- .../jdbc/server/JdbcRegistryServerTest.java | 50 ++++++++- 3 files changed, 103 insertions(+), 50 deletions(-) diff --git a/docs/superpowers/plans/2026-09-26-jdbc-registry-heartbeat-state.md b/docs/superpowers/plans/2026-09-26-jdbc-registry-heartbeat-state.md index 3a87796e8334..d04123ae1727 100644 --- a/docs/superpowers/plans/2026-09-26-jdbc-registry-heartbeat-state.md +++ b/docs/superpowers/plans/2026-09-26-jdbc-registry-heartbeat-state.md @@ -63,7 +63,6 @@ - 保持 `IJdbcRegistryServer` 和 `JdbcRegistryServerState` 不变。 - 删除只包装单个 CAS 的 `transitionToStarted()` 和 `transitionToSuspended()`。 - 保持 `refreshClientsHeartbeat()`、`close()` 和监听器接口签名不变。 - - [ ] **步骤 1：将 close 改为 CAS 终态转换** 在现有 `synchronized (this)` 生命周期临界区内循环读取状态并执行 `compareAndSet(current, STOPPED)`；读取到 `STOPPED` 时直接返回。只有 CAS 成功的调用继续关闭调度器、清理数据库和清空本地集合。 diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java index ae0464468f15..71ddaf3810d5 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java @@ -83,7 +83,7 @@ public class JdbcRegistryServer implements IJdbcRegistryServer { private final ScheduledExecutorService schedulerThreadExecutor; - private Long lastSuccessHeartbeat; + private volatile long lastSuccessHeartbeat; public JdbcRegistryServer(JdbcRegistryDataRepository jdbcRegistryDataRepository, JdbcRegistryLockRepository jdbcRegistryLockRepository, @@ -120,7 +120,10 @@ public synchronized void start() { jdbcRegistryProperties.getSessionTimeout().toMillis(), TimeUnit.MILLISECONDS); jdbcRegistryDataManager.start(); - serverState.set(JdbcRegistryServerState.STARTED); + if (!serverState.compareAndSet(JdbcRegistryServerState.INIT, JdbcRegistryServerState.STARTED)) { + log.warn("The JdbcRegistryServer state changed before startup completed: {}", serverState.get()); + return; + } doTriggerOnConnectedListener(); schedulerThreadExecutor.scheduleWithFixedDelay( this::refreshClientsHeartbeat, @@ -258,10 +261,17 @@ public void releaseJdbcRegistryLock(Long clientId, String lockKey) { @Override public void close() { synchronized (this) { - if (serverState.getAndSet(JdbcRegistryServerState.STOPPED) == JdbcRegistryServerState.STOPPED) { + JdbcRegistryServerState currentState = serverState.get(); + if (currentState == JdbcRegistryServerState.STOPPED) { log.warn("The JdbcRegistryServer is already STOPPED."); return; } + if (!serverState.compareAndSet(currentState, JdbcRegistryServerState.STOPPED)) { + log.warn("Failed to stop JdbcRegistryServer from state {}, current state is {}", + currentState, + serverState.get()); + return; + } } schedulerThreadExecutor.shutdown(); List clientIds = jdbcRegistryClients.stream() @@ -275,7 +285,9 @@ public void close() { private void purgeInvalidJdbcRegistryMetadata() { final StopWatch stopWatch = StopWatch.createStarted(); - if (getServerState() == JdbcRegistryServerState.STOPPED) { + JdbcRegistryServerState currentState = getServerState(); + if (currentState == JdbcRegistryServerState.STOPPED + || currentState == JdbcRegistryServerState.DISCONNECTED) { return; } // remove the client which is already dead from the registry, and remove it's related data and lock. @@ -357,52 +369,54 @@ private void refreshClientsHeartbeat() { } jdbcRegistryClientHeartbeatDTO.setLastHeartbeatTime(clone.getLastHeartbeatTime()); } - transitionToStarted(); - lastSuccessHeartbeat = now; + synchronized (this) { + currentState = serverState.get(); + if (currentState == JdbcRegistryServerState.STOPPED + || currentState == JdbcRegistryServerState.DISCONNECTED) { + return; + } + if (currentState == JdbcRegistryServerState.SUSPENDED) { + if (!serverState.compareAndSet( + JdbcRegistryServerState.SUSPENDED, JdbcRegistryServerState.STARTED)) { + log.debug("Failed to reconnect JdbcRegistryServer; current state is {}", serverState.get()); + return; + } + lastSuccessHeartbeat = now; + doTriggerReconnectedListener(); + } else if (currentState == JdbcRegistryServerState.STARTED) { + lastSuccessHeartbeat = now; + } else { + return; + } + } log.debug("Success refresh clients: {} heartbeat.", CollectionUtils.collect(jdbcRegistryClients, IJdbcRegistryClient::getJdbcRegistryClientIdentify)); } catch (Exception ex) { log.error("Failed to refresh the client's term", ex); - switch (getServerState()) { - case STARTED: - transitionToSuspended(); - break; - case SUSPENDED: - if (System.currentTimeMillis() - lastSuccessHeartbeat > jdbcRegistryProperties.getSessionTimeout() - .toMillis()) { - transitionToDisconnected(); + long sessionTimeoutMillis = jdbcRegistryProperties.getSessionTimeout().toMillis(); + synchronized (this) { + currentState = serverState.get(); + if (currentState == JdbcRegistryServerState.STOPPED + || currentState == JdbcRegistryServerState.DISCONNECTED) { + return; + } + boolean sessionTimedOut = System.currentTimeMillis() - lastSuccessHeartbeat > sessionTimeoutMillis; + if (sessionTimedOut) { + if ((currentState == JdbcRegistryServerState.STARTED + || currentState == JdbcRegistryServerState.SUSPENDED) + && serverState.compareAndSet(currentState, JdbcRegistryServerState.DISCONNECTED)) { + doTriggerOnDisConnectedListener(); + } else { + log.debug("Failed to disconnect JdbcRegistryServer from state {}, current state is {}", + currentState, + serverState.get()); } - break; - default: - break; - } - } - } - - private void transitionToStarted() { - if (serverState.compareAndSet(JdbcRegistryServerState.SUSPENDED, JdbcRegistryServerState.STARTED)) { - doTriggerReconnectedListener(); - } - } - - private void transitionToSuspended() { - if (!serverState.compareAndSet(JdbcRegistryServerState.STARTED, JdbcRegistryServerState.SUSPENDED)) { - log.debug("Failed to transition JdbcRegistryServer to SUSPENDED; current state is {}", serverState.get()); - } - } - - private void transitionToDisconnected() { - while (true) { - JdbcRegistryServerState currentState = serverState.get(); - if (currentState != JdbcRegistryServerState.STARTED - && currentState != JdbcRegistryServerState.SUSPENDED) { - return; - } - if (serverState.compareAndSet(currentState, JdbcRegistryServerState.DISCONNECTED)) { - doTriggerOnDisConnectedListener(); - return; + } else if (currentState == JdbcRegistryServerState.STARTED + && !serverState.compareAndSet( + JdbcRegistryServerState.STARTED, JdbcRegistryServerState.SUSPENDED)) { + log.debug("Failed to suspend JdbcRegistryServer; current state is {}", serverState.get()); + } } - log.debug("Failed to transition JdbcRegistryServer from {} to DISCONNECTED; retrying", currentState); } } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java index 4645043653e0..cb85208b0303 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java @@ -27,6 +27,7 @@ import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryLockRepository; import java.time.Duration; +import java.util.Map; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -144,19 +145,55 @@ void refreshClientsHeartbeat_shouldDisconnectWhenHeartbeatRecordWasPurged() { } @Test - void refreshClientsHeartbeat_shouldDisconnectAfterSessionTimeoutWhenStartedHeartbeatRecordWasPurged() { + void refreshClientsHeartbeat_shouldDisconnectImmediatelyWhenStartedHeartbeatRecordWasPurgedAfterTimeout() { setServerState(JdbcRegistryServerState.STARTED); + ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", 0L); + Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenReturn(false); + + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.DISCONNECTED); + Mockito.verify(jdbcRegistryClientRepository).updateById(Mockito.any()); + Mockito.verify(connectionStateListener).onDisConnected(); + } + + @Test + void refreshClientsHeartbeat_shouldSuspendWhenStartedHeartbeatRecordWasPurgedBeforeTimeout() { + setServerState(JdbcRegistryServerState.STARTED); + ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", System.currentTimeMillis()); Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenReturn(false); ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.SUSPENDED); - ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", 0L); + Mockito.verify(connectionStateListener, Mockito.never()).onDisConnected(); + } + + @Test + void refreshClientsHeartbeat_shouldKeepPartialHeartbeatUpdateWhenLaterClientFails() { + IJdbcRegistryClient secondClient = Mockito.mock(IJdbcRegistryClient.class); + JdbcRegistryClientIdentify secondClientIdentify = new JdbcRegistryClientIdentify(2L, "second-client"); + Mockito.when(secondClient.getJdbcRegistryClientIdentify()).thenReturn(secondClientIdentify); + jdbcRegistryServer.registerClient(secondClient); + setServerState(JdbcRegistryServerState.STARTED); + ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", System.currentTimeMillis()); + AtomicInteger updateInvocations = new AtomicInteger(); + Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())) + .thenAnswer(invocation -> updateInvocations.incrementAndGet() == 1); + + @SuppressWarnings("unchecked") + Map heartbeatMap = + (Map) ReflectionTestUtils + .getField(jdbcRegistryServer, "jdbcRegistryClientDTOMap"); + long originalHeartbeat = heartbeatMap.get(CLIENT_IDENTIFY).getLastHeartbeatTime(); + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); - Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.DISCONNECTED); - Mockito.verify(jdbcRegistryClientRepository, Mockito.times(2)).updateById(Mockito.any()); - Mockito.verify(connectionStateListener).onDisConnected(); + Truth.assertThat(updateInvocations.get()).isEqualTo(2); + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.SUSPENDED); + Truth.assertThat(heartbeatMap.get(CLIENT_IDENTIFY).getLastHeartbeatTime()).isGreaterThan(originalHeartbeat); + Mockito.verify(connectionStateListener, Mockito.never()).onReconnected(); + Mockito.verify(connectionStateListener, Mockito.never()).onDisConnected(); } @Test @@ -191,6 +228,7 @@ void refreshClientsHeartbeat_shouldNotDisconnectWhenCloseWinsRace() throws Excep @Test void refreshClientsHeartbeat_shouldNotReconnectWhenCloseWinsSuccessfulHeartbeatRace() throws Exception { setServerState(JdbcRegistryServerState.SUSPENDED); + ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", 42L); CountDownLatch heartbeatUpdateStarted = new CountDownLatch(1); CountDownLatch allowHeartbeatUpdateToFinish = new CountDownLatch(1); Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenAnswer(invocation -> { @@ -214,6 +252,8 @@ void refreshClientsHeartbeat_shouldNotReconnectWhenCloseWinsSuccessfulHeartbeatR } Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.STOPPED); + Truth.assertThat((long) ReflectionTestUtils.getField(jdbcRegistryServer, "lastSuccessHeartbeat")) + .isEqualTo(42L); Mockito.verify(connectionStateListener, Mockito.never()).onReconnected(); Mockito.verify(connectionStateListener, Mockito.never()).onDisConnected(); } From fb69158bb54e5f1bcef2d0d2d70bae3a9c3e6c8a Mon Sep 17 00:00:00 2001 From: qiuyanjun Date: Sat, 26 Sep 2026 10:22:36 +0800 Subject: [PATCH 10/16] [Fix-18274][Registry] Stabilize JDBC heartbeat test baseline --- .../plugin/registry/jdbc/server/JdbcRegistryServerTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java index cb85208b0303..b4509ff222ad 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java @@ -185,13 +185,13 @@ void refreshClientsHeartbeat_shouldKeepPartialHeartbeatUpdateWhenLaterClientFail Map heartbeatMap = (Map) ReflectionTestUtils .getField(jdbcRegistryServer, "jdbcRegistryClientDTOMap"); - long originalHeartbeat = heartbeatMap.get(CLIENT_IDENTIFY).getLastHeartbeatTime(); + heartbeatMap.get(CLIENT_IDENTIFY).setLastHeartbeatTime(0L); ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); Truth.assertThat(updateInvocations.get()).isEqualTo(2); Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.SUSPENDED); - Truth.assertThat(heartbeatMap.get(CLIENT_IDENTIFY).getLastHeartbeatTime()).isGreaterThan(originalHeartbeat); + Truth.assertThat(heartbeatMap.get(CLIENT_IDENTIFY).getLastHeartbeatTime()).isGreaterThan(0L); Mockito.verify(connectionStateListener, Mockito.never()).onReconnected(); Mockito.verify(connectionStateListener, Mockito.never()).onDisConnected(); } From 3c85344d6737f6529423f5d1069fb13989a7d452 Mon Sep 17 00:00:00 2001 From: qiuyanjun Date: Sat, 26 Sep 2026 11:10:43 +0800 Subject: [PATCH 11/16] docs: add JDBC registry concurrency design --- ...-09-26-jdbc-registry-concurrency-design.md | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 docs/superpowers/specs/2026-09-26-jdbc-registry-concurrency-design.md diff --git a/docs/superpowers/specs/2026-09-26-jdbc-registry-concurrency-design.md b/docs/superpowers/specs/2026-09-26-jdbc-registry-concurrency-design.md new file mode 100644 index 000000000000..ff7de8b57c3a --- /dev/null +++ b/docs/superpowers/specs/2026-09-26-jdbc-registry-concurrency-design.md @@ -0,0 +1,50 @@ +# JDBC 注册中心并发安全设计 + +## 目标 + +修复 JDBC 注册中心在心跳、注册、注销、关闭和过期清理并发执行时的状态与数据库一致性问题，确保： + +- `STOPPED` 是关闭后的终态，心跳结果不能覆盖它。 +- 注册、注销和关闭不会留下孤立的数据库心跳记录。 +- 注销或关闭不会被旧心跳更新反向覆盖。 +- 过期清理不会删除刚刚成功续期的客户端及其临时数据、锁。 +- 连接状态监听器不会在服务器内部锁内执行。 +- 重复关闭具有幂等性，并且调用方不会在清理尚未完成时误以为关闭已完成。 + +## 并发模型 + +服务器使用生命周期锁保护状态和客户端注册表。注册表以客户端标识映射到不可替代的注册对象，注册对象包含客户端实例、心跳 DTO、活动标志和客户端级锁。 + +生命周期锁只保护本地状态和注册表变更，不包住监听器回调。每个客户端的数据库心跳更新、注销删除和关闭删除使用该客户端级锁串行化。这样可以让关闭先冻结注册表，再等待正在进行的客户端操作完成。 + +心跳任务先获取注册表快照，再逐个获取客户端级锁。客户端被注销或关闭时先从注册表移除并标记为非活动；已经取得快照的心跳在获取客户端锁后会再次检查活动标志，因此不会更新已注销客户端。 + +## 状态转换和回调 + +状态转换在生命周期锁内完成并验证源状态。只有转换成功的线程可以生成回调事件。回调在释放生命周期锁之后执行，避免回调阻塞关闭、产生锁循环或重入关闭后继续向已关闭的调度器提交任务。 + +允许在服务器启动前注册客户端，以保持现有启动顺序；服务器进入 `STOPPED` 或 `DISCONNECTED` 后拒绝新注册。注销只处理当前注册对象，旧客户端实例不能删除同标识的新注册对象。 + +## 过期清理 + +清理任务使用查询到的客户端标识和心跳时间执行条件删除，只有 `id` 和 `last_heartbeat_time` 同时仍匹配时才删除。心跳已经更新的记录不会被条件删除。 + +后续临时数据和锁清理只针对确认已经删除的客户端，并在清理前重新读取当前心跳记录，避免使用过期查询结果删除新注册客户端的数据。 + +## 关闭流程 + +关闭过程先在生命周期锁内将状态转换为 `STOPPED`、标记并冻结所有注册对象，然后关闭调度器。随后逐个等待客户端级操作完成并删除对应心跳记录，最后完成关闭信号。并发或重复调用 `close()` 复用同一个关闭完成信号。 + +## 测试范围 + +新增并发回归测试覆盖： + +- 注册与关闭并发； +- 注销与心跳更新并发； +- 注销后立即重新注册； +- 过期清理与心跳更新并发； +- 回调重入关闭； +- 重复关闭等待清理完成； +- 关闭与成功、失败心跳竞态。 + +每个测试先在现有实现上验证失败，再实现对应修复。 From 4ef9952952255bd7a17ce1ffabccf78c3db2e306 Mon Sep 17 00:00:00 2001 From: qiuyanjun Date: Sat, 26 Sep 2026 12:54:27 +0800 Subject: [PATCH 12/16] [Fix-18274][Registry] Fix JDBC registry concurrency races --- .../JdbcRegistryClientHeartbeatMapper.java | 7 + .../jdbc/mapper/JdbcRegistryDataMapper.java | 15 +- .../jdbc/mapper/JdbcRegistryLockMapper.java | 8 + .../JdbcRegistryClientRepository.java | 4 + .../JdbcRegistryDataRepository.java | 16 +- .../JdbcRegistryLockRepository.java | 4 + .../jdbc/server/IJdbcRegistryDataManager.java | 7 + .../jdbc/server/JdbcRegistryDataManager.java | 195 ++++++--- .../jdbc/server/JdbcRegistryLockManager.java | 110 +++-- .../jdbc/server/JdbcRegistryServer.java | 412 ++++++++++++------ .../jdbc/server/JdbcRegistryServerTest.java | 207 ++++++++- 11 files changed, 735 insertions(+), 250 deletions(-) diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryClientHeartbeatMapper.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryClientHeartbeatMapper.java index 2b8499bb4812..d126c1567521 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryClientHeartbeatMapper.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryClientHeartbeatMapper.java @@ -19,6 +19,8 @@ import org.apache.dolphinscheduler.plugin.registry.jdbc.model.DO.JdbcRegistryClientHeartbeat; +import org.apache.ibatis.annotations.Delete; +import org.apache.ibatis.annotations.Param; import org.apache.ibatis.annotations.Select; import java.util.List; @@ -30,4 +32,9 @@ public interface JdbcRegistryClientHeartbeatMapper extends BaseMapper selectAll(); + @Delete("delete from t_ds_jdbc_registry_client_heartbeat " + + "where id = #{id} and last_heartbeat_time = #{lastHeartbeatTime}") + int deleteByIdAndLastHeartbeatTime(@Param("id") Long id, + @Param("lastHeartbeatTime") Long lastHeartbeatTime); + } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryDataMapper.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryDataMapper.java index 261bfae4fe95..b382ba513686 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryDataMapper.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryDataMapper.java @@ -36,7 +36,10 @@ public interface JdbcRegistryDataMapper extends BaseMapper { JdbcRegistryData selectByKey(@Param("key") String key); @Delete("delete from t_ds_jdbc_registry_data where data_key = #{key}") - void deleteByKey(@Param("key") String key); + int deleteByKey(@Param("key") String key); + + @Delete("delete from t_ds_jdbc_registry_data where data_key = #{key} and id = #{id}") + int deleteByKeyAndId(@Param("key") String key, @Param("id") Long id); @Delete({""}) void deleteByClientIds(@Param("clientIds") List clientIds, @Param("dataType") String dataType); + @Delete("delete from t_ds_jdbc_registry_data " + + "where data_key = #{dataKey} " + + "and client_id = #{clientId} " + + "and data_type = 'EPHEMERAL' " + + "and not exists (" + + "select 1 from t_ds_jdbc_registry_client_heartbeat h " + + "where h.id = t_ds_jdbc_registry_data.client_id)") + int deleteEphemeralByKeyAndInactiveClient(@Param("dataKey") String dataKey, + @Param("clientId") Long clientId); + } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryLockMapper.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryLockMapper.java index 0639dfbc204d..5dbe13007778 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryLockMapper.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryLockMapper.java @@ -36,4 +36,12 @@ public interface JdbcRegistryLockMapper extends BaseMapper { "", ""}) void deleteByClientIds(@Param("clientIds") List clientIds); + + @Delete("delete from t_ds_jdbc_registry_lock " + + "where id = #{lockId} " + + "and client_id = #{clientId} " + + "and not exists (" + + "select 1 from t_ds_jdbc_registry_client_heartbeat h " + + "where h.id = t_ds_jdbc_registry_lock.client_id)") + int deleteByIdAndInactiveClient(@Param("lockId") Long lockId, @Param("clientId") Long clientId); } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryClientRepository.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryClientRepository.java index 1791f3c942aa..1f1e8984f02d 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryClientRepository.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryClientRepository.java @@ -52,6 +52,10 @@ public void deleteByIds(Collection clientIds) { jdbcRegistryClientHeartbeatMapper.deleteBatchIds(clientIds); } + public boolean deleteByIdAndLastHeartbeatTime(Long clientId, Long lastHeartbeatTime) { + return jdbcRegistryClientHeartbeatMapper.deleteByIdAndLastHeartbeatTime(clientId, lastHeartbeatTime) == 1; + } + public boolean updateById(JdbcRegistryClientHeartbeatDTO jdbcRegistryClientHeartbeatDTO) { JdbcRegistryClientHeartbeat jdbcRegistryClientHeartbeat = JdbcRegistryClientHeartbeatDTO.toJdbcRegistryClientHeartbeat(jdbcRegistryClientHeartbeatDTO); diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryDataRepository.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryDataRepository.java index e91ee0a90eb8..373204439463 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryDataRepository.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryDataRepository.java @@ -47,8 +47,16 @@ public Optional selectByKey(String key) { .map(JdbcRegistryDataDTO::fromJdbcRegistryData); } - public void deleteByKey(String key) { - jdbcRegistryDataMapper.deleteByKey(key); + public boolean deleteByKey(String key) { + return jdbcRegistryDataMapper.deleteByKey(key) == 1; + } + + public boolean deleteByKeyAndId(String key, Long id) { + return jdbcRegistryDataMapper.deleteByKeyAndId(key, id) == 1; + } + + public boolean deleteEphemeralByKeyAndInactiveClient(String dataKey, Long clientId) { + return jdbcRegistryDataMapper.deleteEphemeralByKeyAndInactiveClient(dataKey, clientId) == 1; } public void insert(JdbcRegistryDataDTO jdbcRegistryData) { @@ -57,7 +65,7 @@ public void insert(JdbcRegistryDataDTO jdbcRegistryData) { jdbcRegistryData.setId(jdbcRegistryDataDO.getId()); } - public void updateById(JdbcRegistryDataDTO jdbcRegistryDataDTO) { - jdbcRegistryDataMapper.updateById(JdbcRegistryDataDTO.toJdbcRegistryData(jdbcRegistryDataDTO)); + public boolean updateById(JdbcRegistryDataDTO jdbcRegistryDataDTO) { + return jdbcRegistryDataMapper.updateById(JdbcRegistryDataDTO.toJdbcRegistryData(jdbcRegistryDataDTO)) == 1; } } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryLockRepository.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryLockRepository.java index 63d7f7f5bee4..b45fffd64df0 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryLockRepository.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryLockRepository.java @@ -52,4 +52,8 @@ public void insert(JdbcRegistryLockDTO jdbcRegistryLock) { public void deleteById(Long id) { jdbcRegistryLockMapper.deleteById(id); } + + public boolean deleteByIdAndInactiveClient(Long lockId, Long clientId) { + return jdbcRegistryLockMapper.deleteByIdAndInactiveClient(lockId, clientId) == 1; + } } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/IJdbcRegistryDataManager.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/IJdbcRegistryDataManager.java index 86db4de6abf7..e2ef19790026 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/IJdbcRegistryDataManager.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/IJdbcRegistryDataManager.java @@ -57,4 +57,11 @@ public interface IJdbcRegistryDataManager { * Delete the {@link JdbcRegistryDataDTO} by key. */ void deleteJdbcRegistryDataByKey(String key); + + /** + * Delete an ephemeral row only when its client heartbeat no longer exists. + * + * @return {@code true} when the row was actually deleted + */ + boolean deleteEphemeralDataIfClientInactive(JdbcRegistryDataDTO data); } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryDataManager.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryDataManager.java index b49ebc6b2f2a..65f3a4b53c22 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryDataManager.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryDataManager.java @@ -34,9 +34,11 @@ import java.util.Date; import java.util.List; import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.ReentrantLock; import java.util.stream.Collectors; import lombok.extern.slf4j.Slf4j; @@ -65,6 +67,12 @@ public class JdbcRegistryDataManager private final List> registryRowChangeListeners; + /** + * Operations for the same registry key must be serialized locally. The database unique key still protects + * multiple registry server instances, while this lock prevents stale read/modify/delete races in one instance. + */ + private final ConcurrentHashMap dataKeyLocks = new ConcurrentHashMap<>(); + private long lastDetectedJdbcRegistryDataChangeEventId = -1; public JdbcRegistryDataManager(JdbcRegistryProperties registryProperties, @@ -172,73 +180,116 @@ public void putJdbcRegistryData(Long clientId, String key, String value, DataTyp checkNotNull(key); checkNotNull(dataType); - final Optional jdbcRegistryDataOptional = jdbcRegistryDataRepository.selectByKey(key); - - jdbcRegistryTransactionTemplate.execute(status -> { - if (jdbcRegistryDataOptional.isPresent()) { - JdbcRegistryDataDTO jdbcRegistryData = jdbcRegistryDataOptional.get(); - if (!dataType.name().equals(jdbcRegistryData.getDataType())) { - throw new UnsupportedOperationException("The data type: " + jdbcRegistryData.getDataType() - + " of the key: " + key + " cannot be updated"); - } + ReentrantLock keyLock = dataKeyLocks.computeIfAbsent(key, ignored -> new ReentrantLock()); + keyLock.lock(); + try { + jdbcRegistryTransactionTemplate.execute(status -> { + Optional jdbcRegistryDataOptional = jdbcRegistryDataRepository.selectByKey(key); + if (jdbcRegistryDataOptional.isPresent()) { + JdbcRegistryDataDTO jdbcRegistryData = jdbcRegistryDataOptional.get(); + if (!dataType.name().equals(jdbcRegistryData.getDataType())) { + throw new UnsupportedOperationException("The data type: " + jdbcRegistryData.getDataType() + + " of the key: " + key + " cannot be updated"); + } - if (DataType.EPHEMERAL.name().equals(jdbcRegistryData.getDataType())) { - if (!jdbcRegistryData.getClientId().equals(clientId)) { + if (DataType.EPHEMERAL.name().equals(jdbcRegistryData.getDataType()) + && !jdbcRegistryData.getClientId().equals(clientId)) { throw new UnsupportedOperationException( "The EPHEMERAL data: " + key + " can only be updated by its owner: " + jdbcRegistryData.getClientId() + " but not: " + clientId); } - } - jdbcRegistryData.setDataValue(value); - jdbcRegistryData.setLastUpdateTime(new Date()); - jdbcRegistryDataRepository.updateById(jdbcRegistryData); - - JdbcRegistryDataChangeEventDTO jdbcRegistryDataChangeEvent = JdbcRegistryDataChangeEventDTO.builder() - .jdbcRegistryData(jdbcRegistryData) - .eventType(JdbcRegistryDataChangeEventDTO.EventType.UPDATE) - .createTime(new Date()) - .build(); - jdbcRegistryDataChangeEventRepository.insert(jdbcRegistryDataChangeEvent); - } else { - JdbcRegistryDataDTO jdbcRegistryDataDTO = JdbcRegistryDataDTO.builder() - .clientId(clientId) - .dataKey(key) - .dataValue(value) - .dataType(dataType.name()) - .createTime(new Date()) - .lastUpdateTime(new Date()) - .build(); - jdbcRegistryDataRepository.insert(jdbcRegistryDataDTO); - JdbcRegistryDataChangeEventDTO registryDataChangeEvent = JdbcRegistryDataChangeEventDTO.builder() - .jdbcRegistryData(jdbcRegistryDataDTO) - .eventType(JdbcRegistryDataChangeEventDTO.EventType.ADD) - .createTime(new Date()) - .build(); - jdbcRegistryDataChangeEventRepository.insert(registryDataChangeEvent); - } - return null; - }); + jdbcRegistryData.setDataValue(value); + jdbcRegistryData.setLastUpdateTime(new Date()); + if (!jdbcRegistryDataRepository.updateById(jdbcRegistryData)) { + throw new IllegalStateException("The registry data was concurrently removed: " + key); + } + + JdbcRegistryDataChangeEventDTO jdbcRegistryDataChangeEvent = + JdbcRegistryDataChangeEventDTO.builder() + .jdbcRegistryData(jdbcRegistryData) + .eventType(JdbcRegistryDataChangeEventDTO.EventType.UPDATE) + .createTime(new Date()) + .build(); + jdbcRegistryDataChangeEventRepository.insert(jdbcRegistryDataChangeEvent); + } else { + JdbcRegistryDataDTO jdbcRegistryDataDTO = JdbcRegistryDataDTO.builder() + .clientId(clientId) + .dataKey(key) + .dataValue(value) + .dataType(dataType.name()) + .createTime(new Date()) + .lastUpdateTime(new Date()) + .build(); + jdbcRegistryDataRepository.insert(jdbcRegistryDataDTO); + JdbcRegistryDataChangeEventDTO registryDataChangeEvent = + JdbcRegistryDataChangeEventDTO.builder() + .jdbcRegistryData(jdbcRegistryDataDTO) + .eventType(JdbcRegistryDataChangeEventDTO.EventType.ADD) + .createTime(new Date()) + .build(); + jdbcRegistryDataChangeEventRepository.insert(registryDataChangeEvent); + } + return null; + }); + } finally { + keyLock.unlock(); + } } @Override public void deleteJdbcRegistryDataByKey(String key) { checkNotNull(key); - Optional jdbcRegistryDataOptional = jdbcRegistryDataRepository.selectByKey(key); - if (!jdbcRegistryDataOptional.isPresent()) { - return; + ReentrantLock keyLock = dataKeyLocks.computeIfAbsent(key, ignored -> new ReentrantLock()); + keyLock.lock(); + try { + Optional jdbcRegistryDataOptional = jdbcRegistryDataRepository.selectByKey(key); + if (!jdbcRegistryDataOptional.isPresent()) { + return; + } + jdbcRegistryTransactionTemplate.execute(status -> { + if (!jdbcRegistryDataRepository.deleteByKeyAndId(key, jdbcRegistryDataOptional.get().getId())) { + return null; + } + final JdbcRegistryDataChangeEventDTO registryDataChangeEvent = + JdbcRegistryDataChangeEventDTO.builder() + .jdbcRegistryData(jdbcRegistryDataOptional.get()) + .eventType(JdbcRegistryDataChangeEventDTO.EventType.DELETE) + .createTime(new Date()) + .build(); + jdbcRegistryDataChangeEventRepository.insert(registryDataChangeEvent); + return null; + }); + } finally { + keyLock.unlock(); + } + } + + @Override + public boolean deleteEphemeralDataIfClientInactive(JdbcRegistryDataDTO data) { + checkNotNull(data); + ReentrantLock keyLock = dataKeyLocks.computeIfAbsent(data.getDataKey(), ignored -> new ReentrantLock()); + keyLock.lock(); + try { + Boolean deleted = jdbcRegistryTransactionTemplate.execute(status -> { + if (!jdbcRegistryDataRepository.deleteEphemeralByKeyAndInactiveClient( + data.getDataKey(), data.getClientId())) { + return false; + } + final JdbcRegistryDataChangeEventDTO registryDataChangeEvent = + JdbcRegistryDataChangeEventDTO.builder() + .jdbcRegistryData(data) + .eventType(JdbcRegistryDataChangeEventDTO.EventType.DELETE) + .createTime(new Date()) + .build(); + jdbcRegistryDataChangeEventRepository.insert(registryDataChangeEvent); + return true; + }); + return Boolean.TRUE.equals(deleted); + } finally { + keyLock.unlock(); } - jdbcRegistryTransactionTemplate.execute(status -> { - jdbcRegistryDataRepository.deleteByKey(key); - final JdbcRegistryDataChangeEventDTO registryDataChangeEvent = JdbcRegistryDataChangeEventDTO.builder() - .jdbcRegistryData(jdbcRegistryDataOptional.get()) - .eventType(JdbcRegistryDataChangeEventDTO.EventType.DELETE) - .createTime(new Date()) - .build(); - jdbcRegistryDataChangeEventRepository.insert(registryDataChangeEvent); - return null; - }); } private void doTriggerJdbcRegistryDataAddedListener(List valuesToAdd) { @@ -247,11 +298,13 @@ private void doTriggerJdbcRegistryDataAddedListener(List va } log.debug("Trigger:onJdbcRegistryDataAdded: {}", valuesToAdd); valuesToAdd.forEach(jdbcRegistryData -> { - try { - registryRowChangeListeners.forEach(listener -> listener.onRegistryRowAdded(jdbcRegistryData)); - } catch (Exception ex) { - log.error("Trigger:onRegistryRowAdded: {} failed", jdbcRegistryData, ex); - } + registryRowChangeListeners.forEach(listener -> { + try { + listener.onRegistryRowAdded(jdbcRegistryData); + } catch (Exception ex) { + log.error("Trigger:onRegistryRowAdded: {} failed", jdbcRegistryData, ex); + } + }); }); } @@ -261,11 +314,13 @@ private void doTriggerJdbcRegistryDataRemovedListener(List } log.debug("Trigger:onJdbcRegistryDataDeleted: {}", valuesToRemoved); valuesToRemoved.forEach(jdbcRegistryData -> { - try { - registryRowChangeListeners.forEach(listener -> listener.onRegistryRowDeleted(jdbcRegistryData)); - } catch (Exception ex) { - log.error("Trigger:onRegistryRowAdded: {} failed", jdbcRegistryData, ex); - } + registryRowChangeListeners.forEach(listener -> { + try { + listener.onRegistryRowDeleted(jdbcRegistryData); + } catch (Exception ex) { + log.error("Trigger:onRegistryRowDeleted: {} failed", jdbcRegistryData, ex); + } + }); }); } @@ -275,11 +330,13 @@ private void doTriggerJdbcRegistryDataUpdatedListener(List } log.debug("Trigger:onJdbcRegistryDataUpdated: {}", valuesToUpdated); valuesToUpdated.forEach(jdbcRegistryData -> { - try { - registryRowChangeListeners.forEach(listener -> listener.onRegistryRowUpdated(jdbcRegistryData)); - } catch (Exception ex) { - log.error("Trigger:onRegistryRowAdded: {} failed", jdbcRegistryData, ex); - } + registryRowChangeListeners.forEach(listener -> { + try { + listener.onRegistryRowUpdated(jdbcRegistryData); + } catch (Exception ex) { + log.error("Trigger:onRegistryRowUpdated: {} failed", jdbcRegistryData, ex); + } + }); }); } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryLockManager.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryLockManager.java index 68ba187778b0..aae62109ddab 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryLockManager.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryLockManager.java @@ -45,6 +45,8 @@ public class JdbcRegistryLockManager implements IJdbcRegistryLockManager { // lockKey -> LockEntry private final Map jdbcRegistryLockHolderMap = new ConcurrentHashMap<>(); + private final Object lockHolderMonitor = new Object(); + public JdbcRegistryLockManager(JdbcRegistryProperties jdbcRegistryProperties, JdbcRegistryLockRepository jdbcRegistryLockRepository) { this.jdbcRegistryProperties = jdbcRegistryProperties; @@ -55,7 +57,7 @@ public JdbcRegistryLockManager(JdbcRegistryProperties jdbcRegistryProperties, public void acquireJdbcRegistryLock(Long clientId, String lockKey) { String lockOwner = LockUtils.getLockOwner(); while (true) { - if (tryReenterLock(lockKey, lockOwner)) { + if (tryReenterLock(clientId, lockKey, lockOwner)) { return; } JdbcRegistryLockDTO jdbcRegistryLock = JdbcRegistryLockDTO.builder() @@ -65,31 +67,39 @@ public void acquireJdbcRegistryLock(Long clientId, String lockKey) { .createTime(new Date()) .build(); try { - jdbcRegistryLockRepository.insert(jdbcRegistryLock); - if (jdbcRegistryLock != null) { + synchronized (lockHolderMonitor) { + jdbcRegistryLockRepository.insert(jdbcRegistryLock); jdbcRegistryLockHolderMap.put(lockKey, LockEntry.builder() .lockKey(lockKey) .lockOwner(lockOwner) .jdbcRegistryLock(jdbcRegistryLock) .build()); - return; } log.debug("{} acquire the lock {} success", lockOwner, lockKey); + return; } catch (DuplicateKeyException duplicateKeyException) { // The lock is already exist, wait it release. - continue; + log.debug("{} failed to acquire the lock {}, it is held by another owner", lockOwner, lockKey); } log.debug("{} acquire the lock {} failed try again", lockOwner, lockKey); // acquire failed, wait and try again - ThreadUtils.sleep(jdbcRegistryProperties.getHeartbeatRefreshInterval().toMillis()); + if (!sleepBeforeRetry(jdbcRegistryProperties.getHeartbeatRefreshInterval().toMillis())) { + throw new IllegalStateException("Interrupted while acquiring the lock: " + lockKey); + } } } - private boolean tryReenterLock(String lockKey, String lockAcquirer) { - LockEntry lockEntry = jdbcRegistryLockHolderMap.get(lockKey); - if (lockEntry != null && lockAcquirer.equals(lockEntry.getLockOwner())) { - lockEntry.lockCount.incrementAndGet(); - return true; + private boolean tryReenterLock(Long clientId, String lockKey, String lockAcquirer) { + synchronized (lockHolderMonitor) { + LockEntry lockEntry = jdbcRegistryLockHolderMap.get(lockKey); + if (lockEntry != null && lockAcquirer.equals(lockEntry.getLockOwner())) { + if (!clientId.equals(lockEntry.getJdbcRegistryLock().getClientId())) { + throw new UnsupportedOperationException( + "The client " + clientId + " is not the lock owner of the lock: " + lockKey); + } + lockEntry.lockCount.incrementAndGet(); + return true; + } } return false; } @@ -99,7 +109,7 @@ public boolean acquireJdbcRegistryLock(Long clientId, String lockKey, long timeo String lockOwner = LockUtils.getLockOwner(); long start = System.currentTimeMillis(); while (System.currentTimeMillis() - start <= timeout) { - if (tryReenterLock(lockKey, lockOwner)) { + if (tryReenterLock(clientId, lockKey, lockOwner)) { return true; } JdbcRegistryLockDTO jdbcRegistryLock = JdbcRegistryLockDTO.builder() @@ -109,47 +119,83 @@ public boolean acquireJdbcRegistryLock(Long clientId, String lockKey, long timeo .createTime(new Date()) .build(); try { - jdbcRegistryLockRepository.insert(jdbcRegistryLock); - if (jdbcRegistryLock != null) { + synchronized (lockHolderMonitor) { + jdbcRegistryLockRepository.insert(jdbcRegistryLock); jdbcRegistryLockHolderMap.put(lockKey, LockEntry.builder() .lockKey(lockKey) .lockOwner(lockOwner) .jdbcRegistryLock(jdbcRegistryLock) .build()); - return true; } log.debug("{} acquire the lock {} success", lockOwner, lockKey); + return true; } catch (DuplicateKeyException duplicateKeyException) { // The lock is already exist, wait it release. - continue; + log.debug("{} failed to acquire the lock {}, it is held by another owner", lockOwner, lockKey); } log.debug("{} acquire the lock {} failed try again", lockOwner, lockKey); // acquire failed, wait and try again - ThreadUtils.sleep(jdbcRegistryProperties.getHeartbeatRefreshInterval().toMillis()); + long remaining = timeout - (System.currentTimeMillis() - start); + if (remaining <= 0 || !sleepBeforeRetry(Math.min( + remaining, jdbcRegistryProperties.getHeartbeatRefreshInterval().toMillis()))) { + return false; + } } return false; } + private boolean sleepBeforeRetry(long millis) { + if (millis <= 0 || Thread.currentThread().isInterrupted()) { + return false; + } + ThreadUtils.sleep(millis); + return !Thread.currentThread().isInterrupted(); + } + @Override public void releaseJdbcRegistryLock(Long clientId, String lockKey) { String lockOwner = LockUtils.getLockOwner(); - LockEntry lockEntry = jdbcRegistryLockHolderMap.get(lockKey); - if (lockEntry == null || !lockOwner.equals(lockEntry.getLockOwner())) { - return; - } - if (!clientId.equals(lockEntry.getJdbcRegistryLock().getClientId())) { - throw new UnsupportedOperationException( - "The client " + clientId + " is not the lock owner of the lock: " + lockKey); - } - int newLockCount = lockEntry.lockCount.decrementAndGet(); - if (newLockCount > 0) { - return; - } - if (newLockCount < 0) { - throw new IllegalMonitorStateException("Jdbc lock count has gone negative for lock: " + lockKey); + LockEntry lockEntry; + synchronized (lockHolderMonitor) { + lockEntry = jdbcRegistryLockHolderMap.get(lockKey); + if (lockEntry == null || !lockOwner.equals(lockEntry.getLockOwner())) { + return; + } + if (!clientId.equals(lockEntry.getJdbcRegistryLock().getClientId())) { + throw new UnsupportedOperationException( + "The client " + clientId + " is not the lock owner of the lock: " + lockKey); + } + int newLockCount = lockEntry.lockCount.decrementAndGet(); + if (newLockCount > 0) { + return; + } + if (newLockCount < 0) { + lockEntry.lockCount.incrementAndGet(); + throw new IllegalMonitorStateException("Jdbc lock count has gone negative for lock: " + lockKey); + } + // Remove before deleting from the database so a replacement entry cannot be removed by this release. + jdbcRegistryLockHolderMap.remove(lockKey, lockEntry); } jdbcRegistryLockRepository.deleteById(lockEntry.getJdbcRegistryLock().getId()); - jdbcRegistryLockHolderMap.remove(lockKey); + } + + /** + * Delete an inactive lock while serializing it with local acquire/release operations. + */ + boolean deleteIfInactive(JdbcRegistryLockDTO jdbcRegistryLock) { + synchronized (lockHolderMonitor) { + boolean deleted = jdbcRegistryLockRepository.deleteByIdAndInactiveClient( + jdbcRegistryLock.getId(), jdbcRegistryLock.getClientId()); + if (!deleted) { + return false; + } + LockEntry lockEntry = jdbcRegistryLockHolderMap.get(jdbcRegistryLock.getLockKey()); + if (lockEntry != null + && jdbcRegistryLock.getId().equals(lockEntry.getJdbcRegistryLock().getId())) { + jdbcRegistryLockHolderMap.remove(jdbcRegistryLock.getLockKey(), lockEntry); + } + return true; + } } @Data diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java index 71ddaf3810d5..55231f9cab27 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java @@ -32,20 +32,22 @@ import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryLockRepository; import org.apache.dolphinscheduler.registry.api.RegistryException; -import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.lang3.time.StopWatch; +import java.util.ArrayList; import java.util.Collection; import java.util.Date; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.locks.ReentrantLock; import java.util.stream.Collectors; import lombok.SneakyThrows; @@ -74,17 +76,41 @@ public class JdbcRegistryServer implements IJdbcRegistryServer { private final AtomicReference serverState = new AtomicReference<>(JdbcRegistryServerState.INIT); - private final List jdbcRegistryClients = new CopyOnWriteArrayList<>(); - private final List connectionStateListeners = new CopyOnWriteArrayList<>(); - private final Map jdbcRegistryClientDTOMap = - new ConcurrentHashMap<>(); + private final ReentrantLock lifecycleLock = new ReentrantLock(); + + private final Map clientRegistrations = new LinkedHashMap<>(); + + private final CompletableFuture closeCompletion = new CompletableFuture<>(); private final ScheduledExecutorService schedulerThreadExecutor; private volatile long lastSuccessHeartbeat; + private static final class ClientRegistration { + + private final IJdbcRegistryClient client; + private final JdbcRegistryClientHeartbeatDTO heartbeat; + private final ReentrantLock lock = new ReentrantLock(); + private volatile boolean active = true; + + private ClientRegistration(IJdbcRegistryClient client, JdbcRegistryClientHeartbeatDTO heartbeat) { + this.client = client; + this.heartbeat = heartbeat; + } + } + + private static final class HeartbeatUpdateException extends RuntimeException { + + private final ClientRegistration registration; + + private HeartbeatUpdateException(ClientRegistration registration) { + super("The client heartbeat record no longer exists: " + registration.heartbeat.getId()); + this.registration = registration; + } + } + public JdbcRegistryServer(JdbcRegistryDataRepository jdbcRegistryDataRepository, JdbcRegistryLockRepository jdbcRegistryLockRepository, JdbcRegistryClientRepository jdbcRegistryClientRepository, @@ -106,30 +132,52 @@ public JdbcRegistryServer(JdbcRegistryDataRepository jdbcRegistryDataRepository, } @Override - public synchronized void start() { - if (serverState.get() != JdbcRegistryServerState.INIT) { - // The server is already started or stopped, will not start again. - return; + public void start() { + lifecycleLock.lock(); + try { + if (serverState.get() != JdbcRegistryServerState.INIT) { + // The server is already started or stopped, will not start again. + return; + } + // Start the Purge thread + // The Purge thread will clear the invalidated data + purgeInvalidJdbcRegistryMetadata(); + schedulerThreadExecutor.scheduleWithFixedDelay( + this::purgeInvalidJdbcRegistryMetadata, + jdbcRegistryProperties.getSessionTimeout().toMillis(), + jdbcRegistryProperties.getSessionTimeout().toMillis(), + TimeUnit.MILLISECONDS); + jdbcRegistryDataManager.start(); + if (!serverState.compareAndSet(JdbcRegistryServerState.INIT, JdbcRegistryServerState.STARTED)) { + log.warn("The JdbcRegistryServer state changed before startup completed: {}", serverState.get()); + return; + } + } finally { + lifecycleLock.unlock(); } - // Start the Purge thread - // The Purge thread will clear the invalidated data - purgeInvalidJdbcRegistryMetadata(); - schedulerThreadExecutor.scheduleWithFixedDelay( - this::purgeInvalidJdbcRegistryMetadata, - jdbcRegistryProperties.getSessionTimeout().toMillis(), - jdbcRegistryProperties.getSessionTimeout().toMillis(), - TimeUnit.MILLISECONDS); - jdbcRegistryDataManager.start(); - if (!serverState.compareAndSet(JdbcRegistryServerState.INIT, JdbcRegistryServerState.STARTED)) { - log.warn("The JdbcRegistryServer state changed before startup completed: {}", serverState.get()); - return; + + lifecycleLock.lock(); + try { + if (serverState.get() != JdbcRegistryServerState.STARTED) { + return; + } + } finally { + lifecycleLock.unlock(); } doTriggerOnConnectedListener(); - schedulerThreadExecutor.scheduleWithFixedDelay( - this::refreshClientsHeartbeat, - 0, - jdbcRegistryProperties.getHeartbeatRefreshInterval().toMillis(), - TimeUnit.MILLISECONDS); + + lifecycleLock.lock(); + try { + if (serverState.get() == JdbcRegistryServerState.STARTED && !schedulerThreadExecutor.isShutdown()) { + schedulerThreadExecutor.scheduleWithFixedDelay( + this::refreshClientsHeartbeat, + 0, + jdbcRegistryProperties.getHeartbeatRefreshInterval().toMillis(), + TimeUnit.MILLISECONDS); + } + } finally { + lifecycleLock.unlock(); + } } @SneakyThrows @@ -150,12 +198,23 @@ public void registerClient(IJdbcRegistryClient jdbcRegistryClient) { .lastHeartbeatTime(System.currentTimeMillis()) .build(); - if (jdbcRegistryClientDTOMap.containsKey(jdbcRegistryClientIdentify)) { - throw new IllegalArgumentException("The client is already registered: " + jdbcRegistryClientIdentify); + lifecycleLock.lock(); + try { + JdbcRegistryServerState currentState = serverState.get(); + if (currentState == JdbcRegistryServerState.STOPPED + || currentState == JdbcRegistryServerState.DISCONNECTED) { + throw new IllegalStateException("Cannot register a client when the JdbcRegistryServer is " + + currentState); + } + if (clientRegistrations.containsKey(jdbcRegistryClientIdentify)) { + throw new IllegalArgumentException("The client is already registered: " + jdbcRegistryClientIdentify); + } + jdbcRegistryClientRepository.insert(registryClientDTO); + clientRegistrations.put( + jdbcRegistryClientIdentify, new ClientRegistration(jdbcRegistryClient, registryClientDTO)); + } finally { + lifecycleLock.unlock(); } - jdbcRegistryClientRepository.insert(registryClientDTO); - jdbcRegistryClients.add(jdbcRegistryClient); - jdbcRegistryClientDTOMap.put(jdbcRegistryClientIdentify, registryClientDTO); } @Override @@ -164,10 +223,23 @@ public void deregisterClient(IJdbcRegistryClient jdbcRegistryClient) { final JdbcRegistryClientIdentify clientIdentify = jdbcRegistryClient.getJdbcRegistryClientIdentify(); checkNotNull(clientIdentify); - jdbcRegistryClients.removeIf(client -> clientIdentify.equals(client.getJdbcRegistryClientIdentify())); - jdbcRegistryClientDTOMap.remove(jdbcRegistryClient.getJdbcRegistryClientIdentify()); - - doPurgeJdbcRegistryClientInDB(Lists.newArrayList(clientIdentify.getClientId())); + lifecycleLock.lock(); + try { + ClientRegistration registration = clientRegistrations.get(clientIdentify); + if (registration == null || registration.client != jdbcRegistryClient) { + return; + } + registration.active = false; + clientRegistrations.remove(clientIdentify, registration); + registration.lock.lock(); + try { + purgeClientRegistration(registration); + } finally { + registration.lock.unlock(); + } + } finally { + lifecycleLock.unlock(); + } } @Override @@ -260,27 +332,58 @@ public void releaseJdbcRegistryLock(Long clientId, String lockKey) { @Override public void close() { - synchronized (this) { + List registrationsToClose; + boolean waitForClose = false; + lifecycleLock.lock(); + try { JdbcRegistryServerState currentState = serverState.get(); if (currentState == JdbcRegistryServerState.STOPPED) { - log.warn("The JdbcRegistryServer is already STOPPED."); - return; - } - if (!serverState.compareAndSet(currentState, JdbcRegistryServerState.STOPPED)) { + waitForClose = true; + registrationsToClose = null; + } else if (!serverState.compareAndSet(currentState, JdbcRegistryServerState.STOPPED)) { log.warn("Failed to stop JdbcRegistryServer from state {}, current state is {}", currentState, serverState.get()); return; + } else { + registrationsToClose = new ArrayList<>(clientRegistrations.values()); + clientRegistrations.clear(); + registrationsToClose.forEach(registration -> registration.active = false); } + } finally { + lifecycleLock.unlock(); + } + + if (waitForClose) { + closeCompletion.join(); + return; + } + + try { + schedulerThreadExecutor.shutdownNow(); + for (ClientRegistration registration : registrationsToClose) { + registration.lock.lock(); + try { + purgeClientRegistration(registration); + } finally { + registration.lock.unlock(); + } + } + try { + if (!schedulerThreadExecutor.awaitTermination( + Math.max(1, jdbcRegistryProperties.getHeartbeatRefreshInterval().toMillis()), + TimeUnit.MILLISECONDS)) { + log.warn("The JdbcRegistryServer scheduler did not terminate within the close timeout."); + } + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + throw new IllegalStateException("Interrupted while closing the JdbcRegistryServer", ex); + } + closeCompletion.complete(null); + } catch (RuntimeException ex) { + closeCompletion.completeExceptionally(ex); + throw ex; } - schedulerThreadExecutor.shutdown(); - List clientIds = jdbcRegistryClients.stream() - .map(IJdbcRegistryClient::getJdbcRegistryClientIdentify) - .map(JdbcRegistryClientIdentify::getClientId) - .collect(Collectors.toList()); - doPurgeJdbcRegistryClientInDB(clientIds); - jdbcRegistryClients.clear(); - jdbcRegistryClientDTOMap.clear(); } private void purgeInvalidJdbcRegistryMetadata() { @@ -292,85 +395,98 @@ private void purgeInvalidJdbcRegistryMetadata() { } // remove the client which is already dead from the registry, and remove it's related data and lock. final List jdbcRegistryClients = jdbcRegistryClientRepository.queryAll(); - final Set deadJdbcRegistryClientIds = jdbcRegistryClients - .stream() + jdbcRegistryClients.stream() .filter(JdbcRegistryClientHeartbeatDTO::isDead) - .map(JdbcRegistryClientHeartbeatDTO::getId) - .collect(Collectors.toSet()); - doPurgeJdbcRegistryClientInDB(deadJdbcRegistryClientIds); + .filter(jdbcRegistryClient -> jdbcRegistryClientRepository.deleteByIdAndLastHeartbeatTime( + jdbcRegistryClient.getId(), jdbcRegistryClient.getLastHeartbeatTime())) + .forEach(jdbcRegistryClient -> log.info( + "Success delete dead jdbcRegistryClient: {}", jdbcRegistryClient.getId())); - // remove the data and lock which client is not exist. - final Set existJdbcRegistryClientIds = jdbcRegistryClients - .stream() - .map(JdbcRegistryClientHeartbeatDTO::getId) - .filter(id -> !deadJdbcRegistryClientIds.contains(id)) - .collect(Collectors.toSet()); + // Remove data and locks only while the database still confirms that the owner has no heartbeat. + purgeInactiveMetadata(null); + stopWatch.stop(); + log.debug("Success purge invalid jdbcRegistryMetadata, cost: {} ms", stopWatch.getTime()); + } + + private void purgeClientRegistration(ClientRegistration registration) { + Long clientId = registration.heartbeat.getId(); + log.info("Begin to delete dead jdbcRegistryClient: {}", clientId); + jdbcRegistryClientRepository.deleteByIdAndLastHeartbeatTime( + clientId, registration.heartbeat.getLastHeartbeatTime()); + purgeInactiveMetadata(Lists.newArrayList(clientId)); + log.info("Success delete dead jdbcRegistryClient: {}", clientId); + } + + private void purgeInactiveMetadata(Collection candidateClientIds) { + Set candidates = candidateClientIds == null + ? null + : candidateClientIds.stream().collect(Collectors.toSet()); jdbcRegistryDataManager.getAllJdbcRegistryData() .stream() - .filter(jdbcRegistryDataDTO -> !existJdbcRegistryClientIds.contains(jdbcRegistryDataDTO.getClientId())) .filter(jdbcRegistryDataDTO -> DataType.EPHEMERAL.name().equals(jdbcRegistryDataDTO.getDataType())) + .filter(jdbcRegistryDataDTO -> candidates == null + || candidates.contains(jdbcRegistryDataDTO.getClientId())) .forEach(jdbcRegistryData -> { log.info("Remove the JdbcRegistryData: {} which client is not exist in the registry", jdbcRegistryData); - jdbcRegistryDataManager.deleteJdbcRegistryDataByKey(jdbcRegistryData.getDataKey()); + jdbcRegistryDataManager.deleteEphemeralDataIfClientInactive(jdbcRegistryData); }); jdbcRegistryLockRepository.queryAll() .stream() - .filter(jdbcRegistryLockDTO -> !existJdbcRegistryClientIds.contains(jdbcRegistryLockDTO.getClientId())) - .forEach(jdbcRegistryLockDTO -> { + .filter(jdbcRegistryLockDTO -> candidates == null + || candidates.contains(jdbcRegistryLockDTO.getClientId())) + .forEach(jdbcRegistryLock -> { log.info("Remove the JdbcRegistryLock: {} which client is not exist in the registry", - jdbcRegistryLockDTO); - jdbcRegistryLockRepository.deleteById(jdbcRegistryLockDTO.getId()); + jdbcRegistryLock); + jdbcRegistryLockManager.deleteIfInactive(jdbcRegistryLock); }); - stopWatch.stop(); - log.debug("Success purge invalid jdbcRegistryMetadata, cost: {} ms", stopWatch.getTime()); - } - - private void doPurgeJdbcRegistryClientInDB(final Collection jdbcRegistryClientIds) { - if (CollectionUtils.isEmpty(jdbcRegistryClientIds)) { - return; - } - log.info("Begin to delete dead jdbcRegistryClient: {}", jdbcRegistryClientIds); - jdbcRegistryClientRepository.deleteByIds(jdbcRegistryClientIds); - log.info("Success delete dead jdbcRegistryClient: {}", jdbcRegistryClientIds); } private void refreshClientsHeartbeat() { - if (CollectionUtils.isEmpty(jdbcRegistryClients)) { - return; + List registrations; + lifecycleLock.lock(); + try { + JdbcRegistryServerState currentState = serverState.get(); + if (currentState == JdbcRegistryServerState.STOPPED + || currentState == JdbcRegistryServerState.DISCONNECTED) { + return; + } + registrations = new ArrayList<>(clientRegistrations.values()); + } finally { + lifecycleLock.unlock(); } - JdbcRegistryServerState currentState = getServerState(); - if (currentState == JdbcRegistryServerState.STOPPED - || currentState == JdbcRegistryServerState.DISCONNECTED) { - log.warn("The JdbcRegistryServer is {}, will not refresh clients: {} heartbeat.", - currentState, - CollectionUtils.collect(jdbcRegistryClients, IJdbcRegistryClient::getJdbcRegistryClientIdentify)); + if (registrations.isEmpty()) { return; } - // Refresh the client's term + + ClientRegistration failedRegistration = null; try { - long now = System.currentTimeMillis(); - for (IJdbcRegistryClient jdbcRegistryClient : jdbcRegistryClients) { - JdbcRegistryClientHeartbeatDTO jdbcRegistryClientHeartbeatDTO = - jdbcRegistryClientDTOMap.get(jdbcRegistryClient.getJdbcRegistryClientIdentify()); - if (jdbcRegistryClientHeartbeatDTO == null) { - // This may occur when the data in db has been deleted, but the client is still alive. - log.error( - "The client {} is not found in the registry, will not refresh it's term. (This may happen when the client is removed from the db)", - jdbcRegistryClient.getJdbcRegistryClientIdentify().getClientId()); - continue; - } - JdbcRegistryClientHeartbeatDTO clone = jdbcRegistryClientHeartbeatDTO.clone(); - clone.setLastHeartbeatTime(now); - if (!jdbcRegistryClientRepository.updateById(clone)) { - log.error("The client heartbeat has expired: {}", jdbcRegistryClientHeartbeatDTO.getId()); - throw new IllegalStateException( - "The client heartbeat record no longer exists: " + jdbcRegistryClientHeartbeatDTO.getId()); + long heartbeatTime = System.currentTimeMillis(); + for (ClientRegistration registration : registrations) { + failedRegistration = registration; + registration.lock.lock(); + try { + if (!registration.active || !isHeartbeatRefreshAllowed()) { + continue; + } + JdbcRegistryClientHeartbeatDTO clone = registration.heartbeat.clone(); + clone.setLastHeartbeatTime(heartbeatTime); + if (!jdbcRegistryClientRepository.updateById(clone)) { + log.error("The client heartbeat has expired: {}", registration.heartbeat.getId()); + throw new HeartbeatUpdateException(registration); + } + if (registration.active) { + registration.heartbeat.setLastHeartbeatTime(clone.getLastHeartbeatTime()); + } + } finally { + registration.lock.unlock(); } - jdbcRegistryClientHeartbeatDTO.setLastHeartbeatTime(clone.getLastHeartbeatTime()); } - synchronized (this) { - currentState = serverState.get(); + + boolean reconnect = false; + lifecycleLock.lock(); + try { + JdbcRegistryServerState currentState = serverState.get(); if (currentState == JdbcRegistryServerState.STOPPED || currentState == JdbcRegistryServerState.DISCONNECTED) { return; @@ -381,42 +497,70 @@ private void refreshClientsHeartbeat() { log.debug("Failed to reconnect JdbcRegistryServer; current state is {}", serverState.get()); return; } - lastSuccessHeartbeat = now; - doTriggerReconnectedListener(); - } else if (currentState == JdbcRegistryServerState.STARTED) { - lastSuccessHeartbeat = now; - } else { + reconnect = true; + } else if (currentState != JdbcRegistryServerState.STARTED) { return; } + lastSuccessHeartbeat = System.currentTimeMillis(); + } finally { + lifecycleLock.unlock(); + } + if (reconnect) { + doTriggerReconnectedListener(); } log.debug("Success refresh clients: {} heartbeat.", - CollectionUtils.collect(jdbcRegistryClients, IJdbcRegistryClient::getJdbcRegistryClientIdentify)); + registrations.stream() + .map(registration -> registration.client.getJdbcRegistryClientIdentify()) + .collect(Collectors.toList())); + } catch (HeartbeatUpdateException ex) { + log.error("Failed to refresh the client's term", ex); + handleHeartbeatFailure(ex.registration); } catch (Exception ex) { log.error("Failed to refresh the client's term", ex); - long sessionTimeoutMillis = jdbcRegistryProperties.getSessionTimeout().toMillis(); - synchronized (this) { - currentState = serverState.get(); - if (currentState == JdbcRegistryServerState.STOPPED - || currentState == JdbcRegistryServerState.DISCONNECTED) { - return; - } - boolean sessionTimedOut = System.currentTimeMillis() - lastSuccessHeartbeat > sessionTimeoutMillis; - if (sessionTimedOut) { - if ((currentState == JdbcRegistryServerState.STARTED - || currentState == JdbcRegistryServerState.SUSPENDED) - && serverState.compareAndSet(currentState, JdbcRegistryServerState.DISCONNECTED)) { - doTriggerOnDisConnectedListener(); - } else { - log.debug("Failed to disconnect JdbcRegistryServer from state {}, current state is {}", - currentState, - serverState.get()); - } - } else if (currentState == JdbcRegistryServerState.STARTED - && !serverState.compareAndSet( - JdbcRegistryServerState.STARTED, JdbcRegistryServerState.SUSPENDED)) { - log.debug("Failed to suspend JdbcRegistryServer; current state is {}", serverState.get()); + handleHeartbeatFailure(failedRegistration); + } + } + + private boolean isHeartbeatRefreshAllowed() { + JdbcRegistryServerState currentState = serverState.get(); + return currentState != JdbcRegistryServerState.STOPPED + && currentState != JdbcRegistryServerState.DISCONNECTED; + } + + private void handleHeartbeatFailure(ClientRegistration failedRegistration) { + if (failedRegistration != null && !failedRegistration.active) { + return; + } + boolean disconnected = false; + long sessionTimeoutMillis = jdbcRegistryProperties.getSessionTimeout().toMillis(); + lifecycleLock.lock(); + try { + JdbcRegistryServerState currentState = serverState.get(); + if (currentState == JdbcRegistryServerState.STOPPED + || currentState == JdbcRegistryServerState.DISCONNECTED) { + return; + } + boolean sessionTimedOut = System.currentTimeMillis() - lastSuccessHeartbeat > sessionTimeoutMillis; + if (sessionTimedOut) { + if ((currentState == JdbcRegistryServerState.STARTED + || currentState == JdbcRegistryServerState.SUSPENDED) + && serverState.compareAndSet(currentState, JdbcRegistryServerState.DISCONNECTED)) { + disconnected = true; + } else { + log.debug("Failed to disconnect JdbcRegistryServer from state {}, current state is {}", + currentState, + serverState.get()); } + } else if (currentState == JdbcRegistryServerState.STARTED + && !serverState.compareAndSet( + JdbcRegistryServerState.STARTED, JdbcRegistryServerState.SUSPENDED)) { + log.debug("Failed to suspend JdbcRegistryServer; current state is {}", serverState.get()); } + } finally { + lifecycleLock.unlock(); + } + if (disconnected) { + doTriggerOnDisConnectedListener(); } } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java index b4509ff222ad..f4d516665dae 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java @@ -17,22 +17,29 @@ package org.apache.dolphinscheduler.plugin.registry.jdbc.server; +import static org.junit.jupiter.api.Assertions.assertThrows; + import org.apache.dolphinscheduler.plugin.registry.jdbc.JdbcRegistryProperties; import org.apache.dolphinscheduler.plugin.registry.jdbc.client.IJdbcRegistryClient; import org.apache.dolphinscheduler.plugin.registry.jdbc.client.JdbcRegistryClientIdentify; import org.apache.dolphinscheduler.plugin.registry.jdbc.model.DTO.JdbcRegistryClientHeartbeatDTO; +import org.apache.dolphinscheduler.plugin.registry.jdbc.model.DTO.JdbcRegistryDataDTO; import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryClientRepository; import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryDataChangeEventRepository; import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryDataRepository; import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryLockRepository; import java.time.Duration; +import java.util.Collections; +import java.util.List; import java.util.Map; 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.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; @@ -111,7 +118,7 @@ void close_shouldOnlyPurgeClientsOnceWhenCalledConcurrently() throws Exception { allowFirstPurgeToFinish.await(5, TimeUnit.SECONDS); } return null; - }).when(jdbcRegistryClientRepository).deleteByIds(Mockito.any()); + }).when(jdbcRegistryClientRepository).deleteByIdAndLastHeartbeatTime(Mockito.any(), Mockito.any()); ExecutorService closeExecutor = Executors.newFixedThreadPool(2); Future firstClose = closeExecutor.submit(jdbcRegistryServer::close); Future secondClose = null; @@ -119,9 +126,12 @@ void close_shouldOnlyPurgeClientsOnceWhenCalledConcurrently() throws Exception { try { Truth.assertThat(firstPurgeStarted.await(5, TimeUnit.SECONDS)).isTrue(); secondClose = closeExecutor.submit(jdbcRegistryServer::close); - secondClose.get(5, TimeUnit.SECONDS); - - Truth.assertThat(purgeInvocations.get()).isEqualTo(1); + try { + secondClose.get(200, TimeUnit.MILLISECONDS); + Truth.assertWithMessage("second close returned before cleanup completed").fail(); + } catch (TimeoutException expected) { + // The second close must wait for the first close to finish cleanup. + } } finally { allowFirstPurgeToFinish.countDown(); firstClose.get(5, TimeUnit.SECONDS); @@ -130,6 +140,178 @@ void close_shouldOnlyPurgeClientsOnceWhenCalledConcurrently() throws Exception { } closeExecutor.shutdownNow(); } + + Truth.assertThat(purgeInvocations.get()).isEqualTo(1); + } + + @Test + void registerClient_shouldRejectAfterClose() { + Mockito.clearInvocations(jdbcRegistryClientRepository); + jdbcRegistryServer.close(); + IJdbcRegistryClient secondClient = Mockito.mock(IJdbcRegistryClient.class); + JdbcRegistryClientIdentify secondClientIdentify = new JdbcRegistryClientIdentify(2L, "second-client"); + Mockito.when(secondClient.getJdbcRegistryClientIdentify()).thenReturn(secondClientIdentify); + + Truth.assertThat( + assertThrows( + IllegalStateException.class, + () -> jdbcRegistryServer.registerClient(secondClient))) + .hasMessageThat() + .contains("STOPPED"); + Mockito.verify(jdbcRegistryClientRepository, Mockito.never()).insert(Mockito.any()); + } + + @Test + void deregisterClient_shouldNotSuspendServerWhenHeartbeatUsesRemovedClient() throws Exception { + setServerState(JdbcRegistryServerState.STARTED); + CountDownLatch heartbeatUpdateStarted = new CountDownLatch(1); + CountDownLatch allowHeartbeatUpdateToFinish = new CountDownLatch(1); + Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenAnswer(invocation -> { + heartbeatUpdateStarted.countDown(); + allowHeartbeatUpdateToFinish.await(5, TimeUnit.SECONDS); + return false; + }); + ExecutorService heartbeatExecutor = Executors.newSingleThreadExecutor(); + Future heartbeatFuture = heartbeatExecutor.submit(() -> { + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + }); + + try { + Truth.assertThat(heartbeatUpdateStarted.await(5, TimeUnit.SECONDS)).isTrue(); + jdbcRegistryServer.deregisterClient(jdbcRegistryClient); + allowHeartbeatUpdateToFinish.countDown(); + heartbeatFuture.get(5, TimeUnit.SECONDS); + } finally { + allowHeartbeatUpdateToFinish.countDown(); + heartbeatExecutor.shutdownNow(); + } + + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.STARTED); + } + + @Test + void deregisterThenRegisterSameIdentify_shouldKeepNewRegistration() throws Exception { + setServerState(JdbcRegistryServerState.STARTED); + AtomicBoolean heartbeatRecordPresent = new AtomicBoolean(true); + CountDownLatch deleteStarted = new CountDownLatch(1); + CountDownLatch allowDeleteToFinish = new CountDownLatch(1); + Mockito.doAnswer(invocation -> { + deleteStarted.countDown(); + allowDeleteToFinish.await(5, TimeUnit.SECONDS); + heartbeatRecordPresent.set(false); + return null; + }).when(jdbcRegistryClientRepository).deleteByIdAndLastHeartbeatTime(Mockito.any(), Mockito.any()); + Mockito.doAnswer(invocation -> { + heartbeatRecordPresent.set(true); + return null; + }).when(jdbcRegistryClientRepository).insert(Mockito.any()); + Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())) + .thenAnswer(invocation -> heartbeatRecordPresent.get()); + + ExecutorService deregisterExecutor = Executors.newFixedThreadPool(2); + Future deregisterFuture = + deregisterExecutor.submit(() -> jdbcRegistryServer.deregisterClient(jdbcRegistryClient)); + Future registerFuture = null; + try { + Truth.assertThat(deleteStarted.await(5, TimeUnit.SECONDS)).isTrue(); + IJdbcRegistryClient replacementClient = Mockito.mock(IJdbcRegistryClient.class); + Mockito.when(replacementClient.getJdbcRegistryClientIdentify()).thenReturn(CLIENT_IDENTIFY); + registerFuture = deregisterExecutor.submit(() -> jdbcRegistryServer.registerClient(replacementClient)); + try { + registerFuture.get(200, TimeUnit.MILLISECONDS); + Truth.assertWithMessage("replacement registration raced with old deregistration").fail(); + } catch (TimeoutException expected) { + // Registration must wait until the previous registration is fully removed. + } + allowDeleteToFinish.countDown(); + deregisterFuture.get(5, TimeUnit.SECONDS); + registerFuture.get(5, TimeUnit.SECONDS); + + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + } finally { + allowDeleteToFinish.countDown(); + deregisterExecutor.shutdownNow(); + } + + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.STARTED); + Mockito.verify(jdbcRegistryClientRepository, Mockito.atLeast(2)).insert(Mockito.any()); + Mockito.verify(jdbcRegistryClientRepository, Mockito.atLeastOnce()).updateById(Mockito.any()); + } + + @Test + void onConnectedListener_shouldBeAbleToCloseServer() { + Mockito.doAnswer(invocation -> { + jdbcRegistryServer.close(); + return null; + }).when(connectionStateListener).onConnected(); + + jdbcRegistryServer.start(); + + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.STOPPED); + } + + @Test + void onReconnectedListener_shouldNotBlockConcurrentClose() throws Exception { + setServerState(JdbcRegistryServerState.SUSPENDED); + ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", System.currentTimeMillis()); + Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenReturn(true); + CountDownLatch callbackStarted = new CountDownLatch(1); + CountDownLatch allowCallbackToFinish = new CountDownLatch(1); + Mockito.doAnswer(invocation -> { + callbackStarted.countDown(); + allowCallbackToFinish.await(5, TimeUnit.SECONDS); + return null; + }).when(connectionStateListener).onReconnected(); + + ExecutorService executor = Executors.newFixedThreadPool(2); + Future heartbeatFuture = executor.submit( + () -> ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat")); + Future closeFuture = null; + try { + Truth.assertThat(callbackStarted.await(5, TimeUnit.SECONDS)).isTrue(); + closeFuture = executor.submit(jdbcRegistryServer::close); + try { + closeFuture.get(500, TimeUnit.MILLISECONDS); + } catch (TimeoutException ex) { + Truth.assertWithMessage("close was blocked by a connection callback").fail(); + } + allowCallbackToFinish.countDown(); + heartbeatFuture.get(5, TimeUnit.SECONDS); + closeFuture.get(5, TimeUnit.SECONDS); + } finally { + allowCallbackToFinish.countDown(); + if (closeFuture != null) { + closeFuture.get(5, TimeUnit.SECONDS); + } + heartbeatFuture.get(5, TimeUnit.SECONDS); + executor.shutdownNow(); + } + + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.STOPPED); + } + + @Test + void purgeInvalidJdbcRegistryMetadata_shouldUseConditionalMetadataCleanup() { + JdbcRegistryClientHeartbeatDTO expiredHeartbeat = JdbcRegistryClientHeartbeatDTO.builder() + .id(9L) + .clientName("expired-client") + .clientConfig(new JdbcRegistryClientHeartbeatDTO.ClientConfig(1_000L)) + .lastHeartbeatTime(0L) + .build(); + JdbcRegistryDataDTO ephemeralData = JdbcRegistryDataDTO.builder() + .id(1L) + .clientId(9L) + .dataKey("/ephemeral") + .dataValue("value") + .dataType("EPHEMERAL") + .build(); + Mockito.when(jdbcRegistryClientRepository.queryAll()).thenReturn(List.of(expiredHeartbeat)); + Mockito.when(jdbcRegistryDataRepository.selectAll()).thenReturn(List.of(ephemeralData)); + Mockito.when(jdbcRegistryLockRepository.queryAll()).thenReturn(Collections.emptyList()); + + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "purgeInvalidJdbcRegistryMetadata"); + + Mockito.verify(jdbcRegistryDataRepository, Mockito.never()).deleteByKey(ephemeralData.getDataKey()); } @Test @@ -181,17 +363,13 @@ void refreshClientsHeartbeat_shouldKeepPartialHeartbeatUpdateWhenLaterClientFail Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())) .thenAnswer(invocation -> updateInvocations.incrementAndGet() == 1); - @SuppressWarnings("unchecked") - Map heartbeatMap = - (Map) ReflectionTestUtils - .getField(jdbcRegistryServer, "jdbcRegistryClientDTOMap"); - heartbeatMap.get(CLIENT_IDENTIFY).setLastHeartbeatTime(0L); + getHeartbeat(CLIENT_IDENTIFY).setLastHeartbeatTime(0L); ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); Truth.assertThat(updateInvocations.get()).isEqualTo(2); Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.SUSPENDED); - Truth.assertThat(heartbeatMap.get(CLIENT_IDENTIFY).getLastHeartbeatTime()).isGreaterThan(0L); + Truth.assertThat(getHeartbeat(CLIENT_IDENTIFY).getLastHeartbeatTime()).isGreaterThan(0L); Mockito.verify(connectionStateListener, Mockito.never()).onReconnected(); Mockito.verify(connectionStateListener, Mockito.never()).onDisConnected(); } @@ -320,4 +498,13 @@ private void setServerState(JdbcRegistryServerState state) { ((AtomicReference) ReflectionTestUtils.getField(jdbcRegistryServer, "serverState")) .set(state); } + + @SuppressWarnings("unchecked") + private JdbcRegistryClientHeartbeatDTO getHeartbeat(JdbcRegistryClientIdentify clientIdentify) { + Map registrations = + (Map) ReflectionTestUtils + .getField(jdbcRegistryServer, "clientRegistrations"); + return (JdbcRegistryClientHeartbeatDTO) ReflectionTestUtils + .getField(registrations.get(clientIdentify), "heartbeat"); + } } From 95c141ad5c3a242a7aa801c27dc9030e8a4ba627 Mon Sep 17 00:00:00 2001 From: qiuyanjun Date: Sun, 27 Sep 2026 09:17:46 +0800 Subject: [PATCH 13/16] [Fix-18274][Docs] Remove internal JDBC registry design documents --- ...026-09-26-jdbc-registry-heartbeat-state.md | 126 ------------------ ...-09-26-jdbc-registry-concurrency-design.md | 50 ------- ...26-jdbc-registry-heartbeat-state-design.md | 35 ----- 3 files changed, 211 deletions(-) delete mode 100644 docs/superpowers/plans/2026-09-26-jdbc-registry-heartbeat-state.md delete mode 100644 docs/superpowers/specs/2026-09-26-jdbc-registry-concurrency-design.md delete mode 100644 docs/superpowers/specs/2026-09-26-jdbc-registry-heartbeat-state-design.md diff --git a/docs/superpowers/plans/2026-09-26-jdbc-registry-heartbeat-state.md b/docs/superpowers/plans/2026-09-26-jdbc-registry-heartbeat-state.md deleted file mode 100644 index d04123ae1727..000000000000 --- a/docs/superpowers/plans/2026-09-26-jdbc-registry-heartbeat-state.md +++ /dev/null @@ -1,126 +0,0 @@ -# JDBC Registry 心跳状态机修复实施计划 - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**目标：** 修复 JDBC Registry heartbeat 失效后的状态转换、关闭竞态和 CAS 失败处理，保证 `STOPPED` 终态、回调和 session timeout 语义一致。 - -**架构：** 继续使用 `AtomicReference` 作为唯一状态存储。所有状态变化使用明确的 CAS；heartbeat 调用点处理 CAS 成功和失败，`close()` 只有成功进入 `STOPPED` 的线程执行清理。 - -**技术栈：** Java、JUnit 5、Mockito、Maven、Spotless。 - -## 全局约束 - -- `STOPPED` 是关闭终态，任何未完成的 heartbeat 结果都不能覆盖它。 -- 只有成功的状态转换才触发连接回调。 -- heartbeat 记录不存在时不能 upsert 或恢复旧身份。 -- `DISCONNECTED` 只能由 session timeout 触发。 -- 不引入新的生产依赖。 - -### 任务 1：补充状态转换和竞态回归测试 - -**文件：** - -- 修改：`dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java` - -**接口：** - -- 使用现有的 `refreshClientsHeartbeat()`、`close()`、`serverState` 和 `lastSuccessHeartbeat` 测试行为，不暴露新的生产接口。 - -- [ ] **步骤 1：添加首次失败已超时就断连的测试** - -设置服务为 `STARTED`、`lastSuccessHeartbeat` 为超时之前，并让 `updateById()` 返回 `false`；断言一次 heartbeat 后状态为 `DISCONNECTED`，且断连回调只触发一次。 - -- [ ] **步骤 2：添加 heartbeat 成功结果晚于 close 的测试** - -阻塞数据库更新，先完成 `close()`，再返回成功；断言最终状态为 `STOPPED`、不触发重连或断连回调，并断言 `lastSuccessHeartbeat` 未被关闭后的结果改写。 - -- [ ] **步骤 3：添加 heartbeat 失败结果晚于 close 的测试** - -阻塞数据库更新，先完成 `close()`，再抛出异常；断言最终状态保持 `STOPPED`，不触发任何连接状态回调。 - -- [ ] **步骤 4：添加 CAS 失败的终态保护测试** - -覆盖服务已经处于 `DISCONNECTED` 或 `STOPPED` 时收到成功、失败 heartbeat 结果的情况，断言状态不变、回调不重复、不会记录虚假的成功 heartbeat。 - -- [ ] **步骤 5：运行新增测试确认当前实现失败** - -运行： - -```bash -./mvnw -pl dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc -am -DskipITs -Dtest=JdbcRegistryServerTest -Dsurefire.failIfNoSpecifiedTests=false test -``` - -预期：新增的首次超时断连或关闭后时间戳断言至少失败一项，失败原因必须来自当前状态处理逻辑，而不是测试编译错误。 - -### 任务 2：统一 heartbeat 状态转换和关闭语义 - -**文件：** - -- 修改：`dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java` - -**接口：** - -- 保持 `IJdbcRegistryServer` 和 `JdbcRegistryServerState` 不变。 -- 删除只包装单个 CAS 的 `transitionToStarted()` 和 `transitionToSuspended()`。 -- 保持 `refreshClientsHeartbeat()`、`close()` 和监听器接口签名不变。 -- [ ] **步骤 1：将 close 改为 CAS 终态转换** - -在现有 `synchronized (this)` 生命周期临界区内循环读取状态并执行 `compareAndSet(current, STOPPED)`；读取到 `STOPPED` 时直接返回。只有 CAS 成功的调用继续关闭调度器、清理数据库和清空本地集合。 - -- [ ] **步骤 2：将 start 的状态写入改为明确的 INIT 到 STARTED 转换** - -保留启动方法的生命周期锁，在初始化完成后使用 `compareAndSet(INIT, STARTED)`，失败时不触发 `onConnected()`。 - -- [ ] **步骤 3：在 heartbeat 成功路径直接处理 CAS 结果** - -成功更新数据库后，仅当服务仍处于 `STARTED` 或成功完成 `SUSPENDED -> STARTED` 时更新 `lastSuccessHeartbeat` 和成功日志；CAS 失败且当前状态为 `STOPPED` 或 `DISCONNECTED` 时结束本次处理。 - -- [ ] **步骤 4：在 heartbeat 失败路径直接处理允许的转换** - -先计算距离上次成功 heartbeat 的时间；超时后依次尝试 `STARTED -> DISCONNECTED` 和 `SUSPENDED -> DISCONNECTED`，只有一次 CAS 成功才触发断连回调。未超时时只尝试 `STARTED -> SUSPENDED`，CAS 失败时根据当前终态结束本次事件。 - -- [ ] **步骤 5：明确时间戳的跨线程可见性** - -将 `lastSuccessHeartbeat` 改为 `volatile long`，保持构造函数初始化并避免空值自动拆箱。 - -- [ ] **步骤 6：停止 DISCONNECTED 状态下的清理调度** - -让 `purgeInvalidJdbcRegistryMetadata()` 在 `DISCONNECTED` 和 `STOPPED` 状态都直接返回，避免失去数据库 session 的服务继续清理其他客户端元数据。 - -- [ ] **步骤 7：运行任务 1 的测试确认通过** - -运行同一 Maven 测试命令，预期 `JdbcRegistryServerTest` 全部通过且无失败、错误。 - -### 任务 3：补充多客户端和格式验证 - -**文件：** - -- 修改：`dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java` - -- [ ] **步骤 1：添加多客户端部分更新失败测试** - -注册两个客户端，让第一个更新成功、第二个更新返回 `false`；断言第一个本地 DTO 时间戳已更新，服务按当前 timeout 规则进入 `SUSPENDED` 或 `DISCONNECTED`，且不会触发重连回调。 - -- [ ] **步骤 2：运行完整目标测试** - -运行： - -```bash -./mvnw clean -pl dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc -am -DskipITs -Dtest=JdbcRegistryServerTest,JdbcRegistryDataChangeListenerAdapterTest -Dsurefire.failIfNoSpecifiedTests=false test -``` - -预期：选定测试全部通过，Maven reactor 构建成功。 - -- [ ] **步骤 3：运行格式检查** - -运行： - -```bash -./mvnw -pl dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc spotless:check -``` - -预期：Spotless 检查成功。 - -- [ ] **步骤 4：检查差异和工作区** - -运行 `git diff --check`、`git status --short` 和目标文件差异审查，确认只包含本次状态机修复、测试和设计文档，不包含工作区原有未跟踪文件。 diff --git a/docs/superpowers/specs/2026-09-26-jdbc-registry-concurrency-design.md b/docs/superpowers/specs/2026-09-26-jdbc-registry-concurrency-design.md deleted file mode 100644 index ff7de8b57c3a..000000000000 --- a/docs/superpowers/specs/2026-09-26-jdbc-registry-concurrency-design.md +++ /dev/null @@ -1,50 +0,0 @@ -# JDBC 注册中心并发安全设计 - -## 目标 - -修复 JDBC 注册中心在心跳、注册、注销、关闭和过期清理并发执行时的状态与数据库一致性问题，确保： - -- `STOPPED` 是关闭后的终态，心跳结果不能覆盖它。 -- 注册、注销和关闭不会留下孤立的数据库心跳记录。 -- 注销或关闭不会被旧心跳更新反向覆盖。 -- 过期清理不会删除刚刚成功续期的客户端及其临时数据、锁。 -- 连接状态监听器不会在服务器内部锁内执行。 -- 重复关闭具有幂等性，并且调用方不会在清理尚未完成时误以为关闭已完成。 - -## 并发模型 - -服务器使用生命周期锁保护状态和客户端注册表。注册表以客户端标识映射到不可替代的注册对象，注册对象包含客户端实例、心跳 DTO、活动标志和客户端级锁。 - -生命周期锁只保护本地状态和注册表变更，不包住监听器回调。每个客户端的数据库心跳更新、注销删除和关闭删除使用该客户端级锁串行化。这样可以让关闭先冻结注册表，再等待正在进行的客户端操作完成。 - -心跳任务先获取注册表快照，再逐个获取客户端级锁。客户端被注销或关闭时先从注册表移除并标记为非活动；已经取得快照的心跳在获取客户端锁后会再次检查活动标志，因此不会更新已注销客户端。 - -## 状态转换和回调 - -状态转换在生命周期锁内完成并验证源状态。只有转换成功的线程可以生成回调事件。回调在释放生命周期锁之后执行，避免回调阻塞关闭、产生锁循环或重入关闭后继续向已关闭的调度器提交任务。 - -允许在服务器启动前注册客户端，以保持现有启动顺序；服务器进入 `STOPPED` 或 `DISCONNECTED` 后拒绝新注册。注销只处理当前注册对象，旧客户端实例不能删除同标识的新注册对象。 - -## 过期清理 - -清理任务使用查询到的客户端标识和心跳时间执行条件删除，只有 `id` 和 `last_heartbeat_time` 同时仍匹配时才删除。心跳已经更新的记录不会被条件删除。 - -后续临时数据和锁清理只针对确认已经删除的客户端，并在清理前重新读取当前心跳记录，避免使用过期查询结果删除新注册客户端的数据。 - -## 关闭流程 - -关闭过程先在生命周期锁内将状态转换为 `STOPPED`、标记并冻结所有注册对象，然后关闭调度器。随后逐个等待客户端级操作完成并删除对应心跳记录，最后完成关闭信号。并发或重复调用 `close()` 复用同一个关闭完成信号。 - -## 测试范围 - -新增并发回归测试覆盖： - -- 注册与关闭并发； -- 注销与心跳更新并发； -- 注销后立即重新注册； -- 过期清理与心跳更新并发； -- 回调重入关闭； -- 重复关闭等待清理完成； -- 关闭与成功、失败心跳竞态。 - -每个测试先在现有实现上验证失败，再实现对应修复。 diff --git a/docs/superpowers/specs/2026-09-26-jdbc-registry-heartbeat-state-design.md b/docs/superpowers/specs/2026-09-26-jdbc-registry-heartbeat-state-design.md deleted file mode 100644 index eee330b32344..000000000000 --- a/docs/superpowers/specs/2026-09-26-jdbc-registry-heartbeat-state-design.md +++ /dev/null @@ -1,35 +0,0 @@ -# JDBC Registry 心跳状态机修复设计 - -## 背景 - -JDBC Registry 客户端的 heartbeat 记录可能在数据库不可用超过 session timeout 后被其他服务清理。数据库恢复后，原服务继续尝试更新已经不存在的记录；如果忽略 `updateById` 的 0 行结果，服务会继续运行并保留过期身份。 - -本次修复需要同时处理 heartbeat 失效、服务关闭竞态和状态转换的 CAS 失败，避免服务在关闭后被恢复为工作状态，也避免回调与实际状态不一致。 - -## 状态约束 - -- `INIT` 只能转换为 `STARTED`。 -- heartbeat 失败时，`STARTED` 进入 `SUSPENDED`；如果从上次成功 heartbeat 起已经超过 session timeout，则直接进入 `DISCONNECTED`。 -- `SUSPENDED` 在 heartbeat 成功后进入 `STARTED`，并只在 CAS 成功时触发重连回调。 -- `SUSPENDED` 或 `STARTED` 在 session timeout 后进入 `DISCONNECTED`，并只在 CAS 成功时触发断连回调。 -- `STOPPED` 是关闭终态，任何未完成的 heartbeat 结果都不能覆盖它。 -- `close()` 只有成功将当前状态转换为 `STOPPED` 的线程执行调度器关闭、数据库清理和本地集合清理。 - -## 实现方案 - -使用现有的 `AtomicReference` 作为唯一状态存储和可见性边界。`close()` 使用 CAS 循环处理重复调用和并发状态变化；heartbeat 的成功、失败和断连路径在调用点直接处理 CAS 结果，删除只包装单个 CAS 的转换方法。断连路径只尝试允许的源状态，CAS 失败时根据当前状态结束本次事件，不通过无限重试把过期事件应用到新的状态上。 - -heartbeat 成功后的本地时间戳和成功日志只在当前服务仍可工作时更新。heartbeat 失败首先区分瞬时失败和已经超过 session timeout 的失败；记录不存在属于更新失败，不能通过 upsert 恢复旧身份。 - -## 测试方案 - -增加或调整以下回归覆盖： - -- heartbeat 记录被清理后的 `STARTED -> SUSPENDED -> DISCONNECTED` 路径。 -- 首次失败时已经超过 session timeout 的直接断连路径。 -- `close()` 与失败、成功 heartbeat 结果的双向竞态，确保 `STOPPED` 最终状态不被覆盖且不会发出错误回调。 -- 每个 CAS 失败分支都不会修改终态、重复触发回调或记录虚假的成功 heartbeat。 -- 多客户端刷新时前一个客户端成功、后一个客户端失败的状态和时间戳行为。 -- 已断连服务不再刷新 heartbeat。 - -验证使用 JDBC registry 模块的目标单元测试、Spotless 检查和必要的编译检查。 From 0cb0955a42801a5c6d2611b50b1047e1227ce272 Mon Sep 17 00:00:00 2001 From: qiuyanjun Date: Sun, 27 Sep 2026 10:00:12 +0800 Subject: [PATCH 14/16] [Fix-18274][Registry] Revert out-of-scope concurrency changes Revert 4ef9952952255bd7a17ce1ffabccf78c3db2e306 to keep this PR focused on expired JDBC heartbeat sessions and their state transitions. --- .../JdbcRegistryClientHeartbeatMapper.java | 7 - .../jdbc/mapper/JdbcRegistryDataMapper.java | 15 +- .../jdbc/mapper/JdbcRegistryLockMapper.java | 8 - .../JdbcRegistryClientRepository.java | 4 - .../JdbcRegistryDataRepository.java | 16 +- .../JdbcRegistryLockRepository.java | 4 - .../jdbc/server/IJdbcRegistryDataManager.java | 7 - .../jdbc/server/JdbcRegistryDataManager.java | 195 +++------ .../jdbc/server/JdbcRegistryLockManager.java | 110 ++--- .../jdbc/server/JdbcRegistryServer.java | 412 ++++++------------ .../jdbc/server/JdbcRegistryServerTest.java | 207 +-------- 11 files changed, 250 insertions(+), 735 deletions(-) diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryClientHeartbeatMapper.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryClientHeartbeatMapper.java index d126c1567521..2b8499bb4812 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryClientHeartbeatMapper.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryClientHeartbeatMapper.java @@ -19,8 +19,6 @@ import org.apache.dolphinscheduler.plugin.registry.jdbc.model.DO.JdbcRegistryClientHeartbeat; -import org.apache.ibatis.annotations.Delete; -import org.apache.ibatis.annotations.Param; import org.apache.ibatis.annotations.Select; import java.util.List; @@ -32,9 +30,4 @@ public interface JdbcRegistryClientHeartbeatMapper extends BaseMapper selectAll(); - @Delete("delete from t_ds_jdbc_registry_client_heartbeat " - + "where id = #{id} and last_heartbeat_time = #{lastHeartbeatTime}") - int deleteByIdAndLastHeartbeatTime(@Param("id") Long id, - @Param("lastHeartbeatTime") Long lastHeartbeatTime); - } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryDataMapper.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryDataMapper.java index b382ba513686..261bfae4fe95 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryDataMapper.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryDataMapper.java @@ -36,10 +36,7 @@ public interface JdbcRegistryDataMapper extends BaseMapper { JdbcRegistryData selectByKey(@Param("key") String key); @Delete("delete from t_ds_jdbc_registry_data where data_key = #{key}") - int deleteByKey(@Param("key") String key); - - @Delete("delete from t_ds_jdbc_registry_data where data_key = #{key} and id = #{id}") - int deleteByKeyAndId(@Param("key") String key, @Param("id") Long id); + void deleteByKey(@Param("key") String key); @Delete({""}) void deleteByClientIds(@Param("clientIds") List clientIds, @Param("dataType") String dataType); - @Delete("delete from t_ds_jdbc_registry_data " - + "where data_key = #{dataKey} " - + "and client_id = #{clientId} " - + "and data_type = 'EPHEMERAL' " - + "and not exists (" - + "select 1 from t_ds_jdbc_registry_client_heartbeat h " - + "where h.id = t_ds_jdbc_registry_data.client_id)") - int deleteEphemeralByKeyAndInactiveClient(@Param("dataKey") String dataKey, - @Param("clientId") Long clientId); - } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryLockMapper.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryLockMapper.java index 5dbe13007778..0639dfbc204d 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryLockMapper.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryLockMapper.java @@ -36,12 +36,4 @@ public interface JdbcRegistryLockMapper extends BaseMapper { "", ""}) void deleteByClientIds(@Param("clientIds") List clientIds); - - @Delete("delete from t_ds_jdbc_registry_lock " - + "where id = #{lockId} " - + "and client_id = #{clientId} " - + "and not exists (" - + "select 1 from t_ds_jdbc_registry_client_heartbeat h " - + "where h.id = t_ds_jdbc_registry_lock.client_id)") - int deleteByIdAndInactiveClient(@Param("lockId") Long lockId, @Param("clientId") Long clientId); } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryClientRepository.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryClientRepository.java index 1f1e8984f02d..1791f3c942aa 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryClientRepository.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryClientRepository.java @@ -52,10 +52,6 @@ public void deleteByIds(Collection clientIds) { jdbcRegistryClientHeartbeatMapper.deleteBatchIds(clientIds); } - public boolean deleteByIdAndLastHeartbeatTime(Long clientId, Long lastHeartbeatTime) { - return jdbcRegistryClientHeartbeatMapper.deleteByIdAndLastHeartbeatTime(clientId, lastHeartbeatTime) == 1; - } - public boolean updateById(JdbcRegistryClientHeartbeatDTO jdbcRegistryClientHeartbeatDTO) { JdbcRegistryClientHeartbeat jdbcRegistryClientHeartbeat = JdbcRegistryClientHeartbeatDTO.toJdbcRegistryClientHeartbeat(jdbcRegistryClientHeartbeatDTO); diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryDataRepository.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryDataRepository.java index 373204439463..e91ee0a90eb8 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryDataRepository.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryDataRepository.java @@ -47,16 +47,8 @@ public Optional selectByKey(String key) { .map(JdbcRegistryDataDTO::fromJdbcRegistryData); } - public boolean deleteByKey(String key) { - return jdbcRegistryDataMapper.deleteByKey(key) == 1; - } - - public boolean deleteByKeyAndId(String key, Long id) { - return jdbcRegistryDataMapper.deleteByKeyAndId(key, id) == 1; - } - - public boolean deleteEphemeralByKeyAndInactiveClient(String dataKey, Long clientId) { - return jdbcRegistryDataMapper.deleteEphemeralByKeyAndInactiveClient(dataKey, clientId) == 1; + public void deleteByKey(String key) { + jdbcRegistryDataMapper.deleteByKey(key); } public void insert(JdbcRegistryDataDTO jdbcRegistryData) { @@ -65,7 +57,7 @@ public void insert(JdbcRegistryDataDTO jdbcRegistryData) { jdbcRegistryData.setId(jdbcRegistryDataDO.getId()); } - public boolean updateById(JdbcRegistryDataDTO jdbcRegistryDataDTO) { - return jdbcRegistryDataMapper.updateById(JdbcRegistryDataDTO.toJdbcRegistryData(jdbcRegistryDataDTO)) == 1; + public void updateById(JdbcRegistryDataDTO jdbcRegistryDataDTO) { + jdbcRegistryDataMapper.updateById(JdbcRegistryDataDTO.toJdbcRegistryData(jdbcRegistryDataDTO)); } } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryLockRepository.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryLockRepository.java index b45fffd64df0..63d7f7f5bee4 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryLockRepository.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryLockRepository.java @@ -52,8 +52,4 @@ public void insert(JdbcRegistryLockDTO jdbcRegistryLock) { public void deleteById(Long id) { jdbcRegistryLockMapper.deleteById(id); } - - public boolean deleteByIdAndInactiveClient(Long lockId, Long clientId) { - return jdbcRegistryLockMapper.deleteByIdAndInactiveClient(lockId, clientId) == 1; - } } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/IJdbcRegistryDataManager.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/IJdbcRegistryDataManager.java index e2ef19790026..86db4de6abf7 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/IJdbcRegistryDataManager.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/IJdbcRegistryDataManager.java @@ -57,11 +57,4 @@ public interface IJdbcRegistryDataManager { * Delete the {@link JdbcRegistryDataDTO} by key. */ void deleteJdbcRegistryDataByKey(String key); - - /** - * Delete an ephemeral row only when its client heartbeat no longer exists. - * - * @return {@code true} when the row was actually deleted - */ - boolean deleteEphemeralDataIfClientInactive(JdbcRegistryDataDTO data); } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryDataManager.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryDataManager.java index 65f3a4b53c22..b49ebc6b2f2a 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryDataManager.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryDataManager.java @@ -34,11 +34,9 @@ import java.util.Date; import java.util.List; import java.util.Optional; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; -import java.util.concurrent.locks.ReentrantLock; import java.util.stream.Collectors; import lombok.extern.slf4j.Slf4j; @@ -67,12 +65,6 @@ public class JdbcRegistryDataManager private final List> registryRowChangeListeners; - /** - * Operations for the same registry key must be serialized locally. The database unique key still protects - * multiple registry server instances, while this lock prevents stale read/modify/delete races in one instance. - */ - private final ConcurrentHashMap dataKeyLocks = new ConcurrentHashMap<>(); - private long lastDetectedJdbcRegistryDataChangeEventId = -1; public JdbcRegistryDataManager(JdbcRegistryProperties registryProperties, @@ -180,116 +172,73 @@ public void putJdbcRegistryData(Long clientId, String key, String value, DataTyp checkNotNull(key); checkNotNull(dataType); - ReentrantLock keyLock = dataKeyLocks.computeIfAbsent(key, ignored -> new ReentrantLock()); - keyLock.lock(); - try { - jdbcRegistryTransactionTemplate.execute(status -> { - Optional jdbcRegistryDataOptional = jdbcRegistryDataRepository.selectByKey(key); - if (jdbcRegistryDataOptional.isPresent()) { - JdbcRegistryDataDTO jdbcRegistryData = jdbcRegistryDataOptional.get(); - if (!dataType.name().equals(jdbcRegistryData.getDataType())) { - throw new UnsupportedOperationException("The data type: " + jdbcRegistryData.getDataType() - + " of the key: " + key + " cannot be updated"); - } + final Optional jdbcRegistryDataOptional = jdbcRegistryDataRepository.selectByKey(key); + + jdbcRegistryTransactionTemplate.execute(status -> { + if (jdbcRegistryDataOptional.isPresent()) { + JdbcRegistryDataDTO jdbcRegistryData = jdbcRegistryDataOptional.get(); + if (!dataType.name().equals(jdbcRegistryData.getDataType())) { + throw new UnsupportedOperationException("The data type: " + jdbcRegistryData.getDataType() + + " of the key: " + key + " cannot be updated"); + } - if (DataType.EPHEMERAL.name().equals(jdbcRegistryData.getDataType()) - && !jdbcRegistryData.getClientId().equals(clientId)) { + if (DataType.EPHEMERAL.name().equals(jdbcRegistryData.getDataType())) { + if (!jdbcRegistryData.getClientId().equals(clientId)) { throw new UnsupportedOperationException( "The EPHEMERAL data: " + key + " can only be updated by its owner: " + jdbcRegistryData.getClientId() + " but not: " + clientId); } - - jdbcRegistryData.setDataValue(value); - jdbcRegistryData.setLastUpdateTime(new Date()); - if (!jdbcRegistryDataRepository.updateById(jdbcRegistryData)) { - throw new IllegalStateException("The registry data was concurrently removed: " + key); - } - - JdbcRegistryDataChangeEventDTO jdbcRegistryDataChangeEvent = - JdbcRegistryDataChangeEventDTO.builder() - .jdbcRegistryData(jdbcRegistryData) - .eventType(JdbcRegistryDataChangeEventDTO.EventType.UPDATE) - .createTime(new Date()) - .build(); - jdbcRegistryDataChangeEventRepository.insert(jdbcRegistryDataChangeEvent); - } else { - JdbcRegistryDataDTO jdbcRegistryDataDTO = JdbcRegistryDataDTO.builder() - .clientId(clientId) - .dataKey(key) - .dataValue(value) - .dataType(dataType.name()) - .createTime(new Date()) - .lastUpdateTime(new Date()) - .build(); - jdbcRegistryDataRepository.insert(jdbcRegistryDataDTO); - JdbcRegistryDataChangeEventDTO registryDataChangeEvent = - JdbcRegistryDataChangeEventDTO.builder() - .jdbcRegistryData(jdbcRegistryDataDTO) - .eventType(JdbcRegistryDataChangeEventDTO.EventType.ADD) - .createTime(new Date()) - .build(); - jdbcRegistryDataChangeEventRepository.insert(registryDataChangeEvent); } - return null; - }); - } finally { - keyLock.unlock(); - } + + jdbcRegistryData.setDataValue(value); + jdbcRegistryData.setLastUpdateTime(new Date()); + jdbcRegistryDataRepository.updateById(jdbcRegistryData); + + JdbcRegistryDataChangeEventDTO jdbcRegistryDataChangeEvent = JdbcRegistryDataChangeEventDTO.builder() + .jdbcRegistryData(jdbcRegistryData) + .eventType(JdbcRegistryDataChangeEventDTO.EventType.UPDATE) + .createTime(new Date()) + .build(); + jdbcRegistryDataChangeEventRepository.insert(jdbcRegistryDataChangeEvent); + } else { + JdbcRegistryDataDTO jdbcRegistryDataDTO = JdbcRegistryDataDTO.builder() + .clientId(clientId) + .dataKey(key) + .dataValue(value) + .dataType(dataType.name()) + .createTime(new Date()) + .lastUpdateTime(new Date()) + .build(); + jdbcRegistryDataRepository.insert(jdbcRegistryDataDTO); + JdbcRegistryDataChangeEventDTO registryDataChangeEvent = JdbcRegistryDataChangeEventDTO.builder() + .jdbcRegistryData(jdbcRegistryDataDTO) + .eventType(JdbcRegistryDataChangeEventDTO.EventType.ADD) + .createTime(new Date()) + .build(); + jdbcRegistryDataChangeEventRepository.insert(registryDataChangeEvent); + } + return null; + }); } @Override public void deleteJdbcRegistryDataByKey(String key) { checkNotNull(key); - ReentrantLock keyLock = dataKeyLocks.computeIfAbsent(key, ignored -> new ReentrantLock()); - keyLock.lock(); - try { - Optional jdbcRegistryDataOptional = jdbcRegistryDataRepository.selectByKey(key); - if (!jdbcRegistryDataOptional.isPresent()) { - return; - } - jdbcRegistryTransactionTemplate.execute(status -> { - if (!jdbcRegistryDataRepository.deleteByKeyAndId(key, jdbcRegistryDataOptional.get().getId())) { - return null; - } - final JdbcRegistryDataChangeEventDTO registryDataChangeEvent = - JdbcRegistryDataChangeEventDTO.builder() - .jdbcRegistryData(jdbcRegistryDataOptional.get()) - .eventType(JdbcRegistryDataChangeEventDTO.EventType.DELETE) - .createTime(new Date()) - .build(); - jdbcRegistryDataChangeEventRepository.insert(registryDataChangeEvent); - return null; - }); - } finally { - keyLock.unlock(); - } - } - - @Override - public boolean deleteEphemeralDataIfClientInactive(JdbcRegistryDataDTO data) { - checkNotNull(data); - ReentrantLock keyLock = dataKeyLocks.computeIfAbsent(data.getDataKey(), ignored -> new ReentrantLock()); - keyLock.lock(); - try { - Boolean deleted = jdbcRegistryTransactionTemplate.execute(status -> { - if (!jdbcRegistryDataRepository.deleteEphemeralByKeyAndInactiveClient( - data.getDataKey(), data.getClientId())) { - return false; - } - final JdbcRegistryDataChangeEventDTO registryDataChangeEvent = - JdbcRegistryDataChangeEventDTO.builder() - .jdbcRegistryData(data) - .eventType(JdbcRegistryDataChangeEventDTO.EventType.DELETE) - .createTime(new Date()) - .build(); - jdbcRegistryDataChangeEventRepository.insert(registryDataChangeEvent); - return true; - }); - return Boolean.TRUE.equals(deleted); - } finally { - keyLock.unlock(); + Optional jdbcRegistryDataOptional = jdbcRegistryDataRepository.selectByKey(key); + if (!jdbcRegistryDataOptional.isPresent()) { + return; } + jdbcRegistryTransactionTemplate.execute(status -> { + jdbcRegistryDataRepository.deleteByKey(key); + final JdbcRegistryDataChangeEventDTO registryDataChangeEvent = JdbcRegistryDataChangeEventDTO.builder() + .jdbcRegistryData(jdbcRegistryDataOptional.get()) + .eventType(JdbcRegistryDataChangeEventDTO.EventType.DELETE) + .createTime(new Date()) + .build(); + jdbcRegistryDataChangeEventRepository.insert(registryDataChangeEvent); + return null; + }); } private void doTriggerJdbcRegistryDataAddedListener(List valuesToAdd) { @@ -298,13 +247,11 @@ private void doTriggerJdbcRegistryDataAddedListener(List va } log.debug("Trigger:onJdbcRegistryDataAdded: {}", valuesToAdd); valuesToAdd.forEach(jdbcRegistryData -> { - registryRowChangeListeners.forEach(listener -> { - try { - listener.onRegistryRowAdded(jdbcRegistryData); - } catch (Exception ex) { - log.error("Trigger:onRegistryRowAdded: {} failed", jdbcRegistryData, ex); - } - }); + try { + registryRowChangeListeners.forEach(listener -> listener.onRegistryRowAdded(jdbcRegistryData)); + } catch (Exception ex) { + log.error("Trigger:onRegistryRowAdded: {} failed", jdbcRegistryData, ex); + } }); } @@ -314,13 +261,11 @@ private void doTriggerJdbcRegistryDataRemovedListener(List } log.debug("Trigger:onJdbcRegistryDataDeleted: {}", valuesToRemoved); valuesToRemoved.forEach(jdbcRegistryData -> { - registryRowChangeListeners.forEach(listener -> { - try { - listener.onRegistryRowDeleted(jdbcRegistryData); - } catch (Exception ex) { - log.error("Trigger:onRegistryRowDeleted: {} failed", jdbcRegistryData, ex); - } - }); + try { + registryRowChangeListeners.forEach(listener -> listener.onRegistryRowDeleted(jdbcRegistryData)); + } catch (Exception ex) { + log.error("Trigger:onRegistryRowAdded: {} failed", jdbcRegistryData, ex); + } }); } @@ -330,13 +275,11 @@ private void doTriggerJdbcRegistryDataUpdatedListener(List } log.debug("Trigger:onJdbcRegistryDataUpdated: {}", valuesToUpdated); valuesToUpdated.forEach(jdbcRegistryData -> { - registryRowChangeListeners.forEach(listener -> { - try { - listener.onRegistryRowUpdated(jdbcRegistryData); - } catch (Exception ex) { - log.error("Trigger:onRegistryRowUpdated: {} failed", jdbcRegistryData, ex); - } - }); + try { + registryRowChangeListeners.forEach(listener -> listener.onRegistryRowUpdated(jdbcRegistryData)); + } catch (Exception ex) { + log.error("Trigger:onRegistryRowAdded: {} failed", jdbcRegistryData, ex); + } }); } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryLockManager.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryLockManager.java index aae62109ddab..68ba187778b0 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryLockManager.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryLockManager.java @@ -45,8 +45,6 @@ public class JdbcRegistryLockManager implements IJdbcRegistryLockManager { // lockKey -> LockEntry private final Map jdbcRegistryLockHolderMap = new ConcurrentHashMap<>(); - private final Object lockHolderMonitor = new Object(); - public JdbcRegistryLockManager(JdbcRegistryProperties jdbcRegistryProperties, JdbcRegistryLockRepository jdbcRegistryLockRepository) { this.jdbcRegistryProperties = jdbcRegistryProperties; @@ -57,7 +55,7 @@ public JdbcRegistryLockManager(JdbcRegistryProperties jdbcRegistryProperties, public void acquireJdbcRegistryLock(Long clientId, String lockKey) { String lockOwner = LockUtils.getLockOwner(); while (true) { - if (tryReenterLock(clientId, lockKey, lockOwner)) { + if (tryReenterLock(lockKey, lockOwner)) { return; } JdbcRegistryLockDTO jdbcRegistryLock = JdbcRegistryLockDTO.builder() @@ -67,39 +65,31 @@ public void acquireJdbcRegistryLock(Long clientId, String lockKey) { .createTime(new Date()) .build(); try { - synchronized (lockHolderMonitor) { - jdbcRegistryLockRepository.insert(jdbcRegistryLock); + jdbcRegistryLockRepository.insert(jdbcRegistryLock); + if (jdbcRegistryLock != null) { jdbcRegistryLockHolderMap.put(lockKey, LockEntry.builder() .lockKey(lockKey) .lockOwner(lockOwner) .jdbcRegistryLock(jdbcRegistryLock) .build()); + return; } log.debug("{} acquire the lock {} success", lockOwner, lockKey); - return; } catch (DuplicateKeyException duplicateKeyException) { // The lock is already exist, wait it release. - log.debug("{} failed to acquire the lock {}, it is held by another owner", lockOwner, lockKey); + continue; } log.debug("{} acquire the lock {} failed try again", lockOwner, lockKey); // acquire failed, wait and try again - if (!sleepBeforeRetry(jdbcRegistryProperties.getHeartbeatRefreshInterval().toMillis())) { - throw new IllegalStateException("Interrupted while acquiring the lock: " + lockKey); - } + ThreadUtils.sleep(jdbcRegistryProperties.getHeartbeatRefreshInterval().toMillis()); } } - private boolean tryReenterLock(Long clientId, String lockKey, String lockAcquirer) { - synchronized (lockHolderMonitor) { - LockEntry lockEntry = jdbcRegistryLockHolderMap.get(lockKey); - if (lockEntry != null && lockAcquirer.equals(lockEntry.getLockOwner())) { - if (!clientId.equals(lockEntry.getJdbcRegistryLock().getClientId())) { - throw new UnsupportedOperationException( - "The client " + clientId + " is not the lock owner of the lock: " + lockKey); - } - lockEntry.lockCount.incrementAndGet(); - return true; - } + private boolean tryReenterLock(String lockKey, String lockAcquirer) { + LockEntry lockEntry = jdbcRegistryLockHolderMap.get(lockKey); + if (lockEntry != null && lockAcquirer.equals(lockEntry.getLockOwner())) { + lockEntry.lockCount.incrementAndGet(); + return true; } return false; } @@ -109,7 +99,7 @@ public boolean acquireJdbcRegistryLock(Long clientId, String lockKey, long timeo String lockOwner = LockUtils.getLockOwner(); long start = System.currentTimeMillis(); while (System.currentTimeMillis() - start <= timeout) { - if (tryReenterLock(clientId, lockKey, lockOwner)) { + if (tryReenterLock(lockKey, lockOwner)) { return true; } JdbcRegistryLockDTO jdbcRegistryLock = JdbcRegistryLockDTO.builder() @@ -119,83 +109,47 @@ public boolean acquireJdbcRegistryLock(Long clientId, String lockKey, long timeo .createTime(new Date()) .build(); try { - synchronized (lockHolderMonitor) { - jdbcRegistryLockRepository.insert(jdbcRegistryLock); + jdbcRegistryLockRepository.insert(jdbcRegistryLock); + if (jdbcRegistryLock != null) { jdbcRegistryLockHolderMap.put(lockKey, LockEntry.builder() .lockKey(lockKey) .lockOwner(lockOwner) .jdbcRegistryLock(jdbcRegistryLock) .build()); + return true; } log.debug("{} acquire the lock {} success", lockOwner, lockKey); - return true; } catch (DuplicateKeyException duplicateKeyException) { // The lock is already exist, wait it release. - log.debug("{} failed to acquire the lock {}, it is held by another owner", lockOwner, lockKey); + continue; } log.debug("{} acquire the lock {} failed try again", lockOwner, lockKey); // acquire failed, wait and try again - long remaining = timeout - (System.currentTimeMillis() - start); - if (remaining <= 0 || !sleepBeforeRetry(Math.min( - remaining, jdbcRegistryProperties.getHeartbeatRefreshInterval().toMillis()))) { - return false; - } + ThreadUtils.sleep(jdbcRegistryProperties.getHeartbeatRefreshInterval().toMillis()); } return false; } - private boolean sleepBeforeRetry(long millis) { - if (millis <= 0 || Thread.currentThread().isInterrupted()) { - return false; - } - ThreadUtils.sleep(millis); - return !Thread.currentThread().isInterrupted(); - } - @Override public void releaseJdbcRegistryLock(Long clientId, String lockKey) { String lockOwner = LockUtils.getLockOwner(); - LockEntry lockEntry; - synchronized (lockHolderMonitor) { - lockEntry = jdbcRegistryLockHolderMap.get(lockKey); - if (lockEntry == null || !lockOwner.equals(lockEntry.getLockOwner())) { - return; - } - if (!clientId.equals(lockEntry.getJdbcRegistryLock().getClientId())) { - throw new UnsupportedOperationException( - "The client " + clientId + " is not the lock owner of the lock: " + lockKey); - } - int newLockCount = lockEntry.lockCount.decrementAndGet(); - if (newLockCount > 0) { - return; - } - if (newLockCount < 0) { - lockEntry.lockCount.incrementAndGet(); - throw new IllegalMonitorStateException("Jdbc lock count has gone negative for lock: " + lockKey); - } - // Remove before deleting from the database so a replacement entry cannot be removed by this release. - jdbcRegistryLockHolderMap.remove(lockKey, lockEntry); + LockEntry lockEntry = jdbcRegistryLockHolderMap.get(lockKey); + if (lockEntry == null || !lockOwner.equals(lockEntry.getLockOwner())) { + return; } - jdbcRegistryLockRepository.deleteById(lockEntry.getJdbcRegistryLock().getId()); - } - - /** - * Delete an inactive lock while serializing it with local acquire/release operations. - */ - boolean deleteIfInactive(JdbcRegistryLockDTO jdbcRegistryLock) { - synchronized (lockHolderMonitor) { - boolean deleted = jdbcRegistryLockRepository.deleteByIdAndInactiveClient( - jdbcRegistryLock.getId(), jdbcRegistryLock.getClientId()); - if (!deleted) { - return false; - } - LockEntry lockEntry = jdbcRegistryLockHolderMap.get(jdbcRegistryLock.getLockKey()); - if (lockEntry != null - && jdbcRegistryLock.getId().equals(lockEntry.getJdbcRegistryLock().getId())) { - jdbcRegistryLockHolderMap.remove(jdbcRegistryLock.getLockKey(), lockEntry); - } - return true; + if (!clientId.equals(lockEntry.getJdbcRegistryLock().getClientId())) { + throw new UnsupportedOperationException( + "The client " + clientId + " is not the lock owner of the lock: " + lockKey); } + int newLockCount = lockEntry.lockCount.decrementAndGet(); + if (newLockCount > 0) { + return; + } + if (newLockCount < 0) { + throw new IllegalMonitorStateException("Jdbc lock count has gone negative for lock: " + lockKey); + } + jdbcRegistryLockRepository.deleteById(lockEntry.getJdbcRegistryLock().getId()); + jdbcRegistryLockHolderMap.remove(lockKey); } @Data diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java index 55231f9cab27..71ddaf3810d5 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java @@ -32,22 +32,20 @@ import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryLockRepository; import org.apache.dolphinscheduler.registry.api.RegistryException; +import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.lang3.time.StopWatch; -import java.util.ArrayList; import java.util.Collection; import java.util.Date; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; -import java.util.concurrent.locks.ReentrantLock; import java.util.stream.Collectors; import lombok.SneakyThrows; @@ -76,41 +74,17 @@ public class JdbcRegistryServer implements IJdbcRegistryServer { private final AtomicReference serverState = new AtomicReference<>(JdbcRegistryServerState.INIT); - private final List connectionStateListeners = new CopyOnWriteArrayList<>(); - - private final ReentrantLock lifecycleLock = new ReentrantLock(); + private final List jdbcRegistryClients = new CopyOnWriteArrayList<>(); - private final Map clientRegistrations = new LinkedHashMap<>(); + private final List connectionStateListeners = new CopyOnWriteArrayList<>(); - private final CompletableFuture closeCompletion = new CompletableFuture<>(); + private final Map jdbcRegistryClientDTOMap = + new ConcurrentHashMap<>(); private final ScheduledExecutorService schedulerThreadExecutor; private volatile long lastSuccessHeartbeat; - private static final class ClientRegistration { - - private final IJdbcRegistryClient client; - private final JdbcRegistryClientHeartbeatDTO heartbeat; - private final ReentrantLock lock = new ReentrantLock(); - private volatile boolean active = true; - - private ClientRegistration(IJdbcRegistryClient client, JdbcRegistryClientHeartbeatDTO heartbeat) { - this.client = client; - this.heartbeat = heartbeat; - } - } - - private static final class HeartbeatUpdateException extends RuntimeException { - - private final ClientRegistration registration; - - private HeartbeatUpdateException(ClientRegistration registration) { - super("The client heartbeat record no longer exists: " + registration.heartbeat.getId()); - this.registration = registration; - } - } - public JdbcRegistryServer(JdbcRegistryDataRepository jdbcRegistryDataRepository, JdbcRegistryLockRepository jdbcRegistryLockRepository, JdbcRegistryClientRepository jdbcRegistryClientRepository, @@ -132,52 +106,30 @@ public JdbcRegistryServer(JdbcRegistryDataRepository jdbcRegistryDataRepository, } @Override - public void start() { - lifecycleLock.lock(); - try { - if (serverState.get() != JdbcRegistryServerState.INIT) { - // The server is already started or stopped, will not start again. - return; - } - // Start the Purge thread - // The Purge thread will clear the invalidated data - purgeInvalidJdbcRegistryMetadata(); - schedulerThreadExecutor.scheduleWithFixedDelay( - this::purgeInvalidJdbcRegistryMetadata, - jdbcRegistryProperties.getSessionTimeout().toMillis(), - jdbcRegistryProperties.getSessionTimeout().toMillis(), - TimeUnit.MILLISECONDS); - jdbcRegistryDataManager.start(); - if (!serverState.compareAndSet(JdbcRegistryServerState.INIT, JdbcRegistryServerState.STARTED)) { - log.warn("The JdbcRegistryServer state changed before startup completed: {}", serverState.get()); - return; - } - } finally { - lifecycleLock.unlock(); + public synchronized void start() { + if (serverState.get() != JdbcRegistryServerState.INIT) { + // The server is already started or stopped, will not start again. + return; } - - lifecycleLock.lock(); - try { - if (serverState.get() != JdbcRegistryServerState.STARTED) { - return; - } - } finally { - lifecycleLock.unlock(); + // Start the Purge thread + // The Purge thread will clear the invalidated data + purgeInvalidJdbcRegistryMetadata(); + schedulerThreadExecutor.scheduleWithFixedDelay( + this::purgeInvalidJdbcRegistryMetadata, + jdbcRegistryProperties.getSessionTimeout().toMillis(), + jdbcRegistryProperties.getSessionTimeout().toMillis(), + TimeUnit.MILLISECONDS); + jdbcRegistryDataManager.start(); + if (!serverState.compareAndSet(JdbcRegistryServerState.INIT, JdbcRegistryServerState.STARTED)) { + log.warn("The JdbcRegistryServer state changed before startup completed: {}", serverState.get()); + return; } doTriggerOnConnectedListener(); - - lifecycleLock.lock(); - try { - if (serverState.get() == JdbcRegistryServerState.STARTED && !schedulerThreadExecutor.isShutdown()) { - schedulerThreadExecutor.scheduleWithFixedDelay( - this::refreshClientsHeartbeat, - 0, - jdbcRegistryProperties.getHeartbeatRefreshInterval().toMillis(), - TimeUnit.MILLISECONDS); - } - } finally { - lifecycleLock.unlock(); - } + schedulerThreadExecutor.scheduleWithFixedDelay( + this::refreshClientsHeartbeat, + 0, + jdbcRegistryProperties.getHeartbeatRefreshInterval().toMillis(), + TimeUnit.MILLISECONDS); } @SneakyThrows @@ -198,23 +150,12 @@ public void registerClient(IJdbcRegistryClient jdbcRegistryClient) { .lastHeartbeatTime(System.currentTimeMillis()) .build(); - lifecycleLock.lock(); - try { - JdbcRegistryServerState currentState = serverState.get(); - if (currentState == JdbcRegistryServerState.STOPPED - || currentState == JdbcRegistryServerState.DISCONNECTED) { - throw new IllegalStateException("Cannot register a client when the JdbcRegistryServer is " - + currentState); - } - if (clientRegistrations.containsKey(jdbcRegistryClientIdentify)) { - throw new IllegalArgumentException("The client is already registered: " + jdbcRegistryClientIdentify); - } - jdbcRegistryClientRepository.insert(registryClientDTO); - clientRegistrations.put( - jdbcRegistryClientIdentify, new ClientRegistration(jdbcRegistryClient, registryClientDTO)); - } finally { - lifecycleLock.unlock(); + if (jdbcRegistryClientDTOMap.containsKey(jdbcRegistryClientIdentify)) { + throw new IllegalArgumentException("The client is already registered: " + jdbcRegistryClientIdentify); } + jdbcRegistryClientRepository.insert(registryClientDTO); + jdbcRegistryClients.add(jdbcRegistryClient); + jdbcRegistryClientDTOMap.put(jdbcRegistryClientIdentify, registryClientDTO); } @Override @@ -223,23 +164,10 @@ public void deregisterClient(IJdbcRegistryClient jdbcRegistryClient) { final JdbcRegistryClientIdentify clientIdentify = jdbcRegistryClient.getJdbcRegistryClientIdentify(); checkNotNull(clientIdentify); - lifecycleLock.lock(); - try { - ClientRegistration registration = clientRegistrations.get(clientIdentify); - if (registration == null || registration.client != jdbcRegistryClient) { - return; - } - registration.active = false; - clientRegistrations.remove(clientIdentify, registration); - registration.lock.lock(); - try { - purgeClientRegistration(registration); - } finally { - registration.lock.unlock(); - } - } finally { - lifecycleLock.unlock(); - } + jdbcRegistryClients.removeIf(client -> clientIdentify.equals(client.getJdbcRegistryClientIdentify())); + jdbcRegistryClientDTOMap.remove(jdbcRegistryClient.getJdbcRegistryClientIdentify()); + + doPurgeJdbcRegistryClientInDB(Lists.newArrayList(clientIdentify.getClientId())); } @Override @@ -332,58 +260,27 @@ public void releaseJdbcRegistryLock(Long clientId, String lockKey) { @Override public void close() { - List registrationsToClose; - boolean waitForClose = false; - lifecycleLock.lock(); - try { + synchronized (this) { JdbcRegistryServerState currentState = serverState.get(); if (currentState == JdbcRegistryServerState.STOPPED) { - waitForClose = true; - registrationsToClose = null; - } else if (!serverState.compareAndSet(currentState, JdbcRegistryServerState.STOPPED)) { + log.warn("The JdbcRegistryServer is already STOPPED."); + return; + } + if (!serverState.compareAndSet(currentState, JdbcRegistryServerState.STOPPED)) { log.warn("Failed to stop JdbcRegistryServer from state {}, current state is {}", currentState, serverState.get()); return; - } else { - registrationsToClose = new ArrayList<>(clientRegistrations.values()); - clientRegistrations.clear(); - registrationsToClose.forEach(registration -> registration.active = false); } - } finally { - lifecycleLock.unlock(); - } - - if (waitForClose) { - closeCompletion.join(); - return; - } - - try { - schedulerThreadExecutor.shutdownNow(); - for (ClientRegistration registration : registrationsToClose) { - registration.lock.lock(); - try { - purgeClientRegistration(registration); - } finally { - registration.lock.unlock(); - } - } - try { - if (!schedulerThreadExecutor.awaitTermination( - Math.max(1, jdbcRegistryProperties.getHeartbeatRefreshInterval().toMillis()), - TimeUnit.MILLISECONDS)) { - log.warn("The JdbcRegistryServer scheduler did not terminate within the close timeout."); - } - } catch (InterruptedException ex) { - Thread.currentThread().interrupt(); - throw new IllegalStateException("Interrupted while closing the JdbcRegistryServer", ex); - } - closeCompletion.complete(null); - } catch (RuntimeException ex) { - closeCompletion.completeExceptionally(ex); - throw ex; } + schedulerThreadExecutor.shutdown(); + List clientIds = jdbcRegistryClients.stream() + .map(IJdbcRegistryClient::getJdbcRegistryClientIdentify) + .map(JdbcRegistryClientIdentify::getClientId) + .collect(Collectors.toList()); + doPurgeJdbcRegistryClientInDB(clientIds); + jdbcRegistryClients.clear(); + jdbcRegistryClientDTOMap.clear(); } private void purgeInvalidJdbcRegistryMetadata() { @@ -395,98 +292,85 @@ private void purgeInvalidJdbcRegistryMetadata() { } // remove the client which is already dead from the registry, and remove it's related data and lock. final List jdbcRegistryClients = jdbcRegistryClientRepository.queryAll(); - jdbcRegistryClients.stream() + final Set deadJdbcRegistryClientIds = jdbcRegistryClients + .stream() .filter(JdbcRegistryClientHeartbeatDTO::isDead) - .filter(jdbcRegistryClient -> jdbcRegistryClientRepository.deleteByIdAndLastHeartbeatTime( - jdbcRegistryClient.getId(), jdbcRegistryClient.getLastHeartbeatTime())) - .forEach(jdbcRegistryClient -> log.info( - "Success delete dead jdbcRegistryClient: {}", jdbcRegistryClient.getId())); + .map(JdbcRegistryClientHeartbeatDTO::getId) + .collect(Collectors.toSet()); + doPurgeJdbcRegistryClientInDB(deadJdbcRegistryClientIds); - // Remove data and locks only while the database still confirms that the owner has no heartbeat. - purgeInactiveMetadata(null); - stopWatch.stop(); - log.debug("Success purge invalid jdbcRegistryMetadata, cost: {} ms", stopWatch.getTime()); - } - - private void purgeClientRegistration(ClientRegistration registration) { - Long clientId = registration.heartbeat.getId(); - log.info("Begin to delete dead jdbcRegistryClient: {}", clientId); - jdbcRegistryClientRepository.deleteByIdAndLastHeartbeatTime( - clientId, registration.heartbeat.getLastHeartbeatTime()); - purgeInactiveMetadata(Lists.newArrayList(clientId)); - log.info("Success delete dead jdbcRegistryClient: {}", clientId); - } - - private void purgeInactiveMetadata(Collection candidateClientIds) { - Set candidates = candidateClientIds == null - ? null - : candidateClientIds.stream().collect(Collectors.toSet()); + // remove the data and lock which client is not exist. + final Set existJdbcRegistryClientIds = jdbcRegistryClients + .stream() + .map(JdbcRegistryClientHeartbeatDTO::getId) + .filter(id -> !deadJdbcRegistryClientIds.contains(id)) + .collect(Collectors.toSet()); jdbcRegistryDataManager.getAllJdbcRegistryData() .stream() + .filter(jdbcRegistryDataDTO -> !existJdbcRegistryClientIds.contains(jdbcRegistryDataDTO.getClientId())) .filter(jdbcRegistryDataDTO -> DataType.EPHEMERAL.name().equals(jdbcRegistryDataDTO.getDataType())) - .filter(jdbcRegistryDataDTO -> candidates == null - || candidates.contains(jdbcRegistryDataDTO.getClientId())) .forEach(jdbcRegistryData -> { log.info("Remove the JdbcRegistryData: {} which client is not exist in the registry", jdbcRegistryData); - jdbcRegistryDataManager.deleteEphemeralDataIfClientInactive(jdbcRegistryData); + jdbcRegistryDataManager.deleteJdbcRegistryDataByKey(jdbcRegistryData.getDataKey()); }); jdbcRegistryLockRepository.queryAll() .stream() - .filter(jdbcRegistryLockDTO -> candidates == null - || candidates.contains(jdbcRegistryLockDTO.getClientId())) - .forEach(jdbcRegistryLock -> { + .filter(jdbcRegistryLockDTO -> !existJdbcRegistryClientIds.contains(jdbcRegistryLockDTO.getClientId())) + .forEach(jdbcRegistryLockDTO -> { log.info("Remove the JdbcRegistryLock: {} which client is not exist in the registry", - jdbcRegistryLock); - jdbcRegistryLockManager.deleteIfInactive(jdbcRegistryLock); + jdbcRegistryLockDTO); + jdbcRegistryLockRepository.deleteById(jdbcRegistryLockDTO.getId()); }); + stopWatch.stop(); + log.debug("Success purge invalid jdbcRegistryMetadata, cost: {} ms", stopWatch.getTime()); + } + + private void doPurgeJdbcRegistryClientInDB(final Collection jdbcRegistryClientIds) { + if (CollectionUtils.isEmpty(jdbcRegistryClientIds)) { + return; + } + log.info("Begin to delete dead jdbcRegistryClient: {}", jdbcRegistryClientIds); + jdbcRegistryClientRepository.deleteByIds(jdbcRegistryClientIds); + log.info("Success delete dead jdbcRegistryClient: {}", jdbcRegistryClientIds); } private void refreshClientsHeartbeat() { - List registrations; - lifecycleLock.lock(); - try { - JdbcRegistryServerState currentState = serverState.get(); - if (currentState == JdbcRegistryServerState.STOPPED - || currentState == JdbcRegistryServerState.DISCONNECTED) { - return; - } - registrations = new ArrayList<>(clientRegistrations.values()); - } finally { - lifecycleLock.unlock(); + if (CollectionUtils.isEmpty(jdbcRegistryClients)) { + return; } - if (registrations.isEmpty()) { + JdbcRegistryServerState currentState = getServerState(); + if (currentState == JdbcRegistryServerState.STOPPED + || currentState == JdbcRegistryServerState.DISCONNECTED) { + log.warn("The JdbcRegistryServer is {}, will not refresh clients: {} heartbeat.", + currentState, + CollectionUtils.collect(jdbcRegistryClients, IJdbcRegistryClient::getJdbcRegistryClientIdentify)); return; } - - ClientRegistration failedRegistration = null; + // Refresh the client's term try { - long heartbeatTime = System.currentTimeMillis(); - for (ClientRegistration registration : registrations) { - failedRegistration = registration; - registration.lock.lock(); - try { - if (!registration.active || !isHeartbeatRefreshAllowed()) { - continue; - } - JdbcRegistryClientHeartbeatDTO clone = registration.heartbeat.clone(); - clone.setLastHeartbeatTime(heartbeatTime); - if (!jdbcRegistryClientRepository.updateById(clone)) { - log.error("The client heartbeat has expired: {}", registration.heartbeat.getId()); - throw new HeartbeatUpdateException(registration); - } - if (registration.active) { - registration.heartbeat.setLastHeartbeatTime(clone.getLastHeartbeatTime()); - } - } finally { - registration.lock.unlock(); + long now = System.currentTimeMillis(); + for (IJdbcRegistryClient jdbcRegistryClient : jdbcRegistryClients) { + JdbcRegistryClientHeartbeatDTO jdbcRegistryClientHeartbeatDTO = + jdbcRegistryClientDTOMap.get(jdbcRegistryClient.getJdbcRegistryClientIdentify()); + if (jdbcRegistryClientHeartbeatDTO == null) { + // This may occur when the data in db has been deleted, but the client is still alive. + log.error( + "The client {} is not found in the registry, will not refresh it's term. (This may happen when the client is removed from the db)", + jdbcRegistryClient.getJdbcRegistryClientIdentify().getClientId()); + continue; + } + JdbcRegistryClientHeartbeatDTO clone = jdbcRegistryClientHeartbeatDTO.clone(); + clone.setLastHeartbeatTime(now); + if (!jdbcRegistryClientRepository.updateById(clone)) { + log.error("The client heartbeat has expired: {}", jdbcRegistryClientHeartbeatDTO.getId()); + throw new IllegalStateException( + "The client heartbeat record no longer exists: " + jdbcRegistryClientHeartbeatDTO.getId()); } + jdbcRegistryClientHeartbeatDTO.setLastHeartbeatTime(clone.getLastHeartbeatTime()); } - - boolean reconnect = false; - lifecycleLock.lock(); - try { - JdbcRegistryServerState currentState = serverState.get(); + synchronized (this) { + currentState = serverState.get(); if (currentState == JdbcRegistryServerState.STOPPED || currentState == JdbcRegistryServerState.DISCONNECTED) { return; @@ -497,70 +381,42 @@ private void refreshClientsHeartbeat() { log.debug("Failed to reconnect JdbcRegistryServer; current state is {}", serverState.get()); return; } - reconnect = true; - } else if (currentState != JdbcRegistryServerState.STARTED) { + lastSuccessHeartbeat = now; + doTriggerReconnectedListener(); + } else if (currentState == JdbcRegistryServerState.STARTED) { + lastSuccessHeartbeat = now; + } else { return; } - lastSuccessHeartbeat = System.currentTimeMillis(); - } finally { - lifecycleLock.unlock(); - } - if (reconnect) { - doTriggerReconnectedListener(); } log.debug("Success refresh clients: {} heartbeat.", - registrations.stream() - .map(registration -> registration.client.getJdbcRegistryClientIdentify()) - .collect(Collectors.toList())); - } catch (HeartbeatUpdateException ex) { - log.error("Failed to refresh the client's term", ex); - handleHeartbeatFailure(ex.registration); + CollectionUtils.collect(jdbcRegistryClients, IJdbcRegistryClient::getJdbcRegistryClientIdentify)); } catch (Exception ex) { log.error("Failed to refresh the client's term", ex); - handleHeartbeatFailure(failedRegistration); - } - } - - private boolean isHeartbeatRefreshAllowed() { - JdbcRegistryServerState currentState = serverState.get(); - return currentState != JdbcRegistryServerState.STOPPED - && currentState != JdbcRegistryServerState.DISCONNECTED; - } - - private void handleHeartbeatFailure(ClientRegistration failedRegistration) { - if (failedRegistration != null && !failedRegistration.active) { - return; - } - boolean disconnected = false; - long sessionTimeoutMillis = jdbcRegistryProperties.getSessionTimeout().toMillis(); - lifecycleLock.lock(); - try { - JdbcRegistryServerState currentState = serverState.get(); - if (currentState == JdbcRegistryServerState.STOPPED - || currentState == JdbcRegistryServerState.DISCONNECTED) { - return; - } - boolean sessionTimedOut = System.currentTimeMillis() - lastSuccessHeartbeat > sessionTimeoutMillis; - if (sessionTimedOut) { - if ((currentState == JdbcRegistryServerState.STARTED - || currentState == JdbcRegistryServerState.SUSPENDED) - && serverState.compareAndSet(currentState, JdbcRegistryServerState.DISCONNECTED)) { - disconnected = true; - } else { - log.debug("Failed to disconnect JdbcRegistryServer from state {}, current state is {}", - currentState, - serverState.get()); + long sessionTimeoutMillis = jdbcRegistryProperties.getSessionTimeout().toMillis(); + synchronized (this) { + currentState = serverState.get(); + if (currentState == JdbcRegistryServerState.STOPPED + || currentState == JdbcRegistryServerState.DISCONNECTED) { + return; + } + boolean sessionTimedOut = System.currentTimeMillis() - lastSuccessHeartbeat > sessionTimeoutMillis; + if (sessionTimedOut) { + if ((currentState == JdbcRegistryServerState.STARTED + || currentState == JdbcRegistryServerState.SUSPENDED) + && serverState.compareAndSet(currentState, JdbcRegistryServerState.DISCONNECTED)) { + doTriggerOnDisConnectedListener(); + } else { + log.debug("Failed to disconnect JdbcRegistryServer from state {}, current state is {}", + currentState, + serverState.get()); + } + } else if (currentState == JdbcRegistryServerState.STARTED + && !serverState.compareAndSet( + JdbcRegistryServerState.STARTED, JdbcRegistryServerState.SUSPENDED)) { + log.debug("Failed to suspend JdbcRegistryServer; current state is {}", serverState.get()); } - } else if (currentState == JdbcRegistryServerState.STARTED - && !serverState.compareAndSet( - JdbcRegistryServerState.STARTED, JdbcRegistryServerState.SUSPENDED)) { - log.debug("Failed to suspend JdbcRegistryServer; current state is {}", serverState.get()); } - } finally { - lifecycleLock.unlock(); - } - if (disconnected) { - doTriggerOnDisConnectedListener(); } } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java index f4d516665dae..b4509ff222ad 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java @@ -17,29 +17,22 @@ package org.apache.dolphinscheduler.plugin.registry.jdbc.server; -import static org.junit.jupiter.api.Assertions.assertThrows; - import org.apache.dolphinscheduler.plugin.registry.jdbc.JdbcRegistryProperties; import org.apache.dolphinscheduler.plugin.registry.jdbc.client.IJdbcRegistryClient; import org.apache.dolphinscheduler.plugin.registry.jdbc.client.JdbcRegistryClientIdentify; import org.apache.dolphinscheduler.plugin.registry.jdbc.model.DTO.JdbcRegistryClientHeartbeatDTO; -import org.apache.dolphinscheduler.plugin.registry.jdbc.model.DTO.JdbcRegistryDataDTO; import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryClientRepository; import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryDataChangeEventRepository; import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryDataRepository; import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryLockRepository; import java.time.Duration; -import java.util.Collections; -import java.util.List; import java.util.Map; 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.TimeoutException; -import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; @@ -118,7 +111,7 @@ void close_shouldOnlyPurgeClientsOnceWhenCalledConcurrently() throws Exception { allowFirstPurgeToFinish.await(5, TimeUnit.SECONDS); } return null; - }).when(jdbcRegistryClientRepository).deleteByIdAndLastHeartbeatTime(Mockito.any(), Mockito.any()); + }).when(jdbcRegistryClientRepository).deleteByIds(Mockito.any()); ExecutorService closeExecutor = Executors.newFixedThreadPool(2); Future firstClose = closeExecutor.submit(jdbcRegistryServer::close); Future secondClose = null; @@ -126,12 +119,9 @@ void close_shouldOnlyPurgeClientsOnceWhenCalledConcurrently() throws Exception { try { Truth.assertThat(firstPurgeStarted.await(5, TimeUnit.SECONDS)).isTrue(); secondClose = closeExecutor.submit(jdbcRegistryServer::close); - try { - secondClose.get(200, TimeUnit.MILLISECONDS); - Truth.assertWithMessage("second close returned before cleanup completed").fail(); - } catch (TimeoutException expected) { - // The second close must wait for the first close to finish cleanup. - } + secondClose.get(5, TimeUnit.SECONDS); + + Truth.assertThat(purgeInvocations.get()).isEqualTo(1); } finally { allowFirstPurgeToFinish.countDown(); firstClose.get(5, TimeUnit.SECONDS); @@ -140,178 +130,6 @@ void close_shouldOnlyPurgeClientsOnceWhenCalledConcurrently() throws Exception { } closeExecutor.shutdownNow(); } - - Truth.assertThat(purgeInvocations.get()).isEqualTo(1); - } - - @Test - void registerClient_shouldRejectAfterClose() { - Mockito.clearInvocations(jdbcRegistryClientRepository); - jdbcRegistryServer.close(); - IJdbcRegistryClient secondClient = Mockito.mock(IJdbcRegistryClient.class); - JdbcRegistryClientIdentify secondClientIdentify = new JdbcRegistryClientIdentify(2L, "second-client"); - Mockito.when(secondClient.getJdbcRegistryClientIdentify()).thenReturn(secondClientIdentify); - - Truth.assertThat( - assertThrows( - IllegalStateException.class, - () -> jdbcRegistryServer.registerClient(secondClient))) - .hasMessageThat() - .contains("STOPPED"); - Mockito.verify(jdbcRegistryClientRepository, Mockito.never()).insert(Mockito.any()); - } - - @Test - void deregisterClient_shouldNotSuspendServerWhenHeartbeatUsesRemovedClient() throws Exception { - setServerState(JdbcRegistryServerState.STARTED); - CountDownLatch heartbeatUpdateStarted = new CountDownLatch(1); - CountDownLatch allowHeartbeatUpdateToFinish = new CountDownLatch(1); - Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenAnswer(invocation -> { - heartbeatUpdateStarted.countDown(); - allowHeartbeatUpdateToFinish.await(5, TimeUnit.SECONDS); - return false; - }); - ExecutorService heartbeatExecutor = Executors.newSingleThreadExecutor(); - Future heartbeatFuture = heartbeatExecutor.submit(() -> { - ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); - }); - - try { - Truth.assertThat(heartbeatUpdateStarted.await(5, TimeUnit.SECONDS)).isTrue(); - jdbcRegistryServer.deregisterClient(jdbcRegistryClient); - allowHeartbeatUpdateToFinish.countDown(); - heartbeatFuture.get(5, TimeUnit.SECONDS); - } finally { - allowHeartbeatUpdateToFinish.countDown(); - heartbeatExecutor.shutdownNow(); - } - - Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.STARTED); - } - - @Test - void deregisterThenRegisterSameIdentify_shouldKeepNewRegistration() throws Exception { - setServerState(JdbcRegistryServerState.STARTED); - AtomicBoolean heartbeatRecordPresent = new AtomicBoolean(true); - CountDownLatch deleteStarted = new CountDownLatch(1); - CountDownLatch allowDeleteToFinish = new CountDownLatch(1); - Mockito.doAnswer(invocation -> { - deleteStarted.countDown(); - allowDeleteToFinish.await(5, TimeUnit.SECONDS); - heartbeatRecordPresent.set(false); - return null; - }).when(jdbcRegistryClientRepository).deleteByIdAndLastHeartbeatTime(Mockito.any(), Mockito.any()); - Mockito.doAnswer(invocation -> { - heartbeatRecordPresent.set(true); - return null; - }).when(jdbcRegistryClientRepository).insert(Mockito.any()); - Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())) - .thenAnswer(invocation -> heartbeatRecordPresent.get()); - - ExecutorService deregisterExecutor = Executors.newFixedThreadPool(2); - Future deregisterFuture = - deregisterExecutor.submit(() -> jdbcRegistryServer.deregisterClient(jdbcRegistryClient)); - Future registerFuture = null; - try { - Truth.assertThat(deleteStarted.await(5, TimeUnit.SECONDS)).isTrue(); - IJdbcRegistryClient replacementClient = Mockito.mock(IJdbcRegistryClient.class); - Mockito.when(replacementClient.getJdbcRegistryClientIdentify()).thenReturn(CLIENT_IDENTIFY); - registerFuture = deregisterExecutor.submit(() -> jdbcRegistryServer.registerClient(replacementClient)); - try { - registerFuture.get(200, TimeUnit.MILLISECONDS); - Truth.assertWithMessage("replacement registration raced with old deregistration").fail(); - } catch (TimeoutException expected) { - // Registration must wait until the previous registration is fully removed. - } - allowDeleteToFinish.countDown(); - deregisterFuture.get(5, TimeUnit.SECONDS); - registerFuture.get(5, TimeUnit.SECONDS); - - ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); - } finally { - allowDeleteToFinish.countDown(); - deregisterExecutor.shutdownNow(); - } - - Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.STARTED); - Mockito.verify(jdbcRegistryClientRepository, Mockito.atLeast(2)).insert(Mockito.any()); - Mockito.verify(jdbcRegistryClientRepository, Mockito.atLeastOnce()).updateById(Mockito.any()); - } - - @Test - void onConnectedListener_shouldBeAbleToCloseServer() { - Mockito.doAnswer(invocation -> { - jdbcRegistryServer.close(); - return null; - }).when(connectionStateListener).onConnected(); - - jdbcRegistryServer.start(); - - Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.STOPPED); - } - - @Test - void onReconnectedListener_shouldNotBlockConcurrentClose() throws Exception { - setServerState(JdbcRegistryServerState.SUSPENDED); - ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", System.currentTimeMillis()); - Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenReturn(true); - CountDownLatch callbackStarted = new CountDownLatch(1); - CountDownLatch allowCallbackToFinish = new CountDownLatch(1); - Mockito.doAnswer(invocation -> { - callbackStarted.countDown(); - allowCallbackToFinish.await(5, TimeUnit.SECONDS); - return null; - }).when(connectionStateListener).onReconnected(); - - ExecutorService executor = Executors.newFixedThreadPool(2); - Future heartbeatFuture = executor.submit( - () -> ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat")); - Future closeFuture = null; - try { - Truth.assertThat(callbackStarted.await(5, TimeUnit.SECONDS)).isTrue(); - closeFuture = executor.submit(jdbcRegistryServer::close); - try { - closeFuture.get(500, TimeUnit.MILLISECONDS); - } catch (TimeoutException ex) { - Truth.assertWithMessage("close was blocked by a connection callback").fail(); - } - allowCallbackToFinish.countDown(); - heartbeatFuture.get(5, TimeUnit.SECONDS); - closeFuture.get(5, TimeUnit.SECONDS); - } finally { - allowCallbackToFinish.countDown(); - if (closeFuture != null) { - closeFuture.get(5, TimeUnit.SECONDS); - } - heartbeatFuture.get(5, TimeUnit.SECONDS); - executor.shutdownNow(); - } - - Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.STOPPED); - } - - @Test - void purgeInvalidJdbcRegistryMetadata_shouldUseConditionalMetadataCleanup() { - JdbcRegistryClientHeartbeatDTO expiredHeartbeat = JdbcRegistryClientHeartbeatDTO.builder() - .id(9L) - .clientName("expired-client") - .clientConfig(new JdbcRegistryClientHeartbeatDTO.ClientConfig(1_000L)) - .lastHeartbeatTime(0L) - .build(); - JdbcRegistryDataDTO ephemeralData = JdbcRegistryDataDTO.builder() - .id(1L) - .clientId(9L) - .dataKey("/ephemeral") - .dataValue("value") - .dataType("EPHEMERAL") - .build(); - Mockito.when(jdbcRegistryClientRepository.queryAll()).thenReturn(List.of(expiredHeartbeat)); - Mockito.when(jdbcRegistryDataRepository.selectAll()).thenReturn(List.of(ephemeralData)); - Mockito.when(jdbcRegistryLockRepository.queryAll()).thenReturn(Collections.emptyList()); - - ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "purgeInvalidJdbcRegistryMetadata"); - - Mockito.verify(jdbcRegistryDataRepository, Mockito.never()).deleteByKey(ephemeralData.getDataKey()); } @Test @@ -363,13 +181,17 @@ void refreshClientsHeartbeat_shouldKeepPartialHeartbeatUpdateWhenLaterClientFail Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())) .thenAnswer(invocation -> updateInvocations.incrementAndGet() == 1); - getHeartbeat(CLIENT_IDENTIFY).setLastHeartbeatTime(0L); + @SuppressWarnings("unchecked") + Map heartbeatMap = + (Map) ReflectionTestUtils + .getField(jdbcRegistryServer, "jdbcRegistryClientDTOMap"); + heartbeatMap.get(CLIENT_IDENTIFY).setLastHeartbeatTime(0L); ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); Truth.assertThat(updateInvocations.get()).isEqualTo(2); Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.SUSPENDED); - Truth.assertThat(getHeartbeat(CLIENT_IDENTIFY).getLastHeartbeatTime()).isGreaterThan(0L); + Truth.assertThat(heartbeatMap.get(CLIENT_IDENTIFY).getLastHeartbeatTime()).isGreaterThan(0L); Mockito.verify(connectionStateListener, Mockito.never()).onReconnected(); Mockito.verify(connectionStateListener, Mockito.never()).onDisConnected(); } @@ -498,13 +320,4 @@ private void setServerState(JdbcRegistryServerState state) { ((AtomicReference) ReflectionTestUtils.getField(jdbcRegistryServer, "serverState")) .set(state); } - - @SuppressWarnings("unchecked") - private JdbcRegistryClientHeartbeatDTO getHeartbeat(JdbcRegistryClientIdentify clientIdentify) { - Map registrations = - (Map) ReflectionTestUtils - .getField(jdbcRegistryServer, "clientRegistrations"); - return (JdbcRegistryClientHeartbeatDTO) ReflectionTestUtils - .getField(registrations.get(clientIdentify), "heartbeat"); - } } From 9a6b9e196e834a161a5852e6ed4516129dd969e5 Mon Sep 17 00:00:00 2001 From: qiuyanjun Date: Sun, 27 Sep 2026 11:02:32 +0800 Subject: [PATCH 15/16] [Fix-18274][Registry] Handle JDBC heartbeat state races --- .../jdbc/server/JdbcRegistryServer.java | 86 ++++++----- .../jdbc/server/JdbcRegistryServerTest.java | 134 +++++++++++++++++- 2 files changed, 179 insertions(+), 41 deletions(-) diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java index 71ddaf3810d5..2ae5430838d9 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java @@ -261,16 +261,19 @@ public void releaseJdbcRegistryLock(Long clientId, String lockKey) { @Override public void close() { synchronized (this) { - JdbcRegistryServerState currentState = serverState.get(); - if (currentState == JdbcRegistryServerState.STOPPED) { - log.warn("The JdbcRegistryServer is already STOPPED."); - return; - } - if (!serverState.compareAndSet(currentState, JdbcRegistryServerState.STOPPED)) { - log.warn("Failed to stop JdbcRegistryServer from state {}, current state is {}", + while (true) { + JdbcRegistryServerState currentState = serverState.get(); + if (currentState == JdbcRegistryServerState.STOPPED) { + log.warn("The JdbcRegistryServer is already STOPPED."); + return; + } + if (serverState.compareAndSet(currentState, JdbcRegistryServerState.STOPPED)) { + break; + } + // A heartbeat can change the state without this monitor. Do not drop the close request. + log.debug("Failed to stop JdbcRegistryServer from state {}, current state is {}, retrying", currentState, serverState.get()); - return; } } schedulerThreadExecutor.shutdown(); @@ -369,53 +372,52 @@ private void refreshClientsHeartbeat() { } jdbcRegistryClientHeartbeatDTO.setLastHeartbeatTime(clone.getLastHeartbeatTime()); } + currentState = serverState.get(); + boolean reconnected = currentState == JdbcRegistryServerState.SUSPENDED; + if (reconnected) { + if (!serverState.compareAndSet(JdbcRegistryServerState.SUSPENDED, JdbcRegistryServerState.STARTED)) { + log.debug("Failed to reconnect JdbcRegistryServer; current state is {}", serverState.get()); + return; + } + } else if (currentState != JdbcRegistryServerState.STARTED) { + return; + } + // Serialize heartbeat side effects with close(), even if close wins after the state transition. synchronized (this) { - currentState = serverState.get(); - if (currentState == JdbcRegistryServerState.STOPPED - || currentState == JdbcRegistryServerState.DISCONNECTED) { + if (serverState.get() != JdbcRegistryServerState.STARTED) { return; } - if (currentState == JdbcRegistryServerState.SUSPENDED) { - if (!serverState.compareAndSet( - JdbcRegistryServerState.SUSPENDED, JdbcRegistryServerState.STARTED)) { - log.debug("Failed to reconnect JdbcRegistryServer; current state is {}", serverState.get()); - return; - } - lastSuccessHeartbeat = now; + lastSuccessHeartbeat = now; + if (reconnected) { doTriggerReconnectedListener(); - } else if (currentState == JdbcRegistryServerState.STARTED) { - lastSuccessHeartbeat = now; - } else { - return; } } log.debug("Success refresh clients: {} heartbeat.", CollectionUtils.collect(jdbcRegistryClients, IJdbcRegistryClient::getJdbcRegistryClientIdentify)); } catch (Exception ex) { log.error("Failed to refresh the client's term", ex); + currentState = serverState.get(); + if (currentState != JdbcRegistryServerState.STARTED + && currentState != JdbcRegistryServerState.SUSPENDED) { + return; + } long sessionTimeoutMillis = jdbcRegistryProperties.getSessionTimeout().toMillis(); - synchronized (this) { - currentState = serverState.get(); - if (currentState == JdbcRegistryServerState.STOPPED - || currentState == JdbcRegistryServerState.DISCONNECTED) { + if (System.currentTimeMillis() - lastSuccessHeartbeat > sessionTimeoutMillis) { + if (!serverState.compareAndSet(currentState, JdbcRegistryServerState.DISCONNECTED)) { + log.debug("Failed to disconnect JdbcRegistryServer from state {}, current state is {}", + currentState, + serverState.get()); return; } - boolean sessionTimedOut = System.currentTimeMillis() - lastSuccessHeartbeat > sessionTimeoutMillis; - if (sessionTimedOut) { - if ((currentState == JdbcRegistryServerState.STARTED - || currentState == JdbcRegistryServerState.SUSPENDED) - && serverState.compareAndSet(currentState, JdbcRegistryServerState.DISCONNECTED)) { + synchronized (this) { + if (serverState.get() == JdbcRegistryServerState.DISCONNECTED) { doTriggerOnDisConnectedListener(); - } else { - log.debug("Failed to disconnect JdbcRegistryServer from state {}, current state is {}", - currentState, - serverState.get()); } - } else if (currentState == JdbcRegistryServerState.STARTED - && !serverState.compareAndSet( - JdbcRegistryServerState.STARTED, JdbcRegistryServerState.SUSPENDED)) { - log.debug("Failed to suspend JdbcRegistryServer; current state is {}", serverState.get()); } + } else if (currentState == JdbcRegistryServerState.STARTED + && !serverState.compareAndSet(JdbcRegistryServerState.STARTED, JdbcRegistryServerState.SUSPENDED)) { + log.debug("Failed to suspend JdbcRegistryServer; current state is {}", serverState.get()); + return; } } } @@ -423,6 +425,9 @@ private void refreshClientsHeartbeat() { private void doTriggerReconnectedListener() { log.info("Trigger:onReconnected listener."); connectionStateListeners.forEach(listener -> { + if (serverState.get() != JdbcRegistryServerState.STARTED) { + return; + } try { listener.onReconnected(); } catch (Exception ex) { @@ -445,6 +450,9 @@ private void doTriggerOnConnectedListener() { private void doTriggerOnDisConnectedListener() { log.info("Trigger:onDisConnected listener."); connectionStateListeners.forEach(listener -> { + if (serverState.get() != JdbcRegistryServerState.DISCONNECTED) { + return; + } try { listener.onDisConnected(); } catch (Exception ex) { diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java index b4509ff222ad..ebb4bede963b 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java @@ -17,6 +17,8 @@ package org.apache.dolphinscheduler.plugin.registry.jdbc.server; +import static org.awaitility.Awaitility.await; + import org.apache.dolphinscheduler.plugin.registry.jdbc.JdbcRegistryProperties; import org.apache.dolphinscheduler.plugin.registry.jdbc.client.IJdbcRegistryClient; import org.apache.dolphinscheduler.plugin.registry.jdbc.client.JdbcRegistryClientIdentify; @@ -41,6 +43,8 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.Mockito; @@ -158,15 +162,76 @@ void refreshClientsHeartbeat_shouldDisconnectImmediatelyWhenStartedHeartbeatReco } @Test - void refreshClientsHeartbeat_shouldSuspendWhenStartedHeartbeatRecordWasPurgedBeforeTimeout() { + void refreshClientsHeartbeat_shouldSuspendUntilMissingHeartbeatTimesOut() { setServerState(JdbcRegistryServerState.STARTED); - ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", System.currentTimeMillis()); + long lastSuccessHeartbeat = System.currentTimeMillis(); + ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", lastSuccessHeartbeat); Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenReturn(false); ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.SUSPENDED); + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.SUSPENDED); + Truth.assertThat((long) ReflectionTestUtils.getField(jdbcRegistryServer, "lastSuccessHeartbeat")) + .isEqualTo(lastSuccessHeartbeat); Mockito.verify(connectionStateListener, Mockito.never()).onDisConnected(); + + ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", 0L); + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.DISCONNECTED); + Mockito.verify(connectionStateListener).onDisConnected(); + Mockito.verify(connectionStateListener, Mockito.never()).onReconnected(); + Mockito.verify(jdbcRegistryClientRepository, Mockito.times(3)).updateById(Mockito.any()); + } + + @Test + void refreshClientsHeartbeat_shouldReconnectOnceAfterTransientFailure() { + setServerState(JdbcRegistryServerState.STARTED); + ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", System.currentTimeMillis()); + Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())) + .thenThrow(new IllegalStateException("Database unavailable")) + .thenReturn(true); + + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.SUSPENDED); + Mockito.verifyNoInteractions(connectionStateListener); + + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.STARTED); + Mockito.verify(connectionStateListener).onReconnected(); + Mockito.verify(connectionStateListener, Mockito.never()).onDisConnected(); + Mockito.verify(jdbcRegistryClientRepository, Mockito.times(3)).updateById(Mockito.any()); + } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void refreshClientsHeartbeat_shouldStopNotifyingWhenListenerClosesServer(boolean heartbeatSucceeds) { + setServerState(JdbcRegistryServerState.SUSPENDED); + ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", 0L); + Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenReturn(heartbeatSucceeds); + if (heartbeatSucceeds) { + Mockito.doAnswer(invocation -> { + jdbcRegistryServer.close(); + return null; + }).when(connectionStateListener).onReconnected(); + } else { + Mockito.doAnswer(invocation -> { + jdbcRegistryServer.close(); + return null; + }).when(connectionStateListener).onDisConnected(); + } + ConnectionStateListener laterListener = Mockito.mock(ConnectionStateListener.class); + jdbcRegistryServer.subscribeConnectionStateChange(laterListener); + + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat"); + + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.STOPPED); + Mockito.verifyNoInteractions(laterListener); } @Test @@ -288,6 +353,71 @@ void refreshClientsHeartbeat_shouldNotSuspendWhenCloseWinsFailedHeartbeatRace() Mockito.verify(connectionStateListener, Mockito.never()).onDisConnected(); } + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void refreshClientsHeartbeat_shouldPreserveCloseWhenFailureTransitionLosesRace(boolean timedOut) throws Exception { + setServerState(JdbcRegistryServerState.STARTED); + ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", + timedOut ? 0L : System.currentTimeMillis()); + JdbcRegistryProperties properties = Mockito.spy((JdbcRegistryProperties) ReflectionTestUtils + .getField(jdbcRegistryServer, "jdbcRegistryProperties")); + ReflectionTestUtils.setField(jdbcRegistryServer, "jdbcRegistryProperties", properties); + CountDownLatch timeoutCheckStarted = new CountDownLatch(1); + CountDownLatch allowTimeoutCheck = new CountDownLatch(1); + Mockito.doAnswer(invocation -> { + timeoutCheckStarted.countDown(); + Truth.assertThat(allowTimeoutCheck.await(5, TimeUnit.SECONDS)).isTrue(); + return timedOut ? Duration.ZERO : Duration.ofDays(1); + }).when(properties).getSessionTimeout(); + Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenReturn(false); + ExecutorService heartbeatExecutor = Executors.newSingleThreadExecutor(); + Future heartbeatFuture = heartbeatExecutor + .submit(() -> ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat")); + + try { + Truth.assertThat(timeoutCheckStarted.await(5, TimeUnit.SECONDS)).isTrue(); + jdbcRegistryServer.close(); + allowTimeoutCheck.countDown(); + heartbeatFuture.get(5, TimeUnit.SECONDS); + } finally { + allowTimeoutCheck.countDown(); + heartbeatExecutor.shutdownNow(); + } + + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.STOPPED); + Mockito.verifyNoInteractions(connectionStateListener); + } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void refreshClientsHeartbeat_shouldNotNotifyWhenClosedAfterStateTransition(boolean heartbeatSucceeds) throws Exception { + setServerState(JdbcRegistryServerState.SUSPENDED); + ReflectionTestUtils.setField(jdbcRegistryServer, "lastSuccessHeartbeat", 42L); + Mockito.when(jdbcRegistryClientRepository.updateById(Mockito.any())).thenReturn(heartbeatSucceeds); + ExecutorService heartbeatExecutor = Executors.newSingleThreadExecutor(); + Future heartbeatFuture; + try { + synchronized (jdbcRegistryServer) { + heartbeatFuture = heartbeatExecutor + .submit(() -> ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "refreshClientsHeartbeat")); + // Pause notification after the CAS, then let close() win before the heartbeat commits its effects. + await().atMost(Duration.ofSeconds(5)) + .until(() -> jdbcRegistryServer + .getServerState() == (heartbeatSucceeds ? JdbcRegistryServerState.STARTED + : JdbcRegistryServerState.DISCONNECTED)); + jdbcRegistryServer.close(); + } + heartbeatFuture.get(5, TimeUnit.SECONDS); + } finally { + heartbeatExecutor.shutdownNow(); + } + + Truth.assertThat(jdbcRegistryServer.getServerState()).isEqualTo(JdbcRegistryServerState.STOPPED); + Truth.assertThat((long) ReflectionTestUtils.getField(jdbcRegistryServer, "lastSuccessHeartbeat")) + .isEqualTo(42L); + Mockito.verifyNoInteractions(connectionStateListener); + } + @Test void refreshClientsHeartbeat_shouldPersistCurrentHeartbeatTimestamp() { ArgumentCaptor registeredHeartbeat = From dc411363bb0fa30685f721e381d613a25020670c Mon Sep 17 00:00:00 2001 From: qiuyanjun Date: Mon, 28 Sep 2026 14:49:05 +0800 Subject: [PATCH 16/16] [Fix-18274][Registry] Guard expired heartbeat cleanup against renewal --- .../JdbcRegistryClientHeartbeatMapper.java | 6 +++++ .../JdbcRegistryClientRepository.java | 4 +++ .../jdbc/server/JdbcRegistryServer.java | 7 ++--- .../jdbc/server/JdbcRegistryServerTest.java | 27 +++++++++++++++++++ 4 files changed, 41 insertions(+), 3 deletions(-) diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryClientHeartbeatMapper.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryClientHeartbeatMapper.java index 2b8499bb4812..73c2b66772f2 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryClientHeartbeatMapper.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/mapper/JdbcRegistryClientHeartbeatMapper.java @@ -19,6 +19,8 @@ import org.apache.dolphinscheduler.plugin.registry.jdbc.model.DO.JdbcRegistryClientHeartbeat; +import org.apache.ibatis.annotations.Delete; +import org.apache.ibatis.annotations.Param; import org.apache.ibatis.annotations.Select; import java.util.List; @@ -30,4 +32,8 @@ public interface JdbcRegistryClientHeartbeatMapper extends BaseMapper selectAll(); + @Delete("delete from t_ds_jdbc_registry_client_heartbeat " + + "where id = #{id} and last_heartbeat_time = #{lastHeartbeatTime}") + int deleteByIdAndLastHeartbeatTime(@Param("id") Long id, @Param("lastHeartbeatTime") Long lastHeartbeatTime); + } diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryClientRepository.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryClientRepository.java index 1791f3c942aa..cee889bf05c8 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryClientRepository.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/repository/JdbcRegistryClientRepository.java @@ -52,6 +52,10 @@ public void deleteByIds(Collection clientIds) { jdbcRegistryClientHeartbeatMapper.deleteBatchIds(clientIds); } + public boolean deleteByIdAndLastHeartbeatTime(Long id, Long lastHeartbeatTime) { + return jdbcRegistryClientHeartbeatMapper.deleteByIdAndLastHeartbeatTime(id, lastHeartbeatTime) == 1; + } + public boolean updateById(JdbcRegistryClientHeartbeatDTO jdbcRegistryClientHeartbeatDTO) { JdbcRegistryClientHeartbeat jdbcRegistryClientHeartbeat = JdbcRegistryClientHeartbeatDTO.toJdbcRegistryClientHeartbeat(jdbcRegistryClientHeartbeatDTO); diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java index 2ae5430838d9..8910395360c3 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/main/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServer.java @@ -295,18 +295,19 @@ private void purgeInvalidJdbcRegistryMetadata() { } // remove the client which is already dead from the registry, and remove it's related data and lock. final List jdbcRegistryClients = jdbcRegistryClientRepository.queryAll(); - final Set deadJdbcRegistryClientIds = jdbcRegistryClients + final Set deletedJdbcRegistryClientIds = jdbcRegistryClients .stream() .filter(JdbcRegistryClientHeartbeatDTO::isDead) + .filter(jdbcRegistryClient -> jdbcRegistryClientRepository.deleteByIdAndLastHeartbeatTime( + jdbcRegistryClient.getId(), jdbcRegistryClient.getLastHeartbeatTime())) .map(JdbcRegistryClientHeartbeatDTO::getId) .collect(Collectors.toSet()); - doPurgeJdbcRegistryClientInDB(deadJdbcRegistryClientIds); // remove the data and lock which client is not exist. final Set existJdbcRegistryClientIds = jdbcRegistryClients .stream() .map(JdbcRegistryClientHeartbeatDTO::getId) - .filter(id -> !deadJdbcRegistryClientIds.contains(id)) + .filter(id -> !deletedJdbcRegistryClientIds.contains(id)) .collect(Collectors.toSet()); jdbcRegistryDataManager.getAllJdbcRegistryData() .stream() diff --git a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java index ebb4bede963b..cd171d448cd0 100644 --- a/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java +++ b/dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc/src/test/java/org/apache/dolphinscheduler/plugin/registry/jdbc/server/JdbcRegistryServerTest.java @@ -23,12 +23,14 @@ import org.apache.dolphinscheduler.plugin.registry.jdbc.client.IJdbcRegistryClient; import org.apache.dolphinscheduler.plugin.registry.jdbc.client.JdbcRegistryClientIdentify; import org.apache.dolphinscheduler.plugin.registry.jdbc.model.DTO.JdbcRegistryClientHeartbeatDTO; +import org.apache.dolphinscheduler.plugin.registry.jdbc.model.DTO.JdbcRegistryLockDTO; import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryClientRepository; import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryDataChangeEventRepository; import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryDataRepository; import org.apache.dolphinscheduler.plugin.registry.jdbc.repository.JdbcRegistryLockRepository; import java.time.Duration; +import java.util.Collections; import java.util.Map; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; @@ -445,6 +447,31 @@ void refreshClientsHeartbeat_shouldNotRefreshAfterDisconnected() { Mockito.verify(jdbcRegistryClientRepository, Mockito.never()).updateById(Mockito.any()); } + @Test + void purgeInvalidJdbcRegistryMetadata_shouldKeepMetadataWhenHeartbeatWasUpdatedAfterSnapshot() { + JdbcRegistryClientHeartbeatDTO staleHeartbeat = JdbcRegistryClientHeartbeatDTO.builder() + .id(CLIENT_IDENTIFY.getClientId()) + .clientName(CLIENT_IDENTIFY.getClientName()) + .lastHeartbeatTime(System.currentTimeMillis() - Duration.ofSeconds(2).toMillis()) + .clientConfig(new JdbcRegistryClientHeartbeatDTO.ClientConfig(Duration.ofSeconds(1).toMillis())) + .build(); + JdbcRegistryLockDTO clientLock = JdbcRegistryLockDTO.builder() + .id(1L) + .clientId(CLIENT_IDENTIFY.getClientId()) + .build(); + Mockito.when(jdbcRegistryClientRepository.queryAll()).thenReturn(Collections.singletonList(staleHeartbeat)); + Mockito.when(jdbcRegistryClientRepository.deleteByIdAndLastHeartbeatTime( + staleHeartbeat.getId(), staleHeartbeat.getLastHeartbeatTime())).thenReturn(false); + Mockito.when(jdbcRegistryDataRepository.selectAll()).thenReturn(Collections.emptyList()); + Mockito.when(jdbcRegistryLockRepository.queryAll()).thenReturn(Collections.singletonList(clientLock)); + + ReflectionTestUtils.invokeMethod(jdbcRegistryServer, "purgeInvalidJdbcRegistryMetadata"); + + Mockito.verify(jdbcRegistryClientRepository).deleteByIdAndLastHeartbeatTime( + staleHeartbeat.getId(), staleHeartbeat.getLastHeartbeatTime()); + Mockito.verify(jdbcRegistryLockRepository, Mockito.never()).deleteById(clientLock.getId()); + } + @SuppressWarnings("unchecked") private void setServerState(JdbcRegistryServerState state) { ((AtomicReference) ReflectionTestUtils.getField(jdbcRegistryServer, "serverState"))