From b236969d00c22ab69fe5e77b159ef6428a4cf84d Mon Sep 17 00:00:00 2001 From: Sergey Matvienko Date: Wed, 12 Jul 2023 12:41:59 +0200 Subject: [PATCH 01/10] TbWebSocketHandler.SessionMetaData removed synchronization to avoid deadlock with WsSession that already have its own lock --- .../server/controller/plugin/TbWebSocketHandler.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/application/src/main/java/org/thingsboard/server/controller/plugin/TbWebSocketHandler.java b/application/src/main/java/org/thingsboard/server/controller/plugin/TbWebSocketHandler.java index 7a396ff795..9cb343dbe3 100644 --- a/application/src/main/java/org/thingsboard/server/controller/plugin/TbWebSocketHandler.java +++ b/application/src/main/java/org/thingsboard/server/controller/plugin/TbWebSocketHandler.java @@ -239,7 +239,7 @@ public class TbWebSocketHandler extends TextWebSocketHandler implements WebSocke this.lastActivityTime = System.currentTimeMillis(); } - synchronized void sendPing(long currentTime) { + void sendPing(long currentTime) { try { long timeSinceLastActivity = currentTime - lastActivityTime; if (timeSinceLastActivity >= pingTimeout) { @@ -262,15 +262,15 @@ public class TbWebSocketHandler extends TextWebSocketHandler implements WebSocke } } - synchronized void processPongMessage(long currentTime) { + void processPongMessage(long currentTime) { lastActivityTime = currentTime; } - synchronized void sendMsg(String msg) { + void sendMsg(String msg) { sendMsg(new TbWebSocketTextMsg(msg)); } - synchronized void sendMsg(TbWebSocketMsg msg) { + void sendMsg(TbWebSocketMsg msg) { if (isSending.compareAndSet(false, true)) { sendMsgInternal(msg); } else { From 0bff83995191f893dca47c04a192f3cfe7723bb2 Mon Sep 17 00:00:00 2001 From: Sergey Matvienko Date: Wed, 12 Jul 2023 13:04:02 +0200 Subject: [PATCH 02/10] TbWebSocketHandler.checkLimits refactored to extract IO session.close() call from synchronized scope --- .../controller/plugin/TbWebSocketHandler.java | 34 ++++++++++++------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/application/src/main/java/org/thingsboard/server/controller/plugin/TbWebSocketHandler.java b/application/src/main/java/org/thingsboard/server/controller/plugin/TbWebSocketHandler.java index 9cb343dbe3..56d88143a5 100644 --- a/application/src/main/java/org/thingsboard/server/controller/plugin/TbWebSocketHandler.java +++ b/application/src/main/java/org/thingsboard/server/controller/plugin/TbWebSocketHandler.java @@ -397,19 +397,21 @@ public class TbWebSocketHandler extends TextWebSocketHandler implements WebSocke if (tenantProfileConfiguration == null) { return true; } - + boolean limitAllowed; String sessionId = session.getId(); if (tenantProfileConfiguration.getMaxWsSessionsPerTenant() > 0) { Set tenantSessions = tenantSessionsMap.computeIfAbsent(sessionRef.getSecurityCtx().getTenantId(), id -> ConcurrentHashMap.newKeySet()); synchronized (tenantSessions) { - if (tenantSessions.size() < tenantProfileConfiguration.getMaxWsSessionsPerTenant()) { + limitAllowed = tenantSessions.size() < tenantProfileConfiguration.getMaxWsSessionsPerTenant(); + if (limitAllowed) { tenantSessions.add(sessionId); - } else { + } + } + if (!limitAllowed) { log.info("[{}][{}][{}] Failed to start session. Max tenant sessions limit reached" , sessionRef.getSecurityCtx().getTenantId(), sessionRef.getSecurityCtx().getId(), sessionId); session.close(CloseStatus.POLICY_VIOLATION.withReason("Max tenant sessions limit reached!")); return false; - } } } @@ -417,42 +419,48 @@ public class TbWebSocketHandler extends TextWebSocketHandler implements WebSocke if (tenantProfileConfiguration.getMaxWsSessionsPerCustomer() > 0) { Set customerSessions = customerSessionsMap.computeIfAbsent(sessionRef.getSecurityCtx().getCustomerId(), id -> ConcurrentHashMap.newKeySet()); synchronized (customerSessions) { - if (customerSessions.size() < tenantProfileConfiguration.getMaxWsSessionsPerCustomer()) { + limitAllowed = customerSessions.size() < tenantProfileConfiguration.getMaxWsSessionsPerCustomer(); + if (limitAllowed) { customerSessions.add(sessionId); - } else { + } + } + if (!limitAllowed) { log.info("[{}][{}][{}] Failed to start session. Max customer sessions limit reached" , sessionRef.getSecurityCtx().getTenantId(), sessionRef.getSecurityCtx().getId(), sessionId); session.close(CloseStatus.POLICY_VIOLATION.withReason("Max customer sessions limit reached")); return false; - } } } if (tenantProfileConfiguration.getMaxWsSessionsPerRegularUser() > 0 && UserPrincipal.Type.USER_NAME.equals(sessionRef.getSecurityCtx().getUserPrincipal().getType())) { Set regularUserSessions = regularUserSessionsMap.computeIfAbsent(sessionRef.getSecurityCtx().getId(), id -> ConcurrentHashMap.newKeySet()); synchronized (regularUserSessions) { - if (regularUserSessions.size() < tenantProfileConfiguration.getMaxWsSessionsPerRegularUser()) { + limitAllowed = regularUserSessions.size() < tenantProfileConfiguration.getMaxWsSessionsPerRegularUser(); + if (limitAllowed) { regularUserSessions.add(sessionId); - } else { + } + } + if (!limitAllowed) { log.info("[{}][{}][{}] Failed to start session. Max regular user sessions limit reached" , sessionRef.getSecurityCtx().getTenantId(), sessionRef.getSecurityCtx().getId(), sessionId); session.close(CloseStatus.POLICY_VIOLATION.withReason("Max regular user sessions limit reached")); return false; - } } } if (tenantProfileConfiguration.getMaxWsSessionsPerPublicUser() > 0 && UserPrincipal.Type.PUBLIC_ID.equals(sessionRef.getSecurityCtx().getUserPrincipal().getType())) { Set publicUserSessions = publicUserSessionsMap.computeIfAbsent(sessionRef.getSecurityCtx().getId(), id -> ConcurrentHashMap.newKeySet()); synchronized (publicUserSessions) { - if (publicUserSessions.size() < tenantProfileConfiguration.getMaxWsSessionsPerPublicUser()) { + limitAllowed = publicUserSessions.size() < tenantProfileConfiguration.getMaxWsSessionsPerPublicUser(); + if (limitAllowed) { publicUserSessions.add(sessionId); - } else { + } + } + if (!limitAllowed) { log.info("[{}][{}][{}] Failed to start session. Max public user sessions limit reached" , sessionRef.getSecurityCtx().getTenantId(), sessionRef.getSecurityCtx().getId(), sessionId); session.close(CloseStatus.POLICY_VIOLATION.withReason("Max public user sessions limit reached")); return false; - } } } } From 5eebbf89859ee08ee6c481646c060495f51086ce Mon Sep 17 00:00:00 2001 From: Sergey Matvienko Date: Thu, 13 Jul 2023 22:28:59 +0200 Subject: [PATCH 03/10] web socket handler tests added. ws msg queue fixed the last msg pickup (and msg order as result) --- .../controller/plugin/TbWebSocketHandler.java | 44 +++-- .../plugin/TbWebSocketHandlerTest.java | 160 ++++++++++++++++++ 2 files changed, 186 insertions(+), 18 deletions(-) create mode 100644 application/src/test/java/org/thingsboard/server/controller/plugin/TbWebSocketHandlerTest.java diff --git a/application/src/main/java/org/thingsboard/server/controller/plugin/TbWebSocketHandler.java b/application/src/main/java/org/thingsboard/server/controller/plugin/TbWebSocketHandler.java index 56d88143a5..481d412d59 100644 --- a/application/src/main/java/org/thingsboard/server/controller/plugin/TbWebSocketHandler.java +++ b/application/src/main/java/org/thingsboard/server/controller/plugin/TbWebSocketHandler.java @@ -219,12 +219,12 @@ public class TbWebSocketHandler extends TextWebSocketHandler implements WebSocke .build(); } - private class SessionMetaData implements SendHandler { + class SessionMetaData implements SendHandler { private final WebSocketSession session; private final RemoteEndpoint.Async asyncRemote; private final WebSocketSessionRef sessionRef; - private final AtomicBoolean isSending = new AtomicBoolean(false); + final AtomicBoolean isSending = new AtomicBoolean(false); private final Queue> msgQueue; private volatile long lastActivityTime; @@ -254,11 +254,13 @@ public class TbWebSocketHandler extends TextWebSocketHandler implements WebSocke } } - private void closeSession(CloseStatus reason) { + void closeSession(CloseStatus reason) { try { close(this.sessionRef, reason); } catch (IOException ioe) { log.trace("[{}] Session transport error", session.getId(), ioe); + } finally { + msgQueue.clear(); } } @@ -271,20 +273,19 @@ public class TbWebSocketHandler extends TextWebSocketHandler implements WebSocke } void sendMsg(TbWebSocketMsg msg) { - if (isSending.compareAndSet(false, true)) { - sendMsgInternal(msg); - } else { - try { - msgQueue.add(msg); - } catch (RuntimeException e) { - if (log.isTraceEnabled()) { - log.trace("[{}][{}] Session closed due to queue error", sessionRef.getSecurityCtx().getTenantId(), session.getId(), e); - } else { - log.info("[{}][{}] Session closed due to queue error", sessionRef.getSecurityCtx().getTenantId(), session.getId()); - } - closeSession(CloseStatus.POLICY_VIOLATION.withReason("Max pending updates limit reached!")); + try { + msgQueue.add(msg); + } catch (RuntimeException e) { + if (log.isTraceEnabled()) { + log.trace("[{}][{}] Session closed due to queue error", sessionRef.getSecurityCtx().getTenantId(), session.getId(), e); + } else { + log.info("[{}][{}] Session closed due to queue error", sessionRef.getSecurityCtx().getTenantId(), session.getId()); } + closeSession(CloseStatus.POLICY_VIOLATION.withReason("Max pending updates limit reached!")); + return; } + + processNextMsg(); } private void sendMsgInternal(TbWebSocketMsg msg) { @@ -292,9 +293,11 @@ public class TbWebSocketHandler extends TextWebSocketHandler implements WebSocke if (TbWebSocketMsgType.TEXT.equals(msg.getType())) { TbWebSocketTextMsg textMsg = (TbWebSocketTextMsg) msg; this.asyncRemote.sendText(textMsg.getMsg(), this); + // isSending status will be reset in the onResult method by call back } else { TbWebSocketPingMsg pingMsg = (TbWebSocketPingMsg) msg; - this.asyncRemote.sendPing(pingMsg.getMsg()); + this.asyncRemote.sendPing(pingMsg.getMsg()); // blocking call + isSending.set(false); processNextMsg(); } } catch (Exception e) { @@ -308,12 +311,17 @@ public class TbWebSocketHandler extends TextWebSocketHandler implements WebSocke if (!result.isOK()) { log.trace("[{}] Failed to send msg", session.getId(), result.getException()); closeSession(CloseStatus.SESSION_NOT_RELIABLE); - } else { - processNextMsg(); + return; } + + isSending.set(false); + processNextMsg(); } private void processNextMsg() { + if (msgQueue.isEmpty() || !isSending.compareAndSet(false, true)) { + return; + } TbWebSocketMsg msg = msgQueue.poll(); if (msg != null) { sendMsgInternal(msg); diff --git a/application/src/test/java/org/thingsboard/server/controller/plugin/TbWebSocketHandlerTest.java b/application/src/test/java/org/thingsboard/server/controller/plugin/TbWebSocketHandlerTest.java new file mode 100644 index 0000000000..0394e8a505 --- /dev/null +++ b/application/src/test/java/org/thingsboard/server/controller/plugin/TbWebSocketHandlerTest.java @@ -0,0 +1,160 @@ +/** + * Copyright © 2016-2023 The Thingsboard Authors + * + * Licensed 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.thingsboard.server.controller.plugin; + +import lombok.extern.slf4j.Slf4j; +import org.awaitility.Awaitility; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; +import org.springframework.web.socket.CloseStatus; +import org.springframework.web.socket.adapter.NativeWebSocketSession; +import org.thingsboard.common.util.ThingsBoardThreadFactory; +import org.thingsboard.server.service.ws.WebSocketSessionRef; + +import javax.websocket.RemoteEndpoint; +import javax.websocket.SendHandler; +import javax.websocket.SendResult; +import javax.websocket.Session; +import java.io.IOException; +import java.util.Collection; +import java.util.List; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; +import java.util.stream.IntStream; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.BDDMockito.willAnswer; +import static org.mockito.BDDMockito.willDoNothing; +import static org.mockito.BDDMockito.willReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +@Slf4j +class TbWebSocketHandlerTest { + + TbWebSocketHandler wsHandler; + NativeWebSocketSession session; + Session nativeSession; + RemoteEndpoint.Async asyncRemote; + WebSocketSessionRef sessionRef; + int maxMsgQueuePerSession; + TbWebSocketHandler.SessionMetaData sendHandler; + ExecutorService executor; + + @BeforeEach + void setUp() throws IOException { + maxMsgQueuePerSession = 100; + executor = Executors.newCachedThreadPool(ThingsBoardThreadFactory.forName(getClass().getSimpleName())); + wsHandler = spy(new TbWebSocketHandler()); + willDoNothing().given(wsHandler).close(any(), any()); + session = mock(NativeWebSocketSession.class); + nativeSession = mock(Session.class); + willReturn(nativeSession).given(session).getNativeSession(Session.class); + asyncRemote = mock(RemoteEndpoint.Async.class); + willReturn(asyncRemote).given(nativeSession).getAsyncRemote(); + sessionRef = mock(WebSocketSessionRef.class, Mockito.RETURNS_DEEP_STUBS); //prevent NPE on logs + sendHandler = spy(wsHandler.new SessionMetaData(session, sessionRef, maxMsgQueuePerSession)); + } + + @AfterEach + void tearDown() { + if (executor != null) { + executor.shutdownNow(); + } + } + + @Test + void sendHandler_sendMsg_parallel_no_race() throws InterruptedException { + CountDownLatch finishLatch = new CountDownLatch(maxMsgQueuePerSession * 2); + AtomicInteger sendersCount = new AtomicInteger(); + willAnswer(invocation -> { + assertThat(sendersCount.incrementAndGet()).as("no race").isEqualTo(1); + String text = invocation.getArgument(0); + SendHandler onResultHandler = invocation.getArgument(1); + SendResult sendResult = new SendResult(); + executor.submit(() -> { + sendersCount.decrementAndGet(); + onResultHandler.onResult(sendResult); + finishLatch.countDown(); + }); + return null; + }).given(asyncRemote).sendText(anyString(), any()); + + assertThat(sendHandler.isSending.get()).as("sendHandler not is in sending state").isFalse(); + //first batch + IntStream.range(0, maxMsgQueuePerSession).parallel().forEach(i -> sendHandler.sendMsg("hello " + i)); + Awaitility.await("first batch processed").atMost(30, TimeUnit.SECONDS).until(() -> finishLatch.getCount() == maxMsgQueuePerSession); + assertThat(sendHandler.isSending.get()).as("sendHandler not is in sending state").isFalse(); + //second batch - to test pause between big msg batches + IntStream.range(100, 100 + maxMsgQueuePerSession).parallel().forEach(i -> sendHandler.sendMsg("hello " + i)); + assertThat(finishLatch.await(30, TimeUnit.SECONDS)).as("all callbacks fired").isTrue(); + + verify(sendHandler, never()).closeSession(any()); + verify(sendHandler, times(maxMsgQueuePerSession * 2)).onResult(any()); + assertThat(sendHandler.isSending.get()).as("sendHandler not is in sending state").isFalse(); + } + + @Test + void sendHandler_sendMsg_message_order() throws InterruptedException { + CountDownLatch finishLatch = new CountDownLatch(maxMsgQueuePerSession); + Collection outputs = new ConcurrentLinkedQueue<>(); + willAnswer(invocation -> { + String text = invocation.getArgument(0); + outputs.add(text); + SendHandler onResultHandler = invocation.getArgument(1); + SendResult sendResult = new SendResult(); + executor.submit(() -> { + onResultHandler.onResult(sendResult); + finishLatch.countDown(); + }); + return null; + }).given(asyncRemote).sendText(anyString(), any()); + + List inputs = IntStream.range(0, maxMsgQueuePerSession).mapToObj(i -> "msg " + i).collect(Collectors.toList()); + inputs.forEach(s -> sendHandler.sendMsg(s)); + + assertThat(finishLatch.await(30, TimeUnit.SECONDS)).as("all callbacks fired").isTrue(); + assertThat(outputs).as("inputs exactly the same as outputs").containsExactlyElementsOf(inputs); + + verify(sendHandler, never()).closeSession(any()); + verify(sendHandler, times(maxMsgQueuePerSession)).onResult(any()); + } + + @Test + void sendHandler_sendMsg_queue_size_exceed() { + willDoNothing().given(asyncRemote).sendText(anyString(), any()); // send text will never call back, so queue will grow each sendMsg + sendHandler.sendMsg("first message to stay in-flight all the time during this test"); + IntStream.range(0, maxMsgQueuePerSession).parallel().forEach(i -> sendHandler.sendMsg("hello " + i)); + verify(sendHandler, never()).closeSession(any()); + sendHandler.sendMsg("excessive message"); + verify(sendHandler, times(1)).closeSession(eq(new CloseStatus(1008, "Max pending updates limit reached!"))); + verify(asyncRemote, times(1)).sendText(anyString(), any()); + } + +} From 3b8a9d94ecfffeb3813bcc75d203c568be4ab567 Mon Sep 17 00:00:00 2001 From: Sergey Matvienko Date: Mon, 24 Jul 2023 22:57:52 +0200 Subject: [PATCH 04/10] Lwm2m transport - merge non-unique endpoints for models fetched from cache --- .../model/LwM2MModelConfigServiceImpl.java | 8 +- .../LwM2MModelConfigServiceImplTest.java | 73 +++++++++++++++++++ 2 files changed, 77 insertions(+), 4 deletions(-) create mode 100644 common/transport/lwm2m/src/test/java/org/thingsboard/server/transport/lwm2m/server/model/LwM2MModelConfigServiceImplTest.java diff --git a/common/transport/lwm2m/src/main/java/org/thingsboard/server/transport/lwm2m/server/model/LwM2MModelConfigServiceImpl.java b/common/transport/lwm2m/src/main/java/org/thingsboard/server/transport/lwm2m/server/model/LwM2MModelConfigServiceImpl.java index 302bd20c8b..eef9a53024 100644 --- a/common/transport/lwm2m/src/main/java/org/thingsboard/server/transport/lwm2m/server/model/LwM2MModelConfigServiceImpl.java +++ b/common/transport/lwm2m/src/main/java/org/thingsboard/server/transport/lwm2m/server/model/LwM2MModelConfigServiceImpl.java @@ -52,7 +52,7 @@ import java.util.stream.Collectors; public class LwM2MModelConfigServiceImpl implements LwM2MModelConfigService { @Autowired - private TbLwM2MModelConfigStore modelStore; + TbLwM2MModelConfigStore modelStore; @Autowired @Lazy @@ -67,14 +67,14 @@ public class LwM2MModelConfigServiceImpl implements LwM2MModelConfigService { @Autowired private LwM2MTelemetryLogService logService; - private ConcurrentMap currentModelConfigs; + ConcurrentMap currentModelConfigs; @AfterStartUp(order = AfterStartUp.BEFORE_TRANSPORT_SERVICE) - private void init() { + public void init() { List models = modelStore.getAll(); log.debug("Fetched model configs: {}", models); currentModelConfigs = models.stream() - .collect(Collectors.toConcurrentMap(LwM2MModelConfig::getEndpoint, m -> m)); + .collect(Collectors.toConcurrentMap(LwM2MModelConfig::getEndpoint, m -> m, (existing, replacement) -> existing)); } @Override diff --git a/common/transport/lwm2m/src/test/java/org/thingsboard/server/transport/lwm2m/server/model/LwM2MModelConfigServiceImplTest.java b/common/transport/lwm2m/src/test/java/org/thingsboard/server/transport/lwm2m/server/model/LwM2MModelConfigServiceImplTest.java new file mode 100644 index 0000000000..fc54ca9e0b --- /dev/null +++ b/common/transport/lwm2m/src/test/java/org/thingsboard/server/transport/lwm2m/server/model/LwM2MModelConfigServiceImplTest.java @@ -0,0 +1,73 @@ +/** + * Copyright © 2016-2023 The Thingsboard Authors + * + * Licensed 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.thingsboard.server.transport.lwm2m.server.model; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.thingsboard.server.transport.lwm2m.server.store.TbLwM2MModelConfigStore; + +import java.util.Collections; +import java.util.List; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.BDDMockito.willReturn; +import static org.mockito.Mockito.mock; + +class LwM2MModelConfigServiceImplTest { + + LwM2MModelConfigServiceImpl service; + TbLwM2MModelConfigStore modelStore; + + @BeforeEach + void setUp() { + service = new LwM2MModelConfigServiceImpl(); + modelStore = mock(TbLwM2MModelConfigStore.class); + service.modelStore = modelStore; + } + + @Test + void testInitWithDuplicatedModels() { + LwM2MModelConfig config = new LwM2MModelConfig("urn:imei:951358811362976"); + List models = List.of(config, config); + willReturn(models).given(modelStore).getAll(); + service.init(); + assertThat(service.currentModelConfigs).containsExactlyEntriesOf(Map.of(config.getEndpoint(), config)); + } + + @Test + void testInitWithNonUniqueEndpoints() { + LwM2MModelConfig configAlfa = new LwM2MModelConfig("urn:imei:951358811362976"); + LwM2MModelConfig configBravo = new LwM2MModelConfig("urn:imei:151358811362976"); + LwM2MModelConfig configDelta = new LwM2MModelConfig("urn:imei:151358811362976"); + assertThat(configBravo.getEndpoint()).as("non-unique endpoints provided").isEqualTo(configDelta.getEndpoint()); + List models = List.of(configAlfa, configBravo, configDelta); + willReturn(models).given(modelStore).getAll(); + service.init(); + assertThat(service.currentModelConfigs).containsExactlyInAnyOrderEntriesOf(Map.of( + configAlfa.getEndpoint(), configAlfa, + configBravo.getEndpoint(), configBravo + )); + } + + @Test + void testInitWithEmptyModels() { + willReturn(Collections.emptyList()).given(modelStore).getAll(); + service.init(); + assertThat(service.currentModelConfigs).isEmpty(); + } + +} From 161d7c3eda40464ec2c318f89c54f18741e78081 Mon Sep 17 00:00:00 2001 From: Sergey Matvienko Date: Mon, 31 Jul 2023 11:59:31 +0200 Subject: [PATCH 05/10] Cassandra: unlimited cached thread pool replaced with limited pool to prevent stack memory peaks. New env CASSANDRA_QUERY_RESULT_PROCESSING_THREADS introduced. --- application/src/main/resources/thingsboard.yml | 3 ++- .../server/dao/nosql/CassandraAbstractAsyncDao.java | 9 ++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/application/src/main/resources/thingsboard.yml b/application/src/main/resources/thingsboard.yml index 589540096c..3666678561 100644 --- a/application/src/main/resources/thingsboard.yml +++ b/application/src/main/resources/thingsboard.yml @@ -245,7 +245,8 @@ cassandra: concurrent_limit: "${CASSANDRA_QUERY_CONCURRENT_LIMIT:1000}" permit_max_wait_time: "${PERMIT_MAX_WAIT_TIME:120000}" dispatcher_threads: "${CASSANDRA_QUERY_DISPATCHER_THREADS:2}" - callback_threads: "${CASSANDRA_QUERY_CALLBACK_THREADS:4}" + callback_threads: "${CASSANDRA_QUERY_CALLBACK_THREADS:4}" # Buffered rate executor (read, write), for managing I/O rate. See "nosql-*-callback" threads in JMX + result_processing_threads: "${CASSANDRA_QUERY_RESULT_PROCESSING_THREADS:50}" # Result set transformer and processing. See "cassandra-callback" threads in JMX poll_ms: "${CASSANDRA_QUERY_POLL_MS:50}" rate_limit_print_interval_ms: "${CASSANDRA_QUERY_RATE_LIMIT_PRINT_MS:10000}" # set all data types values except target to null for the same ts on save diff --git a/dao/src/main/java/org/thingsboard/server/dao/nosql/CassandraAbstractAsyncDao.java b/dao/src/main/java/org/thingsboard/server/dao/nosql/CassandraAbstractAsyncDao.java index 425724bf2c..92289b7178 100644 --- a/dao/src/main/java/org/thingsboard/server/dao/nosql/CassandraAbstractAsyncDao.java +++ b/dao/src/main/java/org/thingsboard/server/dao/nosql/CassandraAbstractAsyncDao.java @@ -19,13 +19,13 @@ import com.google.common.base.Function; import com.google.common.util.concurrent.AsyncFunction; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; -import org.thingsboard.common.util.ThingsBoardThreadFactory; +import org.springframework.beans.factory.annotation.Value; +import org.thingsboard.common.util.ThingsBoardExecutors; import javax.annotation.Nullable; import javax.annotation.PostConstruct; import javax.annotation.PreDestroy; import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; /** * Created by ashvayka on 21.02.17. @@ -34,9 +34,12 @@ public abstract class CassandraAbstractAsyncDao extends CassandraAbstractDao { protected ExecutorService readResultsProcessingExecutor; + @Value("${cassandra.query.result_processing_threads:50}") + private int threadPoolSize; + @PostConstruct public void startExecutor() { - readResultsProcessingExecutor = Executors.newCachedThreadPool(ThingsBoardThreadFactory.forName("cassandra-callback")); + readResultsProcessingExecutor = ThingsBoardExecutors.newWorkStealingPool(threadPoolSize, "cassandra-callback"); } @PreDestroy From 9e92d10dc544b0ecac01f615b8861a295e0929f4 Mon Sep 17 00:00:00 2001 From: Sergey Matvienko Date: Mon, 31 Jul 2023 12:05:23 +0200 Subject: [PATCH 06/10] Cassandra: AbstractBufferedRateExecutor improved logging to clarify whether the Read or Write operations logged --- .../dao/util/AbstractBufferedRateExecutor.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/dao/src/main/java/org/thingsboard/server/dao/util/AbstractBufferedRateExecutor.java b/dao/src/main/java/org/thingsboard/server/dao/util/AbstractBufferedRateExecutor.java index 9fbf796eb0..daade9acf3 100644 --- a/dao/src/main/java/org/thingsboard/server/dao/util/AbstractBufferedRateExecutor.java +++ b/dao/src/main/java/org/thingsboard/server/dao/util/AbstractBufferedRateExecutor.java @@ -124,7 +124,7 @@ public abstract class AbstractBufferedRateExecutor new TbRateLimits(tenantProfileConfiguration.getCassandraQueryTenantRateLimitsConfiguration()) @@ -173,7 +173,7 @@ public abstract class AbstractBufferedRateExecutor taskCtx = null; @@ -185,7 +185,7 @@ public abstract class AbstractBufferedRateExecutor= printQueriesFreq) { printQueriesIdx.set(0); String query = queryToString(finalTaskCtx); - log.info("[{}] Cassandra query: {}", taskCtx.getId(), query); + log.info("[{}][{}] Cassandra query: {}", getBufferName(), taskCtx.getId(), query); } } logTask("Processing", finalTaskCtx); @@ -238,7 +238,7 @@ public abstract class AbstractBufferedRateExecutor taskCtx) { @@ -314,7 +314,7 @@ public abstract class AbstractBufferedRateExecutor Date: Fri, 16 Jun 2023 15:40:29 +0200 Subject: [PATCH 07/10] added recalculetePartitions delay for node restart --- .../src/main/resources/thingsboard.yml | 1 + .../queue/discovery/ZkDiscoveryService.java | 31 ++++++++++++++++++- .../src/main/resources/tb-vc-executor.yml | 1 + .../src/main/resources/tb-coap-transport.yml | 1 + .../src/main/resources/tb-http-transport.yml | 1 + .../src/main/resources/tb-lwm2m-transport.yml | 1 + .../src/main/resources/tb-mqtt-transport.yml | 1 + .../src/main/resources/tb-snmp-transport.yml | 1 + 8 files changed, 37 insertions(+), 1 deletion(-) diff --git a/application/src/main/resources/thingsboard.yml b/application/src/main/resources/thingsboard.yml index 3666678561..19804c0588 100644 --- a/application/src/main/resources/thingsboard.yml +++ b/application/src/main/resources/thingsboard.yml @@ -96,6 +96,7 @@ zk: session_timeout_ms: "${ZOOKEEPER_SESSION_TIMEOUT_MS:3000}" # Name of the directory in zookeeper 'filesystem' zk_dir: "${ZOOKEEPER_NODES_DIR:/thingsboard}" + recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:120000}" cluster: stats: diff --git a/common/queue/src/main/java/org/thingsboard/server/queue/discovery/ZkDiscoveryService.java b/common/queue/src/main/java/org/thingsboard/server/queue/discovery/ZkDiscoveryService.java index fcf80bcf3d..17d046a4cb 100644 --- a/common/queue/src/main/java/org/thingsboard/server/queue/discovery/ZkDiscoveryService.java +++ b/common/queue/src/main/java/org/thingsboard/server/queue/discovery/ZkDiscoveryService.java @@ -44,8 +44,10 @@ import javax.annotation.PostConstruct; import javax.annotation.PreDestroy; import java.util.List; import java.util.NoSuchElementException; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; @@ -66,6 +68,10 @@ public class ZkDiscoveryService implements DiscoveryService, PathChildrenCacheLi private Integer zkSessionTimeout; @Value("${zk.zk_dir}") private String zkDir; + @Value("${zk.recalculate_delay:120000}") + private Long recalculateDelay; + + private final ConcurrentHashMap> delayedTasks; private final TbServiceInfoProvider serviceInfoProvider; private final PartitionService partitionService; @@ -82,6 +88,7 @@ public class ZkDiscoveryService implements DiscoveryService, PathChildrenCacheLi PartitionService partitionService) { this.serviceInfoProvider = serviceInfoProvider; this.partitionService = partitionService; + delayedTasks = new ConcurrentHashMap<>(); } @PostConstruct @@ -290,8 +297,30 @@ public class ZkDiscoveryService implements DiscoveryService, PathChildrenCacheLi log.debug("Processing [{}] event for [{}]", pathChildrenCacheEvent.getType(), instance.getServiceId()); switch (pathChildrenCacheEvent.getType()) { case CHILD_ADDED: + ScheduledFuture task = delayedTasks.remove(instance.getServiceId()); + if (task != null) { + if (!task.cancel(false)) { + log.debug("[{}] Going to recalculate partitions due to adding new node [{}]", + instance.getServiceId(), instance.getServiceTypesList()); + recalculatePartitions(); + } else { + log.debug("[{}] Recalculate partitions ignored. Service restarted in time [{}]", + instance.getServiceId(), instance.getServiceTypesList()); + } + } else { + log.debug("[{}] Going to recalculate partitions due to adding new node [{}]", + instance.getServiceId(), instance.getServiceTypesList()); + recalculatePartitions(); + } + break; case CHILD_REMOVED: - recalculatePartitions(); + ScheduledFuture future = zkExecutorService.schedule(() -> { + log.debug("[{}] Going to recalculate partitions due to removed node [{}]", + instance.getServiceId(), instance.getServiceTypesList()); + delayedTasks.remove(instance.getServiceId()); + recalculatePartitions(); + }, recalculateDelay, TimeUnit.MILLISECONDS); + delayedTasks.put(instance.getServiceId(), future); break; default: break; diff --git a/msa/vc-executor/src/main/resources/tb-vc-executor.yml b/msa/vc-executor/src/main/resources/tb-vc-executor.yml index 352f94e091..0dbb19a71a 100644 --- a/msa/vc-executor/src/main/resources/tb-vc-executor.yml +++ b/msa/vc-executor/src/main/resources/tb-vc-executor.yml @@ -41,6 +41,7 @@ zk: session_timeout_ms: "${ZOOKEEPER_SESSION_TIMEOUT_MS:3000}" # Name of the directory in zookeeper 'filesystem' zk_dir: "${ZOOKEEPER_NODES_DIR:/thingsboard}" + recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:120000}" queue: type: "${TB_QUEUE_TYPE:kafka}" # in-memory or kafka (Apache Kafka) or aws-sqs (AWS SQS) or pubsub (PubSub) or service-bus (Azure Service Bus) or rabbitmq (RabbitMQ) diff --git a/transport/coap/src/main/resources/tb-coap-transport.yml b/transport/coap/src/main/resources/tb-coap-transport.yml index 7ea553fe5c..c8f4b5a099 100644 --- a/transport/coap/src/main/resources/tb-coap-transport.yml +++ b/transport/coap/src/main/resources/tb-coap-transport.yml @@ -41,6 +41,7 @@ zk: session_timeout_ms: "${ZOOKEEPER_SESSION_TIMEOUT_MS:3000}" # Name of the directory in zookeeper 'filesystem' zk_dir: "${ZOOKEEPER_NODES_DIR:/thingsboard}" + recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:120000}" cache: type: "${CACHE_TYPE:redis}" diff --git a/transport/http/src/main/resources/tb-http-transport.yml b/transport/http/src/main/resources/tb-http-transport.yml index 346ec48eae..fe181f12f2 100644 --- a/transport/http/src/main/resources/tb-http-transport.yml +++ b/transport/http/src/main/resources/tb-http-transport.yml @@ -68,6 +68,7 @@ zk: session_timeout_ms: "${ZOOKEEPER_SESSION_TIMEOUT_MS:3000}" # Name of the directory in zookeeper 'filesystem' zk_dir: "${ZOOKEEPER_NODES_DIR:/thingsboard}" + recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:120000}" cache: type: "${CACHE_TYPE:redis}" diff --git a/transport/lwm2m/src/main/resources/tb-lwm2m-transport.yml b/transport/lwm2m/src/main/resources/tb-lwm2m-transport.yml index 4e8167d89d..d80279f582 100644 --- a/transport/lwm2m/src/main/resources/tb-lwm2m-transport.yml +++ b/transport/lwm2m/src/main/resources/tb-lwm2m-transport.yml @@ -41,6 +41,7 @@ zk: session_timeout_ms: "${ZOOKEEPER_SESSION_TIMEOUT_MS:3000}" # Name of the directory in zookeeper 'filesystem' zk_dir: "${ZOOKEEPER_NODES_DIR:/thingsboard}" + recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:120000}" cache: type: "${CACHE_TYPE:redis}" diff --git a/transport/mqtt/src/main/resources/tb-mqtt-transport.yml b/transport/mqtt/src/main/resources/tb-mqtt-transport.yml index 1e0b1ebcd4..fcbf542287 100644 --- a/transport/mqtt/src/main/resources/tb-mqtt-transport.yml +++ b/transport/mqtt/src/main/resources/tb-mqtt-transport.yml @@ -41,6 +41,7 @@ zk: session_timeout_ms: "${ZOOKEEPER_SESSION_TIMEOUT_MS:3000}" # Name of the directory in zookeeper 'filesystem' zk_dir: "${ZOOKEEPER_NODES_DIR:/thingsboard}" + recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:120000}" cache: type: "${CACHE_TYPE:redis}" diff --git a/transport/snmp/src/main/resources/tb-snmp-transport.yml b/transport/snmp/src/main/resources/tb-snmp-transport.yml index 9f086bcbc5..0e84d54fce 100644 --- a/transport/snmp/src/main/resources/tb-snmp-transport.yml +++ b/transport/snmp/src/main/resources/tb-snmp-transport.yml @@ -41,6 +41,7 @@ zk: session_timeout_ms: "${ZOOKEEPER_SESSION_TIMEOUT_MS:3000}" # Name of the directory in zookeeper 'filesystem' zk_dir: "${ZOOKEEPER_NODES_DIR:/thingsboard}" + recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:120000}" cache: type: "${CACHE_TYPE:redis}" From ce9552e1a8ca44f58a369051bfc9f5bc24ca1477 Mon Sep 17 00:00:00 2001 From: YevhenBondarenko Date: Thu, 6 Jul 2023 13:31:25 +0200 Subject: [PATCH 08/10] improvements --- .../queue/discovery/ZkDiscoveryService.java | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/common/queue/src/main/java/org/thingsboard/server/queue/discovery/ZkDiscoveryService.java b/common/queue/src/main/java/org/thingsboard/server/queue/discovery/ZkDiscoveryService.java index 17d046a4cb..24a7863b24 100644 --- a/common/queue/src/main/java/org/thingsboard/server/queue/discovery/ZkDiscoveryService.java +++ b/common/queue/src/main/java/org/thingsboard/server/queue/discovery/ZkDiscoveryService.java @@ -299,16 +299,16 @@ public class ZkDiscoveryService implements DiscoveryService, PathChildrenCacheLi case CHILD_ADDED: ScheduledFuture task = delayedTasks.remove(instance.getServiceId()); if (task != null) { - if (!task.cancel(false)) { - log.debug("[{}] Going to recalculate partitions due to adding new node [{}]", + if (task.cancel(false)) { + log.debug("[{}] Recalculate partitions ignored. Service was restarted in time [{}].", instance.getServiceId(), instance.getServiceTypesList()); - recalculatePartitions(); } else { - log.debug("[{}] Recalculate partitions ignored. Service restarted in time [{}]", + log.debug("[{}] Going to recalculate partitions. Service was not restarted in time [{}]!", instance.getServiceId(), instance.getServiceTypesList()); + recalculatePartitions(); } } else { - log.debug("[{}] Going to recalculate partitions due to adding new node [{}]", + log.debug("[{}] Going to recalculate partitions due to adding new node [{}].", instance.getServiceId(), instance.getServiceTypesList()); recalculatePartitions(); } @@ -317,8 +317,10 @@ public class ZkDiscoveryService implements DiscoveryService, PathChildrenCacheLi ScheduledFuture future = zkExecutorService.schedule(() -> { log.debug("[{}] Going to recalculate partitions due to removed node [{}]", instance.getServiceId(), instance.getServiceTypesList()); - delayedTasks.remove(instance.getServiceId()); - recalculatePartitions(); + ScheduledFuture removedTask = delayedTasks.remove(instance.getServiceId()); + if (removedTask != null) { + recalculatePartitions(); + } }, recalculateDelay, TimeUnit.MILLISECONDS); delayedTasks.put(instance.getServiceId(), future); break; @@ -332,6 +334,7 @@ public class ZkDiscoveryService implements DiscoveryService, PathChildrenCacheLi * Synchronized to ensure that other servers info is up to date * */ synchronized void recalculatePartitions() { + delayedTasks.clear(); partitionService.recalculatePartitions(serviceInfoProvider.getServiceInfo(), getOtherServers()); } From 948f517898ff2207e6ba797e83ca2f77a3194790 Mon Sep 17 00:00:00 2001 From: YevhenBondarenko Date: Fri, 14 Jul 2023 19:45:23 +0200 Subject: [PATCH 09/10] added zk restart node tests --- .../queue/discovery/ZkDiscoveryService.java | 2 +- .../discovery/ZkDiscoveryServiceTest.java | 173 ++++++++++++++++++ 2 files changed, 174 insertions(+), 1 deletion(-) create mode 100644 common/queue/src/test/java/org/thingsboard/server/queue/discovery/ZkDiscoveryServiceTest.java diff --git a/common/queue/src/main/java/org/thingsboard/server/queue/discovery/ZkDiscoveryService.java b/common/queue/src/main/java/org/thingsboard/server/queue/discovery/ZkDiscoveryService.java index 24a7863b24..50378d3387 100644 --- a/common/queue/src/main/java/org/thingsboard/server/queue/discovery/ZkDiscoveryService.java +++ b/common/queue/src/main/java/org/thingsboard/server/queue/discovery/ZkDiscoveryService.java @@ -71,7 +71,7 @@ public class ZkDiscoveryService implements DiscoveryService, PathChildrenCacheLi @Value("${zk.recalculate_delay:120000}") private Long recalculateDelay; - private final ConcurrentHashMap> delayedTasks; + protected final ConcurrentHashMap> delayedTasks; private final TbServiceInfoProvider serviceInfoProvider; private final PartitionService partitionService; diff --git a/common/queue/src/test/java/org/thingsboard/server/queue/discovery/ZkDiscoveryServiceTest.java b/common/queue/src/test/java/org/thingsboard/server/queue/discovery/ZkDiscoveryServiceTest.java new file mode 100644 index 0000000000..38cad217aa --- /dev/null +++ b/common/queue/src/test/java/org/thingsboard/server/queue/discovery/ZkDiscoveryServiceTest.java @@ -0,0 +1,173 @@ +/** + * Copyright © 2016-2023 The Thingsboard Authors + * + * Licensed 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.thingsboard.server.queue.discovery; + +import org.apache.curator.framework.CuratorFramework; +import org.apache.curator.framework.imps.CuratorFrameworkState; +import org.apache.curator.framework.recipes.cache.ChildData; +import org.apache.curator.framework.recipes.cache.PathChildrenCache; +import org.apache.curator.framework.recipes.cache.PathChildrenCacheEvent; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; +import org.springframework.test.util.ReflectionTestUtils; +import org.thingsboard.common.util.ThingsBoardThreadFactory; +import org.thingsboard.server.gen.transport.TransportProtos; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; + +import static org.apache.curator.framework.recipes.cache.PathChildrenCacheEvent.Type.CHILD_ADDED; +import static org.apache.curator.framework.recipes.cache.PathChildrenCacheEvent.Type.CHILD_REMOVED; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.reset; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +@RunWith(MockitoJUnitRunner.class) +public class ZkDiscoveryServiceTest { + + @Mock + private TbServiceInfoProvider serviceInfoProvider; + + @Mock + private PartitionService partitionService; + + @Mock + private CuratorFramework client; + + @Mock + private PathChildrenCache cache; + + private ScheduledExecutorService zkExecutorService; + + @Mock + private CuratorFramework curatorFramework; + + private ZkDiscoveryService zkDiscoveryService; + + @Before + public void setup() { + zkDiscoveryService = Mockito.spy(new ZkDiscoveryService(serviceInfoProvider, partitionService)); + zkExecutorService = Executors.newSingleThreadScheduledExecutor(ThingsBoardThreadFactory.forName("zk-discovery")); + when(client.getState()).thenReturn(CuratorFrameworkState.STARTED); + ReflectionTestUtils.setField(zkDiscoveryService, "stopped", false); + ReflectionTestUtils.setField(zkDiscoveryService, "client", client); + ReflectionTestUtils.setField(zkDiscoveryService, "cache", cache); + ReflectionTestUtils.setField(zkDiscoveryService, "nodePath", "/thingsboard/nodes/0000000010"); + ReflectionTestUtils.setField(zkDiscoveryService, "zkExecutorService", zkExecutorService); + ReflectionTestUtils.setField(zkDiscoveryService, "recalculateDelay", 1000L); + ReflectionTestUtils.setField(zkDiscoveryService, "zkDir", "/thingsboard"); + } + + @Test + public void restartNodeTest() throws Exception { + var currentInfo = TransportProtos.ServiceInfo.newBuilder().setServiceId("currentId").build(); + var currentData = new ChildData("/thingsboard/nodes/0000000010", null, currentInfo.toByteArray()); + var childInfo = TransportProtos.ServiceInfo.newBuilder().setServiceId("childId").build(); + var childData = new ChildData("/thingsboard/nodes/0000000020", null, childInfo.toByteArray()); + + when(serviceInfoProvider.getServiceInfo()).thenReturn(currentInfo); + List dataList = new ArrayList<>(); + dataList.add(currentData); + when(cache.getCurrentData()).thenReturn(dataList); + + startNode(childData); + + verify(partitionService, times(1)).recalculatePartitions(eq(currentInfo), eq(List.of(childInfo))); + + reset(partitionService); + + //Restart in timeAssert.assertTrue(zkDiscoveryService.delayedTasks.isEmpty()); + stopNode(childData); + + assertEquals(1, zkDiscoveryService.delayedTasks.size()); + + verify(partitionService, never()).recalculatePartitions(eq(currentInfo), any()); + + startNode(childData); + + verify(partitionService, never()).recalculatePartitions(eq(currentInfo), any()); + + Thread.sleep(2000); + + verify(partitionService, never()).recalculatePartitions(eq(currentInfo), any()); + + assertTrue(zkDiscoveryService.delayedTasks.isEmpty()); + + //Restart not in time + stopNode(childData); + + assertEquals(1, zkDiscoveryService.delayedTasks.size()); + + Thread.sleep(2000); + + assertTrue(zkDiscoveryService.delayedTasks.isEmpty()); + + startNode(childData); + + verify(partitionService, times(1)).recalculatePartitions(eq(currentInfo), eq(Collections.emptyList())); + + verify(partitionService, times(1)).recalculatePartitions(eq(currentInfo), eq(List.of(childInfo))); + + reset(partitionService); + + //Start another node during restart + var anotherInfo = TransportProtos.ServiceInfo.newBuilder().setServiceId("anotherId").build(); + var anotherData = new ChildData("/thingsboard/nodes/0000000030", null, anotherInfo.toByteArray()); + + stopNode(childData); + + assertEquals(1, zkDiscoveryService.delayedTasks.size()); + + startNode(anotherData); + + assertTrue(zkDiscoveryService.delayedTasks.isEmpty()); + + verify(partitionService, times(1)).recalculatePartitions(eq(currentInfo), eq(List.of(anotherInfo))); + reset(partitionService); + + Thread.sleep(2000); + + verify(partitionService, never()).recalculatePartitions(eq(currentInfo), any()); + + startNode(childData); + + verify(partitionService, times(1)).recalculatePartitions(eq(currentInfo), eq(List.of(anotherInfo, childInfo))); + } + + private void startNode(ChildData data) throws Exception { + cache.getCurrentData().add(data); + zkDiscoveryService.childEvent(curatorFramework, new PathChildrenCacheEvent(CHILD_ADDED, data)); + } + + private void stopNode(ChildData data) throws Exception { + cache.getCurrentData().remove(data); + zkDiscoveryService.childEvent(curatorFramework, new PathChildrenCacheEvent(CHILD_REMOVED, data)); + } + +} From ac2aac8aa7a264e8ff9452714818cd1dfcc9ba00 Mon Sep 17 00:00:00 2001 From: YevhenBondarenko Date: Fri, 28 Jul 2023 14:20:33 +0200 Subject: [PATCH 10/10] refactored due to comments --- .../src/main/resources/thingsboard.yml | 2 +- .../queue/discovery/ZkDiscoveryService.java | 26 +++++--- .../discovery/ZkDiscoveryServiceTest.java | 62 ++++++++++++------- .../src/main/resources/tb-vc-executor.yml | 2 +- .../src/main/resources/tb-coap-transport.yml | 2 +- .../src/main/resources/tb-http-transport.yml | 2 +- .../src/main/resources/tb-lwm2m-transport.yml | 2 +- .../src/main/resources/tb-mqtt-transport.yml | 2 +- .../src/main/resources/tb-snmp-transport.yml | 2 +- 9 files changed, 62 insertions(+), 40 deletions(-) diff --git a/application/src/main/resources/thingsboard.yml b/application/src/main/resources/thingsboard.yml index 19804c0588..1f16fbc414 100644 --- a/application/src/main/resources/thingsboard.yml +++ b/application/src/main/resources/thingsboard.yml @@ -96,7 +96,7 @@ zk: session_timeout_ms: "${ZOOKEEPER_SESSION_TIMEOUT_MS:3000}" # Name of the directory in zookeeper 'filesystem' zk_dir: "${ZOOKEEPER_NODES_DIR:/thingsboard}" - recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:120000}" + recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:60000}" cluster: stats: diff --git a/common/queue/src/main/java/org/thingsboard/server/queue/discovery/ZkDiscoveryService.java b/common/queue/src/main/java/org/thingsboard/server/queue/discovery/ZkDiscoveryService.java index 50378d3387..44999d016a 100644 --- a/common/queue/src/main/java/org/thingsboard/server/queue/discovery/ZkDiscoveryService.java +++ b/common/queue/src/main/java/org/thingsboard/server/queue/discovery/ZkDiscoveryService.java @@ -16,6 +16,7 @@ package org.thingsboard.server.queue.discovery; import com.google.protobuf.InvalidProtocolBufferException; +import com.google.protobuf.ProtocolStringList; import lombok.SneakyThrows; import lombok.extern.slf4j.Slf4j; import org.apache.curator.framework.CuratorFramework; @@ -68,7 +69,7 @@ public class ZkDiscoveryService implements DiscoveryService, PathChildrenCacheLi private Integer zkSessionTimeout; @Value("${zk.zk_dir}") private String zkDir; - @Value("${zk.recalculate_delay:120000}") + @Value("${zk.recalculate_delay:60000}") private Long recalculateDelay; protected final ConcurrentHashMap> delayedTasks; @@ -294,35 +295,39 @@ public class ZkDiscoveryService implements DiscoveryService, PathChildrenCacheLi log.error("Failed to decode server instance for node {}", data.getPath(), e); throw e; } - log.debug("Processing [{}] event for [{}]", pathChildrenCacheEvent.getType(), instance.getServiceId()); + + String serviceId = instance.getServiceId(); + ProtocolStringList serviceTypesList = instance.getServiceTypesList(); + + log.trace("Processing [{}] event for [{}]", pathChildrenCacheEvent.getType(), serviceId); switch (pathChildrenCacheEvent.getType()) { case CHILD_ADDED: - ScheduledFuture task = delayedTasks.remove(instance.getServiceId()); + ScheduledFuture task = delayedTasks.remove(serviceId); if (task != null) { if (task.cancel(false)) { log.debug("[{}] Recalculate partitions ignored. Service was restarted in time [{}].", - instance.getServiceId(), instance.getServiceTypesList()); + serviceId, serviceTypesList); } else { log.debug("[{}] Going to recalculate partitions. Service was not restarted in time [{}]!", - instance.getServiceId(), instance.getServiceTypesList()); + serviceId, serviceTypesList); recalculatePartitions(); } } else { - log.debug("[{}] Going to recalculate partitions due to adding new node [{}].", - instance.getServiceId(), instance.getServiceTypesList()); + log.trace("[{}] Going to recalculate partitions due to adding new node [{}].", + serviceId, serviceTypesList); recalculatePartitions(); } break; case CHILD_REMOVED: ScheduledFuture future = zkExecutorService.schedule(() -> { log.debug("[{}] Going to recalculate partitions due to removed node [{}]", - instance.getServiceId(), instance.getServiceTypesList()); - ScheduledFuture removedTask = delayedTasks.remove(instance.getServiceId()); + serviceId, serviceTypesList); + ScheduledFuture removedTask = delayedTasks.remove(serviceId); if (removedTask != null) { recalculatePartitions(); } }, recalculateDelay, TimeUnit.MILLISECONDS); - delayedTasks.put(instance.getServiceId(), future); + delayedTasks.put(serviceId, future); break; default: break; @@ -334,6 +339,7 @@ public class ZkDiscoveryService implements DiscoveryService, PathChildrenCacheLi * Synchronized to ensure that other servers info is up to date * */ synchronized void recalculatePartitions() { + delayedTasks.values().forEach(future -> future.cancel(false)); delayedTasks.clear(); partitionService.recalculatePartitions(serviceInfoProvider.getServiceInfo(), getOtherServers()); } diff --git a/common/queue/src/test/java/org/thingsboard/server/queue/discovery/ZkDiscoveryServiceTest.java b/common/queue/src/test/java/org/thingsboard/server/queue/discovery/ZkDiscoveryServiceTest.java index 38cad217aa..a8810efd0e 100644 --- a/common/queue/src/test/java/org/thingsboard/server/queue/discovery/ZkDiscoveryServiceTest.java +++ b/common/queue/src/test/java/org/thingsboard/server/queue/discovery/ZkDiscoveryServiceTest.java @@ -63,68 +63,76 @@ public class ZkDiscoveryServiceTest { @Mock private PathChildrenCache cache; - private ScheduledExecutorService zkExecutorService; - @Mock private CuratorFramework curatorFramework; private ZkDiscoveryService zkDiscoveryService; + private static final long RECALCULATE_DELAY = 100L; + + final TransportProtos.ServiceInfo currentInfo = TransportProtos.ServiceInfo.newBuilder().setServiceId("tb-rule-engine-0").build(); + final ChildData currentData = new ChildData("/thingsboard/nodes/0000000010", null, currentInfo.toByteArray()); + final TransportProtos.ServiceInfo childInfo = TransportProtos.ServiceInfo.newBuilder().setServiceId("tb-rule-engine-1").build(); + final ChildData childData = new ChildData("/thingsboard/nodes/0000000020", null, childInfo.toByteArray()); + @Before public void setup() { zkDiscoveryService = Mockito.spy(new ZkDiscoveryService(serviceInfoProvider, partitionService)); - zkExecutorService = Executors.newSingleThreadScheduledExecutor(ThingsBoardThreadFactory.forName("zk-discovery")); + ScheduledExecutorService zkExecutorService = Executors.newSingleThreadScheduledExecutor(ThingsBoardThreadFactory.forName("zk-discovery")); when(client.getState()).thenReturn(CuratorFrameworkState.STARTED); ReflectionTestUtils.setField(zkDiscoveryService, "stopped", false); ReflectionTestUtils.setField(zkDiscoveryService, "client", client); ReflectionTestUtils.setField(zkDiscoveryService, "cache", cache); ReflectionTestUtils.setField(zkDiscoveryService, "nodePath", "/thingsboard/nodes/0000000010"); ReflectionTestUtils.setField(zkDiscoveryService, "zkExecutorService", zkExecutorService); - ReflectionTestUtils.setField(zkDiscoveryService, "recalculateDelay", 1000L); + ReflectionTestUtils.setField(zkDiscoveryService, "recalculateDelay", RECALCULATE_DELAY); ReflectionTestUtils.setField(zkDiscoveryService, "zkDir", "/thingsboard"); - } - - @Test - public void restartNodeTest() throws Exception { - var currentInfo = TransportProtos.ServiceInfo.newBuilder().setServiceId("currentId").build(); - var currentData = new ChildData("/thingsboard/nodes/0000000010", null, currentInfo.toByteArray()); - var childInfo = TransportProtos.ServiceInfo.newBuilder().setServiceId("childId").build(); - var childData = new ChildData("/thingsboard/nodes/0000000020", null, childInfo.toByteArray()); when(serviceInfoProvider.getServiceInfo()).thenReturn(currentInfo); + List dataList = new ArrayList<>(); dataList.add(currentData); when(cache.getCurrentData()).thenReturn(dataList); + } + @Test + public void restartNodeInTimeTest() throws Exception { startNode(childData); verify(partitionService, times(1)).recalculatePartitions(eq(currentInfo), eq(List.of(childInfo))); reset(partitionService); - //Restart in timeAssert.assertTrue(zkDiscoveryService.delayedTasks.isEmpty()); stopNode(childData); assertEquals(1, zkDiscoveryService.delayedTasks.size()); - verify(partitionService, never()).recalculatePartitions(eq(currentInfo), any()); + verify(partitionService, never()).recalculatePartitions(any(), any()); startNode(childData); - verify(partitionService, never()).recalculatePartitions(eq(currentInfo), any()); + verify(partitionService, never()).recalculatePartitions(any(), any()); - Thread.sleep(2000); + Thread.sleep(RECALCULATE_DELAY * 2); - verify(partitionService, never()).recalculatePartitions(eq(currentInfo), any()); + verify(partitionService, never()).recalculatePartitions(any(), any()); assertTrue(zkDiscoveryService.delayedTasks.isEmpty()); + } + + @Test + public void restartNodeNotInTimeTest() throws Exception { + startNode(childData); + + verify(partitionService, times(1)).recalculatePartitions(eq(currentInfo), eq(List.of(childInfo))); + + reset(partitionService); - //Restart not in time stopNode(childData); assertEquals(1, zkDiscoveryService.delayedTasks.size()); - Thread.sleep(2000); + Thread.sleep(RECALCULATE_DELAY * 2); assertTrue(zkDiscoveryService.delayedTasks.isEmpty()); @@ -135,11 +143,19 @@ public class ZkDiscoveryServiceTest { verify(partitionService, times(1)).recalculatePartitions(eq(currentInfo), eq(List.of(childInfo))); reset(partitionService); + } - //Start another node during restart - var anotherInfo = TransportProtos.ServiceInfo.newBuilder().setServiceId("anotherId").build(); + @Test + public void startAnotherNodeDuringRestartTest() throws Exception { + var anotherInfo = TransportProtos.ServiceInfo.newBuilder().setServiceId("tb-transport").build(); var anotherData = new ChildData("/thingsboard/nodes/0000000030", null, anotherInfo.toByteArray()); + startNode(childData); + + verify(partitionService, times(1)).recalculatePartitions(eq(currentInfo), eq(List.of(childInfo))); + + reset(partitionService); + stopNode(childData); assertEquals(1, zkDiscoveryService.delayedTasks.size()); @@ -151,9 +167,9 @@ public class ZkDiscoveryServiceTest { verify(partitionService, times(1)).recalculatePartitions(eq(currentInfo), eq(List.of(anotherInfo))); reset(partitionService); - Thread.sleep(2000); + Thread.sleep(RECALCULATE_DELAY * 2); - verify(partitionService, never()).recalculatePartitions(eq(currentInfo), any()); + verify(partitionService, never()).recalculatePartitions(any(), any()); startNode(childData); diff --git a/msa/vc-executor/src/main/resources/tb-vc-executor.yml b/msa/vc-executor/src/main/resources/tb-vc-executor.yml index 0dbb19a71a..66c6b4d3da 100644 --- a/msa/vc-executor/src/main/resources/tb-vc-executor.yml +++ b/msa/vc-executor/src/main/resources/tb-vc-executor.yml @@ -41,7 +41,7 @@ zk: session_timeout_ms: "${ZOOKEEPER_SESSION_TIMEOUT_MS:3000}" # Name of the directory in zookeeper 'filesystem' zk_dir: "${ZOOKEEPER_NODES_DIR:/thingsboard}" - recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:120000}" + recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:60000}" queue: type: "${TB_QUEUE_TYPE:kafka}" # in-memory or kafka (Apache Kafka) or aws-sqs (AWS SQS) or pubsub (PubSub) or service-bus (Azure Service Bus) or rabbitmq (RabbitMQ) diff --git a/transport/coap/src/main/resources/tb-coap-transport.yml b/transport/coap/src/main/resources/tb-coap-transport.yml index c8f4b5a099..f4b5e0bc94 100644 --- a/transport/coap/src/main/resources/tb-coap-transport.yml +++ b/transport/coap/src/main/resources/tb-coap-transport.yml @@ -41,7 +41,7 @@ zk: session_timeout_ms: "${ZOOKEEPER_SESSION_TIMEOUT_MS:3000}" # Name of the directory in zookeeper 'filesystem' zk_dir: "${ZOOKEEPER_NODES_DIR:/thingsboard}" - recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:120000}" + recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:60000}" cache: type: "${CACHE_TYPE:redis}" diff --git a/transport/http/src/main/resources/tb-http-transport.yml b/transport/http/src/main/resources/tb-http-transport.yml index fe181f12f2..f92da86b99 100644 --- a/transport/http/src/main/resources/tb-http-transport.yml +++ b/transport/http/src/main/resources/tb-http-transport.yml @@ -68,7 +68,7 @@ zk: session_timeout_ms: "${ZOOKEEPER_SESSION_TIMEOUT_MS:3000}" # Name of the directory in zookeeper 'filesystem' zk_dir: "${ZOOKEEPER_NODES_DIR:/thingsboard}" - recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:120000}" + recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:60000}" cache: type: "${CACHE_TYPE:redis}" diff --git a/transport/lwm2m/src/main/resources/tb-lwm2m-transport.yml b/transport/lwm2m/src/main/resources/tb-lwm2m-transport.yml index d80279f582..05388473f0 100644 --- a/transport/lwm2m/src/main/resources/tb-lwm2m-transport.yml +++ b/transport/lwm2m/src/main/resources/tb-lwm2m-transport.yml @@ -41,7 +41,7 @@ zk: session_timeout_ms: "${ZOOKEEPER_SESSION_TIMEOUT_MS:3000}" # Name of the directory in zookeeper 'filesystem' zk_dir: "${ZOOKEEPER_NODES_DIR:/thingsboard}" - recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:120000}" + recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:60000}" cache: type: "${CACHE_TYPE:redis}" diff --git a/transport/mqtt/src/main/resources/tb-mqtt-transport.yml b/transport/mqtt/src/main/resources/tb-mqtt-transport.yml index fcbf542287..e131788929 100644 --- a/transport/mqtt/src/main/resources/tb-mqtt-transport.yml +++ b/transport/mqtt/src/main/resources/tb-mqtt-transport.yml @@ -41,7 +41,7 @@ zk: session_timeout_ms: "${ZOOKEEPER_SESSION_TIMEOUT_MS:3000}" # Name of the directory in zookeeper 'filesystem' zk_dir: "${ZOOKEEPER_NODES_DIR:/thingsboard}" - recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:120000}" + recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:60000}" cache: type: "${CACHE_TYPE:redis}" diff --git a/transport/snmp/src/main/resources/tb-snmp-transport.yml b/transport/snmp/src/main/resources/tb-snmp-transport.yml index 0e84d54fce..a7928eb49f 100644 --- a/transport/snmp/src/main/resources/tb-snmp-transport.yml +++ b/transport/snmp/src/main/resources/tb-snmp-transport.yml @@ -41,7 +41,7 @@ zk: session_timeout_ms: "${ZOOKEEPER_SESSION_TIMEOUT_MS:3000}" # Name of the directory in zookeeper 'filesystem' zk_dir: "${ZOOKEEPER_NODES_DIR:/thingsboard}" - recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:120000}" + recalculate_delay: "${ZOOKEEPER_RECALCULATE_DELAY_MS:60000}" cache: type: "${CACHE_TYPE:redis}"