From d1af55fc102e23d5edded9e3a11a0b8ec51e62f0 Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Wed, 7 Oct 2026 13:36:26 +0200 Subject: [PATCH 1/6] UNOMI-1000: Run background tasks for every tenant Profile purge, segment date recalculation, rule statistics refresh and router startup only saw the system tenant. Walk each tenant with executeAsTenant (restoring context afterward) instead of widening the system persistence filter. --- .../core/context/RouterCamelContext.java | 49 ++- .../OSGI-INF/blueprint/blueprint.xml | 1 + .../RouterCamelContextTenantLoadTest.java | 183 ++++++++++ .../impl/profiles/ProfileServiceImpl.java | 25 +- .../services/impl/rules/RulesServiceImpl.java | 11 +- .../impl/segments/SegmentServiceImpl.java | 14 +- .../impl/BackgroundTenantTasksTest.java | 345 ++++++++++++++++++ 7 files changed, 618 insertions(+), 10 deletions(-) create mode 100644 extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java create mode 100644 services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java diff --git a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java index 5911de36b5..806d10a610 100644 --- a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java +++ b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java @@ -31,6 +31,8 @@ import org.apache.unomi.api.services.ProfileService; import org.apache.unomi.api.services.SchedulerService; import org.apache.unomi.api.tasks.ScheduledTask; +import org.apache.unomi.api.tenants.Tenant; +import org.apache.unomi.api.tenants.TenantService; import org.apache.unomi.api.security.SecurityService; import org.apache.unomi.persistence.spi.PersistenceService; import org.apache.unomi.router.api.ExportConfiguration; @@ -95,6 +97,7 @@ public class RouterCamelContext implements IRouterCamelContext { private ConfigSharingService configSharingService; private ExecutionContextManager contextManager; private SecurityService securityService; + private TenantService tenantService; private SchedulerService schedulerService; private ScheduledTask scheduledTask; @@ -127,6 +130,10 @@ public void setContextManager(ExecutionContextManager contextManager) { this.contextManager = contextManager; } + public void setTenantService(TenantService tenantService) { + this.tenantService = tenantService; + } + /** {@inheritDoc} */ @Override public void setTracing(boolean tracing) { @@ -285,6 +292,8 @@ public boolean isEnabled(EventObject event) { ProfileImportFromSourceRouteBuilder builderReader = new ProfileImportFromSourceRouteBuilder(kafkaProps, configType); builderReader.setProfileService(profileService); builderReader.setImportConfigurationService(importConfigurationService); + // Load configs under each tenant: getAll() alone only sees the current (system) context. + builderReader.setImportConfigurationList(loadConfigurationsAcrossTenants(importConfigurationService)); builderReader.setJacksonDataFormat(jacksonDataFormat); builderReader.setAllowedEndpoints(allowedEndpoints); builderReader.setPermittedImportBaseDirs(permittedImportBaseDirs); @@ -314,7 +323,7 @@ public boolean isEnabled(EventObject event) { //Profiles collect ProfileExportCollectRouteBuilder profileExportCollectRouteBuilder = new ProfileExportCollectRouteBuilder(kafkaProps, configType); - profileExportCollectRouteBuilder.setExportConfigurationList(exportConfigurationService.getAll()); + profileExportCollectRouteBuilder.setExportConfigurationList(loadConfigurationsAcrossTenants(exportConfigurationService)); profileExportCollectRouteBuilder.setExportConfigurationService(exportConfigurationService); profileExportCollectRouteBuilder.setPersistenceService(persistenceService); profileExportCollectRouteBuilder.setAllowedEndpoints(allowedEndpoints); @@ -507,4 +516,42 @@ public void setPermittedImportBaseDirs(String permittedImportBaseDirs) { public void setPermittedExportBaseDirs(String permittedExportBaseDirs) { this.permittedExportBaseDirs = permittedExportBaseDirs; } + + /** + * Loads import/export configurations for every real tenant plus the system tenant. + * {@link ImportExportConfigurationService#getAll()} is context-scoped, so a single call + * under the system context at startup would miss recurring configs of migrated tenants. + */ + private List loadConfigurationsAcrossTenants(ImportExportConfigurationService service) { + if (service == null || contextManager == null) { + return new ArrayList<>(); + } + // Startup has no request subject; nest tenant switches under system like other router paths. + return contextManager.executeAsSystem(() -> { + List all = new ArrayList<>(); + Set tenantIds = new LinkedHashSet<>(); + if (tenantService != null) { + try { + for (Tenant tenant : tenantService.getAllTenants()) { + tenantIds.add(tenant.getItemId()); + } + } catch (Exception e) { + LOGGER.error("Could not list tenants while loading router configurations", e); + } + } + tenantIds.add(TenantService.SYSTEM_TENANT); + + for (String tenantId : tenantIds) { + try { + List tenantConfigs = contextManager.executeAsTenant(tenantId, service::getAll); + if (tenantConfigs != null) { + all.addAll(tenantConfigs); + } + } catch (Exception e) { + LOGGER.error("Could not load router configurations for tenant {}", tenantId, e); + } + } + return all; + }); + } } diff --git a/extensions/router/router-core/src/main/resources/OSGI-INF/blueprint/blueprint.xml b/extensions/router/router-core/src/main/resources/OSGI-INF/blueprint/blueprint.xml index e40ce79455..7cc53a8f89 100644 --- a/extensions/router/router-core/src/main/resources/OSGI-INF/blueprint/blueprint.xml +++ b/extensions/router/router-core/src/main/resources/OSGI-INF/blueprint/blueprint.xml @@ -133,6 +133,7 @@ + diff --git a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java new file mode 100644 index 0000000000..4f5e52f512 --- /dev/null +++ b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java @@ -0,0 +1,183 @@ +/* + * 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.unomi.router.core.context; + +import org.apache.unomi.api.ExecutionContext; +import org.apache.unomi.api.services.ExecutionContextManager; +import org.apache.unomi.api.tenants.Tenant; +import org.apache.unomi.api.tenants.TenantService; +import org.apache.unomi.router.api.ImportConfiguration; +import org.apache.unomi.router.api.RouterConstants; +import org.apache.unomi.router.api.services.ImportExportConfigurationService; +import org.junit.Test; + +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +/** + * UNOMI-1000: router startup must load recurring configs for every tenant. + */ +public class RouterCamelContextTenantLoadTest { + + @Test + @SuppressWarnings("unchecked") + public void loadConfigurationsAcrossTenantsVisitsEveryTenant() throws Exception { + Set visitedTenants = new HashSet<>(); + AtomicReference currentTenant = new AtomicReference<>(); + + ImportConfiguration configA = new ImportConfiguration(); + configA.setItemId("import-a"); + ImportConfiguration configB = new ImportConfiguration(); + configB.setItemId("import-b"); + + ImportExportConfigurationService service = + new ImportExportConfigurationService() { + @Override + public List getAll() { + String tenant = currentTenant.get(); + if ("tenant-a".equals(tenant)) { + return Collections.singletonList(configA); + } + if ("tenant-b".equals(tenant)) { + return Collections.singletonList(configB); + } + return Collections.emptyList(); + } + + @Override + public ImportConfiguration load(String configId) { + return null; + } + + @Override + public ImportConfiguration save(ImportConfiguration configuration, boolean updateRunningRoute) { + return configuration; + } + + @Override + public void delete(String configId) { + } + + @Override + public Map> consumeConfigsToBeRefresh() { + return Collections.emptyMap(); + } + + @Override + public void requeueForRefresh(String tenantId, String configId, + RouterConstants.CONFIG_CAMEL_REFRESH refreshType) { + } + }; + + TenantService tenantService = (TenantService) Proxy.newProxyInstance( + TenantService.class.getClassLoader(), + new Class[]{TenantService.class}, + (proxy, method, args) -> { + if ("getAllTenants".equals(method.getName())) { + return Arrays.asList(simpleTenant("tenant-a"), simpleTenant("tenant-b")); + } + if (method.getReturnType() == boolean.class) { + return false; + } + if (method.getReturnType() == long.class || method.getReturnType() == int.class) { + return 0; + } + return null; + }); + + ExecutionContextManager contextManager = new ExecutionContextManager() { + @Override + public ExecutionContext getCurrentContext() { + return null; + } + + @Override + public void setCurrentContext(ExecutionContext context) { + } + + @Override + public T executeAsSystem(Supplier operation) { + return operation.get(); + } + + @Override + public void executeAsSystem(Runnable operation) { + operation.run(); + } + + @Override + public T executeAsTenant(String tenantId, Supplier operation) { + visitedTenants.add(tenantId); + currentTenant.set(tenantId); + try { + return operation.get(); + } finally { + currentTenant.set(null); + } + } + + @Override + public void executeAsTenant(String tenantId, Runnable operation) { + visitedTenants.add(tenantId); + currentTenant.set(tenantId); + try { + operation.run(); + } finally { + currentTenant.set(null); + } + } + + @Override + public ExecutionContext createContext(String tenantId) { + return null; + } + }; + + RouterCamelContext context = new RouterCamelContext(); + context.setContextManager(contextManager); + context.setTenantService(tenantService); + + Method method = RouterCamelContext.class.getDeclaredMethod( + "loadConfigurationsAcrossTenants", ImportExportConfigurationService.class); + method.setAccessible(true); + List loaded = (List) method.invoke(context, service); + + assertEquals("expected one config per real tenant", 2, loaded.size()); + assertTrue(loaded.stream().anyMatch(c -> "import-a".equals(c.getItemId()))); + assertTrue(loaded.stream().anyMatch(c -> "import-b".equals(c.getItemId()))); + assertTrue(visitedTenants.contains("tenant-a")); + assertTrue(visitedTenants.contains("tenant-b")); + assertTrue(visitedTenants.contains(TenantService.SYSTEM_TENANT)); + } + + private static Tenant simpleTenant(String id) { + Tenant tenant = new Tenant(); + tenant.setItemId(id); + return tenant; + } +} diff --git a/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java index 8bb24622a6..04469d94c6 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java @@ -365,13 +365,24 @@ public void execute(ScheduledTask task, TaskExecutor.TaskStatusCallback callback long purgeStartTime = System.currentTimeMillis(); LOGGER.info("Purge: triggered"); - // Profile purge - purgeProfiles(purgeProfileInactiveTime, purgeProfileExistTime); - if (purgeSessionExistTime > 0) { - purgeSessionItems(purgeSessionExistTime); - } - if (purgeEventExistTime > 0) { - purgeEventItems(purgeEventExistTime); + // Run under each tenant so persistence sees that tenant's data + // (system context alone only matches tenantId=system). + for (String tenantId : getTenants()) { + try { + contextManager.executeAsTenant(tenantId, () -> { + purgeProfiles(purgeProfileInactiveTime, purgeProfileExistTime); + if (purgeSessionExistTime > 0) { + purgeSessionItems(purgeSessionExistTime); + } + if (purgeEventExistTime > 0) { + purgeEventItems(purgeEventExistTime); + } + return null; + }); + } catch (Throwable t) { + LOGGER.error("Error while purging profiles, sessions, or events for tenant {}", + tenantId, t); + } } LOGGER.info("Purge: executed in {} ms", System.currentTimeMillis() - purgeStartTime); diff --git a/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java index 8e9264c325..a7aebce9f0 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java @@ -195,7 +195,16 @@ public void postConstruct() { .nonPersistent() .withPeriod(rulesStatisticsRefreshInterval, TimeUnit.MILLISECONDS) .withFixedDelay() - .withSimpleExecutor(() -> contextManager.executeAsSystem(() -> syncRuleStatistics())) + .withSimpleExecutor(() -> contextManager.executeAsSystem(() -> { + // Flush in-memory stats for every tenant; system context alone only persists system. + for (String tenantId : getTenants()) { + try { + contextManager.executeAsTenant(tenantId, () -> syncRuleStatistics()); + } catch (Throwable t) { + LOGGER.error("Error syncing rule statistics for tenant {}", tenantId, t); + } + } + })) .schedule(); LOGGER.info("Rule service initialized."); diff --git a/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java index 1ebbc0453c..2604a53da3 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java @@ -1554,7 +1554,19 @@ public void execute(ScheduledTask task, TaskExecutor.TaskStatusCallback callback try { long currentTimeMillis = System.currentTimeMillis(); LOGGER.info("Running scheduled task to recalculate segments and scoring that contains date relative conditions..."); - recalculatePastEventConditions(); + // Run under each tenant so rules/segments/profiles of real tenants are seen + // (system context alone only matches tenantId=system). + for (String tenantId : getTenants()) { + try { + contextManager.executeAsTenant(tenantId, () -> { + recalculatePastEventConditions(); + return null; + }); + } catch (Throwable t) { + LOGGER.error("Error recalculating date-relative segments/scoring for tenant {}", + tenantId, t); + } + } LOGGER.info("...Finished recalculate segments and scoring that contains date relative conditions in {}ms. ", System.currentTimeMillis() - currentTimeMillis); callback.complete(); } catch (Throwable t) { diff --git a/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java b/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java new file mode 100644 index 0000000000..9a52c79ce9 --- /dev/null +++ b/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java @@ -0,0 +1,345 @@ +/* + * 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.unomi.services.impl; + +import org.apache.unomi.api.ExecutionContext; +import org.apache.unomi.api.Profile; +import org.apache.unomi.api.services.EventService; +import org.apache.unomi.api.services.RulesService; +import org.apache.unomi.api.services.SegmentService; +import org.apache.unomi.api.tasks.ScheduledTask; +import org.apache.unomi.api.tasks.TaskExecutor; +import org.apache.unomi.api.tenants.TenantService; +import org.apache.unomi.persistence.spi.PersistenceService; +import org.apache.unomi.persistence.spi.conditions.evaluator.ConditionEvaluatorDispatcher; +import org.apache.unomi.services.TestHelper; +import org.apache.unomi.services.common.security.ExecutionContextManagerImpl; +import org.apache.unomi.services.common.security.KarafSecurityService; +import org.apache.unomi.services.impl.cache.MultiTypeCacheServiceImpl; +import org.apache.unomi.services.impl.definitions.DefinitionsServiceImpl; +import org.apache.unomi.services.impl.profiles.ProfileServiceImpl; +import org.apache.unomi.services.impl.rules.RulesServiceImpl; +import org.apache.unomi.services.impl.scheduler.SchedulerServiceImpl; +import org.apache.unomi.services.impl.segments.SegmentServiceImpl; +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.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; +import org.osgi.framework.Bundle; + +import java.util.Collections; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.when; + +/** + * UNOMI-1000: background jobs must visit every tenant (not only {@code system}). + *

+ * These tests fail on the 4.0.0 RC behaviour where purge / segment recalculation / + * rule-statistics refresh ran solely under {@code executeAsSystem}, so persistence + * only ever saw {@code tenantId=system}. + */ +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.LENIENT) +public class BackgroundTenantTasksTest { + + private static final String TENANT_A = "tenant1"; + private static final String TENANT_B = "tenant2"; + + private TestTenantService tenantService; + private PersistenceService persistenceService; + private DefinitionsServiceImpl definitionsService; + private TestBundleContext bundleContext; + private ExecutionContextManagerImpl executionContextManager; + private MultiTypeCacheServiceImpl multiTypeCacheService; + private KarafSecurityService securityService; + private SchedulerServiceImpl schedulerService; + + @BeforeEach + public void setUp() { + bundleContext = new TestBundleContext(); + Bundle systemBundle = mock(Bundle.class); + when(systemBundle.getBundleContext()).thenReturn(bundleContext); + when(systemBundle.getBundleId()).thenReturn(0L); + when(systemBundle.getSymbolicName()).thenReturn("org.apache.unomi.predefined"); + bundleContext.addBundle(systemBundle); + + tenantService = new TestTenantService(); + TestHelper.setupCommonTestData(tenantService); + + securityService = TestHelper.createSecurityService(); + executionContextManager = TestHelper.createExecutionContextManager(securityService); + multiTypeCacheService = new MultiTypeCacheServiceImpl(); + + ConditionEvaluatorDispatcher dispatcher = TestConditionEvaluators.createDispatcher(); + persistenceService = new InMemoryPersistenceServiceImpl(executionContextManager, dispatcher); + schedulerService = TestHelper.createSchedulerService( + "background-tenant-tasks-node", persistenceService, executionContextManager, + bundleContext, null, -1, true, true); + + definitionsService = TestHelper.createDefinitionService( + persistenceService, bundleContext, schedulerService, multiTypeCacheService, + executionContextManager, tenantService); + TestHelper.injectDefinitionsServiceIntoDispatcher(dispatcher, definitionsService); + TestConditionEvaluators.getConditionTypes().forEach((k, v) -> definitionsService.setConditionType(v)); + } + + @AfterEach + public void tearDown() throws Exception { + TestHelper.tearDown(schedulerService, multiTypeCacheService, persistenceService, tenantService, + TENANT_A, TENANT_B, TenantService.SYSTEM_TENANT); + TestHelper.cleanupReferences(tenantService, securityService, executionContextManager, + persistenceService, definitionsService, bundleContext, schedulerService, multiTypeCacheService); + } + + @Test + public void profilePurgeTaskRunsUnderEveryTenantAndRestoresContext() throws Exception { + Set visitedTenants = ConcurrentHashMap.newKeySet(); + ExecutionContextManagerImpl spiedContext = spy(executionContextManager); + trackTenantVisits(spiedContext, visitedTenants); + + AtomicReference purgeExecutor = new AtomicReference<>(); + SchedulerServiceImpl spiedScheduler = spy(schedulerService); + captureExecutor(spiedScheduler, "profile-purge", purgeExecutor); + + ProfileServiceImpl profileService = new ProfileServiceImpl(); + profileService.setBundleContext(bundleContext); + profileService.setPersistenceService(persistenceService); + profileService.setDefinitionsService(definitionsService); + profileService.setContextManager(spiedContext); + profileService.setSchedulerService(spiedScheduler); + profileService.setCacheService(multiTypeCacheService); + profileService.setTenantService(tenantService); + profileService.setSegmentService(mock(SegmentService.class)); + profileService.setPurgeProfileExistTime(30); + profileService.setPurgeProfileInterval(1); + profileService.postConstruct(); + + assertNotNull(purgeExecutor.get(), "profile-purge executor must be registered"); + + ExecutionContext before = spiedContext.getCurrentContext(); + String beforeTenant = before != null ? before.getTenantId() : null; + + runExecutor(purgeExecutor.get()); + + assertTrue(visitedTenants.contains(TENANT_A), "purge must visit tenant1, visited=" + visitedTenants); + assertTrue(visitedTenants.contains(TENANT_B), "purge must visit tenant2, visited=" + visitedTenants); + assertTrue(visitedTenants.contains(TenantService.SYSTEM_TENANT), + "purge must also visit system, visited=" + visitedTenants); + + ExecutionContext after = spiedContext.getCurrentContext(); + String afterTenant = after != null ? after.getTenantId() : null; + assertEquals(beforeTenant, afterTenant, "tenant context must be restored after the purge task"); + } + + @Test + public void segmentDateRecalculationTaskRunsUnderEveryTenant() throws Exception { + Set visitedTenants = ConcurrentHashMap.newKeySet(); + ExecutionContextManagerImpl spiedContext = spy(executionContextManager); + trackTenantVisits(spiedContext, visitedTenants); + + AtomicReference segmentExecutor = new AtomicReference<>(); + SchedulerServiceImpl spiedScheduler = spy(schedulerService); + captureExecutor(spiedScheduler, "segment-date-recalculation", segmentExecutor); + + RulesService rulesService = mock(RulesService.class); + when(rulesService.getAllRules()).thenReturn(Collections.emptyList()); + + SegmentServiceImpl segmentService = new SegmentServiceImpl(); + segmentService.setBundleContext(bundleContext); + segmentService.setPersistenceService(persistenceService); + segmentService.setDefinitionsService(definitionsService); + segmentService.setContextManager(spiedContext); + segmentService.setSchedulerService(spiedScheduler); + segmentService.setCacheService(multiTypeCacheService); + segmentService.setTenantService(tenantService); + segmentService.setRulesService(rulesService); + segmentService.setEventService(mock(EventService.class)); + segmentService.setTracerService(TestHelper.createTracerService()); + segmentService.postConstruct(); + + assertNotNull(segmentExecutor.get(), "segment-date-recalculation executor must be registered"); + runExecutor(segmentExecutor.get()); + + assertTrue(visitedTenants.contains(TENANT_A), "segment recalc must visit tenant1, visited=" + visitedTenants); + assertTrue(visitedTenants.contains(TENANT_B), "segment recalc must visit tenant2, visited=" + visitedTenants); + } + + @Test + public void ruleStatisticsRefreshVisitsEveryTenant() throws Exception { + Set visitedTenants = ConcurrentHashMap.newKeySet(); + ExecutionContextManagerImpl spiedContext = spy(executionContextManager); + trackTenantVisits(spiedContext, visitedTenants); + + AtomicReference statsExecutor = new AtomicReference<>(); + SchedulerServiceImpl spiedScheduler = spy(schedulerService); + captureExecutor(spiedScheduler, "rules-statistics-refresh", statsExecutor); + + RulesServiceImpl rulesService = new RulesServiceImpl(); + rulesService.setBundleContext(bundleContext); + rulesService.setPersistenceService(persistenceService); + rulesService.setDefinitionsService(definitionsService); + rulesService.setContextManager(spiedContext); + rulesService.setSchedulerService(spiedScheduler); + rulesService.setCacheService(multiTypeCacheService); + rulesService.setTenantService(tenantService); + rulesService.setEventService(mock(EventService.class)); + rulesService.setActionExecutorDispatcher(mock(org.apache.unomi.services.actions.ActionExecutorDispatcher.class)); + rulesService.setTracerService(TestHelper.createTracerService()); + rulesService.setRulesRefreshInterval(60000); + rulesService.setRulesStatisticsRefreshInterval(60000); + rulesService.postConstruct(); + + assertNotNull(statsExecutor.get(), "rules-statistics-refresh executor must be registered"); + runExecutor(statsExecutor.get()); + + assertTrue(visitedTenants.contains(TENANT_A), "stats refresh must visit tenant1, visited=" + visitedTenants); + assertTrue(visitedTenants.contains(TENANT_B), "stats refresh must visit tenant2, visited=" + visitedTenants); + assertTrue(visitedTenants.contains(TenantService.SYSTEM_TENANT), + "stats refresh must visit system, visited=" + visitedTenants); + } + + @Test + public void systemContextPersistenceStillOnlySeesSystemTenant() { + executionContextManager.executeAsTenant(TENANT_A, () -> { + Profile profile = new Profile("tenant-a-only"); + persistenceService.save(profile); + return null; + }); + persistenceService.refresh(); + + executionContextManager.executeAsSystem(() -> { + assertNull(persistenceService.load("tenant-a-only", Profile.class), + "system context must not load another tenant's profile"); + assertEquals(0L, persistenceService.getAllItemsCount(Profile.ITEM_TYPE), + "system context must not count another tenant's profiles"); + return null; + }); + + executionContextManager.executeAsTenant(TENANT_A, () -> { + assertNotNull(persistenceService.load("tenant-a-only", Profile.class)); + return null; + }); + } + + @Test + public void purgeForOneTenantDoesNotRemoveAnotherTenantsProfiles() { + ProfileServiceImpl profileService = new ProfileServiceImpl(); + profileService.setBundleContext(bundleContext); + profileService.setPersistenceService(persistenceService); + profileService.setDefinitionsService(definitionsService); + profileService.setContextManager(executionContextManager); + profileService.setSchedulerService(schedulerService); + profileService.setCacheService(multiTypeCacheService); + profileService.setTenantService(tenantService); + profileService.setSegmentService(mock(SegmentService.class)); + profileService.postConstruct(); + + executionContextManager.executeAsTenant(TENANT_A, () -> { + Profile old = new Profile("a-old"); + old.setProperty("firstVisit", new java.util.Date(0)); + persistenceService.save(old); + return null; + }); + executionContextManager.executeAsTenant(TENANT_B, () -> { + Profile old = new Profile("b-old"); + old.setProperty("firstVisit", new java.util.Date(0)); + persistenceService.save(old); + return null; + }); + persistenceService.refresh(); + + executionContextManager.executeAsTenant(TENANT_A, () -> { + profileService.purgeProfiles(0, 1); + return null; + }); + persistenceService.refresh(); + + executionContextManager.executeAsTenant(TENANT_A, () -> + assertNull(persistenceService.load("a-old", Profile.class))); + executionContextManager.executeAsTenant(TENANT_B, () -> + assertNotNull(persistenceService.load("b-old", Profile.class), + "tenant B profiles must stay when purge runs only for tenant A")); + } + + @SuppressWarnings("unchecked") + private static void trackTenantVisits(ExecutionContextManagerImpl spiedContext, Set visitedTenants) { + doAnswer(invocation -> { + visitedTenants.add(invocation.getArgument(0)); + return invocation.callRealMethod(); + }).when(spiedContext).executeAsTenant(any(String.class), any(Supplier.class)); + } + + private static void captureExecutor(SchedulerServiceImpl spiedScheduler, String taskType, + AtomicReference target) { + doAnswer(invocation -> { + TaskExecutor executor = invocation.getArgument(0); + if (taskType.equals(executor.getTaskType())) { + target.set(executor); + } + return invocation.callRealMethod(); + }).when(spiedScheduler).registerTaskExecutor(any(TaskExecutor.class)); + } + + private static void runExecutor(TaskExecutor executor) throws Exception { + CountDownLatch done = new CountDownLatch(1); + AtomicReference failure = new AtomicReference<>(); + executor.execute(mock(ScheduledTask.class), new TaskExecutor.TaskStatusCallback() { + @Override + public void updateStep(String step, Map details) { + } + + @Override + public void checkpoint(Map checkpointData) { + } + + @Override + public void updateStatusDetails(Map details) { + } + + @Override + public void complete() { + done.countDown(); + } + + @Override + public void fail(String error) { + failure.set(error); + done.countDown(); + } + }); + assertTrue(done.await(30, TimeUnit.SECONDS), "task should finish"); + assertNull(failure.get(), "task should not fail: " + failure.get()); + } +} From e55fd88eac6b1934f11fbcc4f1902867438d7e49 Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Fri, 9 Oct 2026 08:26:07 +0200 Subject: [PATCH 2/6] UNOMI-1000: Keep migration IT data safe under per-tenant purge Disable the scheduled profile purge in ITs so years-old snapshot data is not deleted once purge walks every tenant. Also harden tenant task execution and use tenant-scoped router route IDs. --- .../unomi/router/api/IRouterCamelContext.java | 30 +- .../org/apache/unomi/router/api/RouteIds.java | 56 ++ .../core/context/RouterCamelContext.java | 314 +++++++---- .../ExportRouteCompletionProcessor.java | 3 +- .../ImportRouteCompletionProcessor.java | 6 +- .../ProfileExportCollectRouteBuilder.java | 3 +- .../ProfileImportFromSourceRouteBuilder.java | 3 +- .../RouterCamelContextTenantLoadTest.java | 498 ++++++++++++++---- .../RouteCompletionProcessorTest.java | 127 +++++ .../route/FileEndpointContainmentTest.java | 28 +- .../route/RefusedConfigurationStatusTest.java | 13 +- .../unomi/router/core/route/RouteIdsTest.java | 47 ++ .../java/org/apache/unomi/itests/BaseIT.java | 51 +- .../unomi/itests/ProfileImportActorsIT.java | 4 +- .../unomi/itests/ProfileImportSurfersIT.java | 12 +- .../AbstractMultiTypeCachingService.java | 45 ++ .../impl/profiles/ProfileServiceImpl.java | 26 +- .../services/impl/rules/RulesServiceImpl.java | 21 +- .../impl/segments/SegmentServiceImpl.java | 14 +- .../impl/BackgroundTenantTasksTest.java | 363 +++++++++---- 20 files changed, 1242 insertions(+), 422 deletions(-) create mode 100644 extensions/router/router-api/src/main/java/org/apache/unomi/router/api/RouteIds.java create mode 100644 extensions/router/router-core/src/test/java/org/apache/unomi/router/core/processor/RouteCompletionProcessorTest.java create mode 100644 extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RouteIdsTest.java diff --git a/extensions/router/router-api/src/main/java/org/apache/unomi/router/api/IRouterCamelContext.java b/extensions/router/router-api/src/main/java/org/apache/unomi/router/api/IRouterCamelContext.java index c6835f143e..63d0b82c12 100644 --- a/extensions/router/router-api/src/main/java/org/apache/unomi/router/api/IRouterCamelContext.java +++ b/extensions/router/router-api/src/main/java/org/apache/unomi/router/api/IRouterCamelContext.java @@ -33,7 +33,8 @@ *

Typical usage: *

    *
  • Management services call update methods when import/export configuration documents change
    • - *
  • Cleanup paths call {@link #killExistingRoute(String, boolean)} to drop routes whose configs were removed
    • + *
  • Cleanup paths call {@link #killExistingRoute(String, String, boolean)} to drop the route of a tenant's + * removed config; Camel route ids are built by {@link RouteIds}
    • *
* * @since 1.0 @@ -41,36 +42,41 @@ public interface IRouterCamelContext { /** - * Stops and removes an existing Camel route by id, if it is currently registered in the context. + * Stops and removes the Camel route of a tenant's import/export configuration, if it is currently + * registered in the context. A configuration of the same id in another tenant keeps its route. * - * @param routeId Camel route identifier (usually aligned with import/export configuration id) + * @param tenantId tenant owning the configuration + * @param configId identifier of the import/export configuration whose route is removed * @param fireEvent when {@code true}, signals that router lifecycle events may be emitted; the concrete * implementation defines whether events are fired (reserved hook for observability) * @throws Exception if Camel fails to remove the route definition */ - void killExistingRoute(String routeId, boolean fireEvent) throws Exception; + void killExistingRoute(String tenantId, String configId, boolean fireEvent) throws Exception; /** - * Refreshes the profile import reader route for the given configuration: removes any existing route with the - * same id, loads the {@link org.apache.unomi.router.api.ImportConfiguration}, and—for recurrent configs— - * registers a new route built from current settings. + * Refreshes the profile import reader route for the given configuration: removes the existing route of that + * tenant's configuration, loads the {@link org.apache.unomi.router.api.ImportConfiguration}, and—for recurrent + * configs—registers a new route built from current settings. Must be called in the tenant's execution context. * + * @param tenantId tenant owning the configuration * @param configId identifier of the import configuration whose reader route should be updated * @param fireEvent when {@code true}, signals that router lifecycle events may be emitted after the update * @throws Exception if route removal or registration fails */ - void updateProfileImportReaderRoute(String configId, boolean fireEvent) throws Exception; + void updateProfileImportReaderRoute(String tenantId, String configId, boolean fireEvent) throws Exception; /** - * Refreshes the profile export reader (collect) route for the given configuration: removes any existing route - * with the same id, loads the {@link org.apache.unomi.router.api.ExportConfiguration}, and—for recurrent - * configs—registers a new collect route built from current settings. + * Refreshes the profile export reader (collect) route for the given configuration: removes the existing route + * of that tenant's configuration, loads the {@link org.apache.unomi.router.api.ExportConfiguration}, and—for + * recurrent configs—registers a new collect route built from current settings. Must be called in the tenant's + * execution context. * + * @param tenantId tenant owning the configuration * @param configId identifier of the export configuration whose reader route should be updated * @param fireEvent when {@code true}, signals that router lifecycle events may be emitted after the update * @throws Exception if route removal or registration fails */ - void updateProfileExportReaderRoute(String configId, boolean fireEvent) throws Exception; + void updateProfileExportReaderRoute(String tenantId, String configId, boolean fireEvent) throws Exception; /** * Enables or disables Camel route tracing on the underlying {@code CamelContext} for debugging (message flow, diff --git a/extensions/router/router-api/src/main/java/org/apache/unomi/router/api/RouteIds.java b/extensions/router/router-api/src/main/java/org/apache/unomi/router/api/RouteIds.java new file mode 100644 index 0000000000..617e8fc972 --- /dev/null +++ b/extensions/router/router-api/src/main/java/org/apache/unomi/router/api/RouteIds.java @@ -0,0 +1,56 @@ +/* + * 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.unomi.router.api; + +import org.apache.unomi.api.ExecutionContext; + +/** + * Identifiers of the Camel routes built from recurrent import/export configurations. + *

A configuration identifier is only unique within its tenant, while Camel route identifiers are + * global, so a route is identified by {@code :}. Tenant identifiers cannot + * contain a colon, which keeps the configuration identifier recoverable whatever it contains.

+ */ +public final class RouteIds { + + private static final char SEPARATOR = ':'; + + private RouteIds() { + } + + /** + * Returns the identifier of the route of a configuration. + * + * @param tenantId the tenant owning the configuration, the system tenant when {@code null} + * @param configId the configuration identifier + * @return the Camel route identifier + */ + public static String of(String tenantId, String configId) { + return (tenantId != null ? tenantId : ExecutionContext.SYSTEM_TENANT) + SEPARATOR + configId; + } + + /** + * Returns the identifier of the configuration a route was built from. + * + * @param routeId a Camel route identifier + * @return the configuration identifier, or the route identifier itself for a route that is not + * built from a configuration + */ + public static String configId(String routeId) { + int separator = routeId.indexOf(SEPARATOR); + return separator < 0 ? routeId : routeId.substring(separator + 1); + } +} diff --git a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java index 806d10a610..53e2ba1706 100644 --- a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java +++ b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java @@ -26,6 +26,7 @@ import org.apache.camel.management.event.ExchangeSentEvent; import org.apache.camel.model.RouteDefinition; import org.apache.camel.support.EventNotifierSupport; +import org.apache.unomi.api.Item; import org.apache.unomi.api.services.ConfigSharingService; import org.apache.unomi.api.services.ExecutionContextManager; import org.apache.unomi.api.services.ProfileService; @@ -38,6 +39,7 @@ import org.apache.unomi.router.api.ExportConfiguration; import org.apache.unomi.router.api.IRouterCamelContext; import org.apache.unomi.router.api.ImportConfiguration; +import org.apache.unomi.router.api.RouteIds; import org.apache.unomi.router.api.RouterConstants; import org.apache.unomi.router.api.services.ImportExportConfigurationService; import org.apache.unomi.router.api.services.ProfileExportService; @@ -99,6 +101,14 @@ public class RouterCamelContext implements IRouterCamelContext { private SecurityService securityService; private TenantService tenantService; + /** Tenants whose recurring routes could not be built at startup; the refresh timer retries them. */ + private final Set importTenantsToRetry = ConcurrentHashMap.newKeySet(); + private final Set exportTenantsToRetry = ConcurrentHashMap.newKeySet(); + private volatile boolean tenantListingToRetry; + static final long MAX_STARTUP_RETRY_DELAY_MS = 60_000; + private long startupRetryDelay; + private long nextStartupRetryTime; + private SchedulerService schedulerService; private ScheduledTask scheduledTask; @@ -154,7 +164,20 @@ public void init() throws Exception { configSharingService.setProperty(RouterConstants.CONFIG_EXPORT_BASE_DIRS, permittedExportBaseDirs); configSharingService.setProperty(RouterConstants.KEY_HISTORY_SIZE, execHistorySize); - initCamel(); + try { + initCamel(); + } catch (Exception e) { + // Do not leave a context behind that nothing will ever stop. + if (camelContext != null) { + try { + camelContext.stop(); + } catch (Exception stopFailure) { + e.addSuppressed(stopFailure); + } + camelContext = null; + } + throw e; + } initTimers(); LOGGER.info("Camel Context initialized successfully."); @@ -174,93 +197,112 @@ private void initTimers() { TimerTask task = new TimerTask() { @Override public void run() { - try { - Map> tenantsImportConfigsToRefresh = importConfigurationService.consumeConfigsToBeRefresh(); + refreshRoutes(); + } + }; + scheduledTask = schedulerService.createRecurringTask("camel-route-refresh", configsRefreshInterval, TimeUnit.MILLISECONDS, task, false); + } - for (Map.Entry> tenantImportConfigsToRefresh : tenantsImportConfigsToRefresh.entrySet()) { - String tenantId = tenantImportConfigsToRefresh.getKey(); - contextManager.executeAsTenant(tenantId, () -> { + /** + * One tick of the refresh timer: retries what could not be started at startup, then rebuilds or removes + * the routes of the configurations that were saved, deleted or re-queued since the previous tick. + */ + void refreshRoutes() { + try { + retryStartupLoadsWhenDue(System.currentTimeMillis()); + } catch (Exception e) { + // must not keep the refreshes below from being processed + LOGGER.warn("Unexpected error while retrying to start the recurring routes that could not be started at startup", e); + } + try { + Map> tenantsImportConfigsToRefresh = importConfigurationService.consumeConfigsToBeRefresh(); + + for (Map.Entry> tenantImportConfigsToRefresh : tenantsImportConfigsToRefresh.entrySet()) { + String tenantId = tenantImportConfigsToRefresh.getKey(); + contextManager.executeAsTenant(tenantId, () -> { + try { + for (Map.Entry importConfigToRefresh : tenantImportConfigsToRefresh.getValue().entrySet()) { + String configId = importConfigToRefresh.getKey(); + RouterConstants.CONFIG_CAMEL_REFRESH refreshType = importConfigToRefresh.getValue(); + RetryKey retryKey = new RetryKey("import", tenantId, configId); try { - for (Map.Entry importConfigToRefresh : tenantImportConfigsToRefresh.getValue().entrySet()) { - String configId = importConfigToRefresh.getKey(); - RouterConstants.CONFIG_CAMEL_REFRESH refreshType = importConfigToRefresh.getValue(); - RetryKey retryKey = new RetryKey("import", tenantId, configId); - try { - if (refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED)) { - updateProfileImportReaderRoute(configId, true); - routeCreationRetryCount.remove(retryKey); - } else if (refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.REMOVED)) { - killExistingRoute(configId, true); - routeCreationRetryCount.remove(retryKey); - } - } catch (Exception e) { - int attempt = routeCreationRetryCount.merge(retryKey, 1, Integer::sum); - if (attempt <= MAX_ROUTE_CREATION_RETRIES) { - LOGGER.error("Refreshing({}) camel route {} failed (attempt {}/{}) — will retry on next tick", - refreshType, configId, attempt, MAX_ROUTE_CREATION_RETRIES, e); - importConfigurationService.requeueForRefresh(tenantId, configId, refreshType); - } else { - LOGGER.error("Refreshing({}) camel route {} failed after {} attempts — giving up", - refreshType, configId, MAX_ROUTE_CREATION_RETRIES, e); - routeCreationRetryCount.remove(retryKey); - markImportRouteCreationFailed(configId); - } - } + if (refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED)) { + updateProfileImportReaderRoute(tenantId, configId, true); + routeCreationRetryCount.remove(retryKey); + } else if (refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.REMOVED)) { + killExistingRoute(tenantId, configId, true); + routeCreationRetryCount.remove(retryKey); } } catch (Exception e) { - LOGGER.error("Unexpected error while refreshing import/export camel routes for tenant {}", tenantId, e); + int attempt = routeCreationRetryCount.merge(retryKey, 1, Integer::sum); + if (attempt <= MAX_ROUTE_CREATION_RETRIES) { + LOGGER.error("Refreshing({}) camel route {} failed (attempt {}/{}) — will retry on next tick", + refreshType, configId, attempt, MAX_ROUTE_CREATION_RETRIES, e); + importConfigurationService.requeueForRefresh(tenantId, configId, refreshType); + } else { + LOGGER.error("Refreshing({}) camel route {} failed after {} attempts — giving up", + refreshType, configId, MAX_ROUTE_CREATION_RETRIES, e); + routeCreationRetryCount.remove(retryKey); + markImportRouteCreationFailed(configId); + } } - return null; - }); + } + } catch (Exception e) { + LOGGER.error("Unexpected error while refreshing import/export camel routes for tenant {}", tenantId, e); } + return null; + }); + } - Map> tenantsExportConfigsToRefresh = exportConfigurationService.consumeConfigsToBeRefresh(); - for (Map.Entry> tenantExportConfigsToRefresh : tenantsExportConfigsToRefresh.entrySet()) { - String tenantId = tenantExportConfigsToRefresh.getKey(); - contextManager.executeAsTenant(tenantId, () -> { + Map> tenantsExportConfigsToRefresh = exportConfigurationService.consumeConfigsToBeRefresh(); + for (Map.Entry> tenantExportConfigsToRefresh : tenantsExportConfigsToRefresh.entrySet()) { + String tenantId = tenantExportConfigsToRefresh.getKey(); + contextManager.executeAsTenant(tenantId, () -> { + try { + for (Map.Entry exportConfigToRefresh : tenantExportConfigsToRefresh.getValue().entrySet()) { + String configId = exportConfigToRefresh.getKey(); + RouterConstants.CONFIG_CAMEL_REFRESH refreshType = exportConfigToRefresh.getValue(); + RetryKey retryKey = new RetryKey("export", tenantId, configId); try { - for (Map.Entry exportConfigToRefresh : tenantExportConfigsToRefresh.getValue().entrySet()) { - String configId = exportConfigToRefresh.getKey(); - RouterConstants.CONFIG_CAMEL_REFRESH refreshType = exportConfigToRefresh.getValue(); - RetryKey retryKey = new RetryKey("export", tenantId, configId); - try { - if (refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED)) { - updateProfileExportReaderRoute(configId, true); - routeCreationRetryCount.remove(retryKey); - } else if (refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.REMOVED)) { - killExistingRoute(configId, true); - routeCreationRetryCount.remove(retryKey); - } - } catch (Exception e) { - int attempt = routeCreationRetryCount.merge(retryKey, 1, Integer::sum); - if (attempt <= MAX_ROUTE_CREATION_RETRIES) { - LOGGER.error("Refreshing({}) camel route {} failed (attempt {}/{}) — will retry on next tick", - refreshType, configId, attempt, MAX_ROUTE_CREATION_RETRIES, e); - exportConfigurationService.requeueForRefresh(tenantId, configId, refreshType); - } else { - LOGGER.error("Refreshing({}) camel route {} failed after {} attempts — giving up", - refreshType, configId, MAX_ROUTE_CREATION_RETRIES, e); - routeCreationRetryCount.remove(retryKey); - markExportRouteCreationFailed(configId); - } - } + if (refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED)) { + updateProfileExportReaderRoute(tenantId, configId, true); + routeCreationRetryCount.remove(retryKey); + } else if (refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.REMOVED)) { + killExistingRoute(tenantId, configId, true); + routeCreationRetryCount.remove(retryKey); } } catch (Exception e) { - LOGGER.error("Unexpected error while refreshing import/export camel routes for tenant {}", tenantId, e); + int attempt = routeCreationRetryCount.merge(retryKey, 1, Integer::sum); + if (attempt <= MAX_ROUTE_CREATION_RETRIES) { + LOGGER.error("Refreshing({}) camel route {} failed (attempt {}/{}) — will retry on next tick", + refreshType, configId, attempt, MAX_ROUTE_CREATION_RETRIES, e); + exportConfigurationService.requeueForRefresh(tenantId, configId, refreshType); + } else { + LOGGER.error("Refreshing({}) camel route {} failed after {} attempts — giving up", + refreshType, configId, MAX_ROUTE_CREATION_RETRIES, e); + routeCreationRetryCount.remove(retryKey); + markExportRouteCreationFailed(configId); + } } - return null; - }); + } + } catch (Exception e) { + LOGGER.error("Unexpected error while refreshing import/export camel routes for tenant {}", tenantId, e); } - } catch (Exception e) { - LOGGER.error("Unexpected error while refreshing import/export camel routes", e); - } + return null; + }); } - }; - scheduledTask = schedulerService.createRecurringTask("camel-route-refresh", configsRefreshInterval, TimeUnit.MILLISECONDS, task, false); + } catch (Exception e) { + LOGGER.error("Unexpected error while refreshing import/export camel routes", e); + } } - private void initCamel() throws Exception { - camelContext = new OsgiDefaultCamelContext(bundleContext); + /** Creates the Camel context the routes are added to; a seam for running the route setup outside OSGi. */ + CamelContext createCamelContext() { + return new OsgiDefaultCamelContext(bundleContext); + } + + void initCamel() throws Exception { + camelContext = createCamelContext(); // Setup listener, we might want to improve this to know exactly what is running at a given time and expose an API to query this information camelContext.getManagementStrategy().addEventNotifier(new EventNotifierSupport() { @@ -286,6 +328,9 @@ public boolean isEnabled(EventObject event) { } }); + // Listed once for the import and the export routes, so that both start for the same tenants. + Set startupTenantIds = contextManager.executeAsSystem(() -> startupTenantIds()); + //--IMPORT ROUTES //Source @@ -293,7 +338,7 @@ public boolean isEnabled(EventObject event) { builderReader.setProfileService(profileService); builderReader.setImportConfigurationService(importConfigurationService); // Load configs under each tenant: getAll() alone only sees the current (system) context. - builderReader.setImportConfigurationList(loadConfigurationsAcrossTenants(importConfigurationService)); + builderReader.setImportConfigurationList(loadConfigurationsAcrossTenants(importConfigurationService, startupTenantIds, importTenantsToRetry)); builderReader.setJacksonDataFormat(jacksonDataFormat); builderReader.setAllowedEndpoints(allowedEndpoints); builderReader.setPermittedImportBaseDirs(permittedImportBaseDirs); @@ -323,7 +368,7 @@ public boolean isEnabled(EventObject event) { //Profiles collect ProfileExportCollectRouteBuilder profileExportCollectRouteBuilder = new ProfileExportCollectRouteBuilder(kafkaProps, configType); - profileExportCollectRouteBuilder.setExportConfigurationList(loadConfigurationsAcrossTenants(exportConfigurationService)); + profileExportCollectRouteBuilder.setExportConfigurationList(loadConfigurationsAcrossTenants(exportConfigurationService, startupTenantIds, exportTenantsToRetry)); profileExportCollectRouteBuilder.setExportConfigurationService(exportConfigurationService); profileExportCollectRouteBuilder.setPersistenceService(persistenceService); profileExportCollectRouteBuilder.setAllowedEndpoints(allowedEndpoints); @@ -366,7 +411,8 @@ private void markExportRouteCreationFailed(String configId) { /** {@inheritDoc} */ @Override - public void killExistingRoute(String routeId, boolean fireEvent) throws Exception { + public void killExistingRoute(String tenantId, String configId, boolean fireEvent) throws Exception { + String routeId = RouteIds.of(tenantId, configId); //Active routes Route route = camelContext.getRoute(routeId); if (route != null) { @@ -379,8 +425,8 @@ public void killExistingRoute(String routeId, boolean fireEvent) throws Exceptio /** {@inheritDoc} */ @Override - public void updateProfileImportReaderRoute(String configId, boolean fireEvent) throws Exception { - killExistingRoute(configId, false); + public void updateProfileImportReaderRoute(String tenantId, String configId, boolean fireEvent) throws Exception { + killExistingRoute(tenantId, configId, false); ImportConfiguration importConfiguration = importConfigurationService.load(configId); if (importConfiguration == null) { @@ -404,8 +450,8 @@ public void updateProfileImportReaderRoute(String configId, boolean fireEvent) t /** {@inheritDoc} */ @Override - public void updateProfileExportReaderRoute(String configId, boolean fireEvent) throws Exception { - killExistingRoute(configId, false); + public void updateProfileExportReaderRoute(String tenantId, String configId, boolean fireEvent) throws Exception { + killExistingRoute(tenantId, configId, false); ExportConfiguration exportConfiguration = exportConfigurationService.load(configId); if (exportConfiguration == null) { @@ -518,40 +564,112 @@ public void setPermittedExportBaseDirs(String permittedExportBaseDirs) { } /** - * Loads import/export configurations for every real tenant plus the system tenant. + * Loads import/export configurations for the given tenants. * {@link ImportExportConfigurationService#getAll()} is context-scoped, so a single call * under the system context at startup would miss recurring configs of migrated tenants. + *

A tenant whose configurations cannot be loaded does not block the others: it is added to + * {@code tenantsToRetry} and the refresh timer builds its routes once it can be loaded.

*/ - private List loadConfigurationsAcrossTenants(ImportExportConfigurationService service) { - if (service == null || contextManager == null) { - return new ArrayList<>(); - } + List loadConfigurationsAcrossTenants(ImportExportConfigurationService service, Set tenantIds, + Set tenantsToRetry) { // Startup has no request subject; nest tenant switches under system like other router paths. return contextManager.executeAsSystem(() -> { List all = new ArrayList<>(); - Set tenantIds = new LinkedHashSet<>(); - if (tenantService != null) { + for (String tenantId : tenantIds) { try { - for (Tenant tenant : tenantService.getAllTenants()) { - tenantIds.add(tenant.getItemId()); - } + all.addAll(contextManager.executeAsTenant(tenantId, service::getAll)); } catch (Exception e) { - LOGGER.error("Could not list tenants while loading router configurations", e); + LOGGER.error("Could not load router configurations for tenant {}, its recurring routes are not " + + "started; the load will be retried", tenantId, e); + tenantsToRetry.add(tenantId); } } - tenantIds.add(TenantService.SYSTEM_TENANT); + return all; + }); + } - for (String tenantId : tenantIds) { + /** Every tenant plus the system tenant, or the system tenant alone until tenants can be listed. */ + Set startupTenantIds() { + Set tenantIds = new LinkedHashSet<>(); + try { + for (Tenant tenant : tenantService.getAllTenants()) { + tenantIds.add(tenant.getItemId()); + } + } catch (Exception e) { + LOGGER.error("Could not list tenants, only the recurring routes of the system tenant are started; " + + "the load will be retried", e); + tenantListingToRetry = true; + } + tenantIds.add(TenantService.SYSTEM_TENANT); + return tenantIds; + } + + /** + * Called on every tick of the refresh timer: retries what could not be started at startup, waiting + * twice as long after each unsuccessful attempt, up to {@link #MAX_STARTUP_RETRY_DELAY_MS}. + * + * @param now the current time in milliseconds + */ + void retryStartupLoadsWhenDue(long now) { + if (now < nextStartupRetryTime) { + return; + } + if (retryStartupLoads()) { + startupRetryDelay = 0; + } else { + startupRetryDelay = Math.min(Math.max(configsRefreshInterval, startupRetryDelay * 2), MAX_STARTUP_RETRY_DELAY_MS); + LOGGER.warn("Not all recurring routes could be started yet (tenants still to list: {}, import tenants to load: {}, " + + "export tenants to load: {}), next attempt in {} ms", + tenantListingToRetry, importTenantsToRetry, exportTenantsToRetry, startupRetryDelay); + } + nextStartupRetryTime = now + startupRetryDelay; + } + + /** + * Builds the recurring routes that could not be built at startup because tenants could not be listed or + * a tenant's configurations could not be loaded. + * + * @return {@code true} when nothing is left to retry + */ + boolean retryStartupLoads() { + if (!tenantListingToRetry && importTenantsToRetry.isEmpty() && exportTenantsToRetry.isEmpty()) { + return true; + } + contextManager.executeAsSystem(() -> { + if (tenantListingToRetry) { try { - List tenantConfigs = contextManager.executeAsTenant(tenantId, service::getAll); - if (tenantConfigs != null) { - all.addAll(tenantConfigs); + // the system tenant was loaded at startup, the others were not known + for (Tenant tenant : tenantService.getAllTenants()) { + importTenantsToRetry.add(tenant.getItemId()); + exportTenantsToRetry.add(tenant.getItemId()); } + tenantListingToRetry = false; } catch (Exception e) { - LOGGER.error("Could not load router configurations for tenant {}", tenantId, e); + LOGGER.warn("Still cannot list tenants to start their recurring routes", e); } } - return all; + retryStartupLoad(importTenantsToRetry, importConfigurationService); + retryStartupLoad(exportTenantsToRetry, exportConfigurationService); }); + return !tenantListingToRetry && importTenantsToRetry.isEmpty() && exportTenantsToRetry.isEmpty(); + } + + /** + * Hands the configurations of the tenants that can now be loaded to the refresh the timer runs next, + * which builds their routes with its usual retries and failure status. + */ + private void retryStartupLoad(Set tenantsToRetry, ImportExportConfigurationService service) { + for (String tenantId : tenantsToRetry) { + try { + List configs = contextManager.executeAsTenant(tenantId, service::getAll); + for (T config : configs) { + service.requeueForRefresh(tenantId, config.getItemId(), RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED); + } + tenantsToRetry.remove(tenantId); + LOGGER.info("Router configurations of tenant {} loaded, its recurring routes are being started", tenantId); + } catch (Exception e) { + LOGGER.warn("Still cannot load router configurations for tenant {}", tenantId, e); + } + } } } diff --git a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/processor/ExportRouteCompletionProcessor.java b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/processor/ExportRouteCompletionProcessor.java index a45a524e08..0c07861bf1 100644 --- a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/processor/ExportRouteCompletionProcessor.java +++ b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/processor/ExportRouteCompletionProcessor.java @@ -22,6 +22,7 @@ import org.apache.unomi.router.api.RouterConstants; import org.apache.unomi.router.api.RouterUtils; import org.apache.unomi.router.api.services.ImportExportConfigurationService; +import org.apache.unomi.router.api.RouteIds; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -72,7 +73,7 @@ public class ExportRouteCompletionProcessor implements Processor { @Override public void process(Exchange exchange) throws Exception { // We load the conf from ES because we are going to increment the execution number - ExportConfiguration exportConfiguration = exportConfigurationService.load(exchange.getFromRouteId()); + ExportConfiguration exportConfiguration = exportConfigurationService.load(RouteIds.configId(exchange.getFromRouteId())); if (exportConfiguration == null) { LOGGER.warn("Unable to complete export, config cannot not found: {}", exchange.getFromRouteId()); return; diff --git a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/processor/ImportRouteCompletionProcessor.java b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/processor/ImportRouteCompletionProcessor.java index 25784d6c52..0bc58c08eb 100644 --- a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/processor/ImportRouteCompletionProcessor.java +++ b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/processor/ImportRouteCompletionProcessor.java @@ -76,9 +76,13 @@ public void process(Exchange exchange) throws Exception { if (importConfigOneShot != null) { importConfigId = importConfigOneShot.getItemId(); } else { - importConfigId = exchange.getFromRouteId(); + importConfigId = RouteIds.configId(exchange.getFromRouteId()); } ImportConfiguration importConfiguration = importConfigurationService.load(importConfigId); + if (importConfiguration == null) { + LOGGER.error("Unable to complete import, config cannot be found: {}. Its status is left as it is.", importConfigId); + return; + } long successCount = 0; long failureCount = 0; long ignoreCount = 0; diff --git a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/route/ProfileExportCollectRouteBuilder.java b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/route/ProfileExportCollectRouteBuilder.java index bd652f3afb..f7a28bbc1f 100644 --- a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/route/ProfileExportCollectRouteBuilder.java +++ b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/route/ProfileExportCollectRouteBuilder.java @@ -23,6 +23,7 @@ import org.apache.unomi.persistence.spi.PersistenceService; import org.apache.unomi.router.api.EndpointValidator; import org.apache.unomi.router.api.ExportConfiguration; +import org.apache.unomi.router.api.RouteIds; import org.apache.unomi.router.api.RouterConstants; import org.apache.unomi.router.api.services.ImportExportConfigurationService; import org.apache.unomi.router.core.bean.CollectProfileBean; @@ -116,7 +117,7 @@ public void configure() throws Exception { timerString += "&delay=" + (String) exportConfiguration.getProperties().get("delay"); } ProcessorDefinition prDef = from(timerString) - .routeId(exportConfiguration.getItemId())// This allow identification of the route for manual start/stop + .routeId(RouteIds.of(exportConfiguration.getTenantId(), exportConfiguration.getItemId()))// This allow identification of the route for manual start/stop .autoStartup(exportConfiguration.isActive()) .setHeader(RouterConstants.HEADER_TENANT_ID, constant(exportConfiguration.getTenantId())) .bean(collectProfileBean, "extractProfileBySegment(" + exportConfiguration.getProperties().get("segment") + "," + exportConfiguration.getTenantId() + ")") diff --git a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/route/ProfileImportFromSourceRouteBuilder.java b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/route/ProfileImportFromSourceRouteBuilder.java index f9919a670d..e4f3e4e888 100644 --- a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/route/ProfileImportFromSourceRouteBuilder.java +++ b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/route/ProfileImportFromSourceRouteBuilder.java @@ -27,6 +27,7 @@ import org.apache.unomi.api.services.ExecutionContextManager; import org.apache.unomi.router.api.EndpointValidator; import org.apache.unomi.router.api.ImportConfiguration; +import org.apache.unomi.router.api.RouteIds; import org.apache.unomi.router.api.RouterConstants; import org.apache.unomi.router.api.services.ImportExportConfigurationService; import org.apache.unomi.router.api.exceptions.BadProfileDataFormatException; @@ -149,7 +150,7 @@ public void configure() throws Exception { recordEndpointOutcome(importConfiguration, importConfigurationService, refusal, executionContextManager); if (refusal == null) { ProcessorDefinition prDef = from(endpoint) - .routeId(importConfiguration.getItemId())// This allow identification of the route for manual start/stop + .routeId(RouteIds.of(importConfiguration.getTenantId(), importConfiguration.getItemId()))// This allow identification of the route for manual start/stop .autoStartup(importConfiguration.isActive())// Auto-start if the import configuration is set active .shutdownRunningTask(ShutdownRunningTask.CompleteAllTasks) .onCompletion() diff --git a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java index 4f5e52f512..9a180d7da6 100644 --- a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java +++ b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java @@ -16,27 +16,49 @@ */ package org.apache.unomi.router.core.context; +import org.apache.camel.CamelContext; +import org.apache.camel.component.jackson.JacksonDataFormat; +import org.apache.camel.impl.DefaultCamelContext; import org.apache.unomi.api.ExecutionContext; +import org.apache.unomi.api.Item; import org.apache.unomi.api.services.ExecutionContextManager; +import org.apache.unomi.api.services.ProfileService; import org.apache.unomi.api.tenants.Tenant; import org.apache.unomi.api.tenants.TenantService; +import org.apache.unomi.router.api.ExportConfiguration; import org.apache.unomi.router.api.ImportConfiguration; +import org.apache.unomi.router.api.ProfileToImport; import org.apache.unomi.router.api.RouterConstants; import org.apache.unomi.router.api.services.ImportExportConfigurationService; +import org.apache.unomi.router.core.processor.ExportRouteCompletionProcessor; +import org.apache.unomi.router.core.processor.ImportConfigByFileNameProcessor; +import org.apache.unomi.router.core.processor.ImportRouteCompletionProcessor; +import org.apache.unomi.router.core.processor.UnomiStorageProcessor; +import org.apache.unomi.router.api.RouteIds; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TemporaryFolder; -import java.lang.reflect.Method; +import java.io.File; import java.lang.reflect.Proxy; +import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.atomic.AtomicReference; import java.util.function.Supplier; +import java.util.stream.Collectors; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; /** @@ -44,140 +66,392 @@ */ public class RouterCamelContextTenantLoadTest { + private static final Map NO_KAFKA = new HashMap<>(); + + @Rule + public TemporaryFolder tmp = new TemporaryFolder(); + + private final Map> configsByTenant = new HashMap<>(); + private final Map> exportConfigsByTenant = new HashMap<>(); + private final List visitedTenants = new ArrayList<>(); + private String currentTenant; + private String failingTenant; + private boolean tenantListingFails; + + private RouterCamelContext context; + private DefaultCamelContext camelContext; + private TenantScopedService importService; + + @Before + public void setUp() { + context = new RouterCamelContext(); + context.setContextManager(new TenantTrackingContextManager()); + context.setTenantService(tenantService("tenant-a", "tenant-b")); + } + + @After + public void tearDown() throws Exception { + if (camelContext != null) { + camelContext.stop(); + } + } + @Test - @SuppressWarnings("unchecked") - public void loadConfigurationsAcrossTenantsVisitsEveryTenant() throws Exception { - Set visitedTenants = new HashSet<>(); - AtomicReference currentTenant = new AtomicReference<>(); - - ImportConfiguration configA = new ImportConfiguration(); - configA.setItemId("import-a"); - ImportConfiguration configB = new ImportConfiguration(); - configB.setItemId("import-b"); - - ImportExportConfigurationService service = - new ImportExportConfigurationService() { - @Override - public List getAll() { - String tenant = currentTenant.get(); - if ("tenant-a".equals(tenant)) { - return Collections.singletonList(configA); - } - if ("tenant-b".equals(tenant)) { - return Collections.singletonList(configB); - } - return Collections.emptyList(); - } + public void loadsConfigurationsOfEveryTenantIncludingSystem() { + configsByTenant.put("tenant-a", Collections.singletonList(config("import-a", "tenant-a"))); + configsByTenant.put("tenant-b", Collections.singletonList(config("import-b", "tenant-b"))); + configsByTenant.put(TenantService.SYSTEM_TENANT, Collections.singletonList(config("import-system", TenantService.SYSTEM_TENANT))); + Set tenantsToRetry = new HashSet<>(); - @Override - public ImportConfiguration load(String configId) { - return null; - } + List loaded = context.loadConfigurationsAcrossTenants( + new TenantScopedService<>(configsByTenant), context.startupTenantIds(), tenantsToRetry); - @Override - public ImportConfiguration save(ImportConfiguration configuration, boolean updateRunningRoute) { - return configuration; - } + assertEquals(Arrays.asList("import-a", "import-b", "import-system"), ids(loaded)); + assertEquals(Arrays.asList("tenant-a", "tenant-b", TenantService.SYSTEM_TENANT), visitedTenants); + assertTrue(tenantsToRetry.isEmpty()); + assertNull("tenant context must be restored after the load", currentTenant); + } - @Override - public void delete(String configId) { - } + /** + * The startup path itself: after a restart, the routes of every tenant's recurrent configurations + * must exist without anyone saving the configurations again. + */ + @Test + public void startupBuildsTheRecurrentRoutesOfEveryTenant() throws Exception { + RouterCamelContext router = startRouter(); - @Override - public Map> consumeConfigsToBeRefresh() { - return Collections.emptyMap(); - } + for (String tenantId : Arrays.asList("tenant-a", "tenant-b")) { + assertRoutesBuilt(tenantId); + } - @Override - public void requeueForRefresh(String tenantId, String configId, - RouterConstants.CONFIG_CAMEL_REFRESH refreshType) { - } - }; + // Removing one tenant's route must leave the other tenant's route of the same configuration id. + router.killExistingRoute("tenant-a", "crm-import", false); + assertNull(camelContext.getRouteDefinition(RouteIds.of("tenant-a", "crm-import"))); + assertNotNull("tenant-b keeps its route", camelContext.getRouteDefinition(RouteIds.of("tenant-b", "crm-import"))); + } + + @Test + public void tenantWhoseLoadFailsAtStartupDoesNotBlockTheOthersAndIsRetried() throws Exception { + failingTenant = "tenant-a"; + + RouterCamelContext router = startRouter(); + + assertRoutesBuilt("tenant-b"); + assertNull("tenant-a cannot have a route yet", camelContext.getRouteDefinition(RouteIds.of("tenant-a", "crm-import"))); + + assertFalse("tenant-a still cannot be loaded", router.retryStartupLoads()); + router.refreshRoutes(); + assertNull(camelContext.getRouteDefinition(RouteIds.of("tenant-a", "crm-import"))); + + failingTenant = null; + assertTrue("nothing is left to retry", router.retryStartupLoads()); + router.refreshRoutes(); + assertRoutesBuilt("tenant-a"); + assertRoutesBuilt("tenant-b"); + } + + @Test + public void refreshTimerRetriesWhatStartupCouldNotLoad() throws Exception { + failingTenant = "tenant-a"; + RouterCamelContext router = startRouter(); + failingTenant = null; + + // first tick retries the load and hands the configurations to the refresh of the same tick + router.refreshRoutes(); + + assertRoutesBuilt("tenant-a"); + } + + @Test + public void startupRetriesBackOffUntilTheTenantCanBeLoaded() throws Exception { + failingTenant = "tenant-a"; + RouterCamelContext router = startRouter(); + router.setConfigsRefreshInterval(1000); + + router.retryStartupLoadsWhenDue(0); + assertEquals("the first tick retries", 1, retriesOf("tenant-a")); + router.retryStartupLoadsWhenDue(999); + assertEquals("nothing is retried before the delay has passed", 1, retriesOf("tenant-a")); + router.retryStartupLoadsWhenDue(1000); + assertEquals("retried once the delay has passed", 2, retriesOf("tenant-a")); + router.retryStartupLoadsWhenDue(2999); + assertEquals("the delay doubles after an unsuccessful attempt", 2, retriesOf("tenant-a")); + router.retryStartupLoadsWhenDue(3000); + assertEquals(3, retriesOf("tenant-a")); + + // keep failing until the delay stops growing + long now = 3000; + for (int attempt = 0; attempt < 10; attempt++) { + now += RouterCamelContext.MAX_STARTUP_RETRY_DELAY_MS; + router.retryStartupLoadsWhenDue(now); + } + int retries = retriesOf("tenant-a"); + router.retryStartupLoadsWhenDue(now + RouterCamelContext.MAX_STARTUP_RETRY_DELAY_MS - 1); + assertEquals("the delay is capped", retries, retriesOf("tenant-a")); + + failingTenant = null; + router.retryStartupLoadsWhenDue(now + RouterCamelContext.MAX_STARTUP_RETRY_DELAY_MS); + router.refreshRoutes(); + assertRoutesBuilt("tenant-a"); + + retries = retriesOf("tenant-a"); + router.retryStartupLoadsWhenDue(now + 10 * RouterCamelContext.MAX_STARTUP_RETRY_DELAY_MS); + assertEquals("nothing is retried once every tenant is started", retries, retriesOf("tenant-a")); + } + + @Test + public void startupRetryKeepsARefreshAlreadyQueuedForAConfiguration() throws Exception { + failingTenant = "tenant-a"; + RouterCamelContext router = startRouter(); + failingTenant = null; + // the configuration is deleted while its tenant is still waiting for the startup retry + importService.requeueForRefresh("tenant-a", "crm-import", RouterConstants.CONFIG_CAMEL_REFRESH.REMOVED); + + router.refreshRoutes(); + + assertNull("the queued removal must win over the startup retry", + camelContext.getRouteDefinition(RouteIds.of("tenant-a", "crm-import"))); + assertNotNull(camelContext.getRouteDefinition(RouteIds.of("tenant-a", "crm-export"))); + } + + @Test + public void tenantListingFailureAtStartupIsRetried() throws Exception { + tenantListingFails = true; + + RouterCamelContext router = startRouter(); + + assertNull("no tenant is known yet", camelContext.getRouteDefinition(RouteIds.of("tenant-a", "crm-import"))); + + assertFalse("tenants still cannot be listed", router.retryStartupLoads()); + + tenantListingFails = false; + assertTrue("nothing is left to retry", router.retryStartupLoads()); + router.refreshRoutes(); + assertRoutesBuilt("tenant-a"); + assertRoutesBuilt("tenant-b"); + } + + @Test + public void nothingIsReloadedOnceEveryTenantIsStarted() throws Exception { + RouterCamelContext router = startRouter(); + visitedTenants.clear(); + + router.refreshRoutes(); + + assertTrue("the refresh timer must not reload tenants on every tick", visitedTenants.isEmpty()); + } + + /** + * Starts a router over two tenants that both own a recurrent import and a recurrent export, under the same + * configuration ids: ids are only unique within a tenant. + */ + private RouterCamelContext startRouter() throws Exception { + File importRoot = tmp.newFolder("permitted-import"); + File exportRoot = tmp.newFolder("permitted-export"); + for (String tenantId : Arrays.asList("tenant-a", "tenant-b")) { + configsByTenant.put(tenantId, Collections.singletonList( + recurrentImport("crm-import", tenantId, new File(importRoot, tenantId)))); + exportConfigsByTenant.put(tenantId, Collections.singletonList( + recurrentExport("crm-export", tenantId, new File(exportRoot, tenantId)))); + } + + camelContext = new DefaultCamelContext(); + RouterCamelContext router = new RouterCamelContext() { + @Override + CamelContext createCamelContext() { + return camelContext; + } + }; + router.setContextManager(new TenantTrackingContextManager()); + router.setTenantService(tenantService("tenant-a", "tenant-b")); + importService = new TenantScopedService<>(configsByTenant); + router.setImportConfigurationService(importService); + router.setExportConfigurationService(new TenantScopedService<>(exportConfigsByTenant)); + router.setProfileService(noOpProfileService()); + router.setKafkaProps(NO_KAFKA); + router.setConfigType(RouterConstants.CONFIG_TYPE_NOBROKER); + router.setJacksonDataFormat(new JacksonDataFormat(ProfileToImport.class)); + router.setAllowedEndpoints("file"); + router.setPermittedImportBaseDirs(importRoot.getAbsolutePath()); + router.setPermittedExportBaseDirs(exportRoot.getAbsolutePath()); + router.setUnomiStorageProcessor(new UnomiStorageProcessor()); + router.setImportRouteCompletionProcessor(new ImportRouteCompletionProcessor()); + router.setExportRouteCompletionProcessor(new ExportRouteCompletionProcessor()); + router.setImportConfigByFileNameProcessor(new ImportConfigByFileNameProcessor()); + router.initCamel(); + return router; + } - TenantService tenantService = (TenantService) Proxy.newProxyInstance( + /** How many times the startup retry tried to load the tenant's import configurations. */ + private int retriesOf(String tenantId) { + return importService.loadAttempts.getOrDefault(tenantId, 0) - 1; + } + + private void assertRoutesBuilt(String tenantId) { + for (String configId : Arrays.asList("crm-import", "crm-export")) { + assertNotNull("the route of " + configId + " must be built for " + tenantId, + camelContext.getRouteDefinition(RouteIds.of(tenantId, configId))); + } + } + + private static ImportConfiguration recurrentImport(String id, String tenantId, File directory) { + assertTrue(directory.mkdirs()); + ImportConfiguration configuration = config(id, tenantId); + configuration.setConfigType(RouterConstants.IMPORT_EXPORT_CONFIG_TYPE_RECURRENT); + // inactive: the route is built but does not start polling during the test + configuration.setActive(false); + configuration.getProperties().put("source", "file://" + directory.getAbsolutePath() + "?fileName=profiles.csv"); + configuration.getProperties().put("mapping", Collections.singletonMap("0", 0)); + return configuration; + } + + private static ExportConfiguration recurrentExport(String id, String tenantId, File directory) { + assertTrue(directory.mkdirs()); + ExportConfiguration configuration = new ExportConfiguration(); + configuration.setItemId(id); + configuration.setTenantId(tenantId); + configuration.setConfigType(RouterConstants.IMPORT_EXPORT_CONFIG_TYPE_RECURRENT); + configuration.setActive(false); + configuration.getProperties().put("destination", "file://" + directory.getAbsolutePath() + "?fileName=profiles.csv"); + configuration.getProperties().put("mapping", Collections.singletonMap("0", "firstName")); + configuration.getProperties().put("segment", "exportSegment"); + configuration.getProperties().put("period", "1m"); + return configuration; + } + + private static ProfileService noOpProfileService() { + return (ProfileService) Proxy.newProxyInstance( + ProfileService.class.getClassLoader(), + new Class[]{ProfileService.class}, + (proxy, method, args) -> Collection.class.isAssignableFrom(method.getReturnType()) + ? Collections.emptyList() : null); + } + + private static List ids(List configs) { + return configs.stream().map(ImportConfiguration::getItemId).collect(Collectors.toList()); + } + + private static ImportConfiguration config(String id, String tenantId) { + ImportConfiguration config = new ImportConfiguration(); + config.setItemId(id); + config.setTenantId(tenantId); + return config; + } + + private TenantService tenantService(String... tenantIds) { + return (TenantService) Proxy.newProxyInstance( TenantService.class.getClassLoader(), new Class[]{TenantService.class}, (proxy, method, args) -> { if ("getAllTenants".equals(method.getName())) { - return Arrays.asList(simpleTenant("tenant-a"), simpleTenant("tenant-b")); - } - if (method.getReturnType() == boolean.class) { - return false; - } - if (method.getReturnType() == long.class || method.getReturnType() == int.class) { - return 0; + if (tenantListingFails) { + throw new IllegalStateException("persistence unavailable"); + } + List tenants = new ArrayList<>(); + for (String tenantId : tenantIds) { + Tenant tenant = new Tenant(); + tenant.setItemId(tenantId); + tenants.add(tenant); + } + return tenants; } - return null; + throw new UnsupportedOperationException(method.getName()); }); + } - ExecutionContextManager contextManager = new ExecutionContextManager() { - @Override - public ExecutionContext getCurrentContext() { - return null; - } + /** Returns only the configurations of the tenant the context manager is currently switched to. */ + private class TenantScopedService implements ImportExportConfigurationService { + private final Map> configs; + private final Map> toRefresh = new HashMap<>(); + /** Calls to getAll() per tenant, the startup load included. */ + private final Map loadAttempts = new HashMap<>(); - @Override - public void setCurrentContext(ExecutionContext context) { - } + TenantScopedService(Map> configs) { + this.configs = configs; + } - @Override - public T executeAsSystem(Supplier operation) { - return operation.get(); + @Override + public List getAll() { + loadAttempts.merge(String.valueOf(currentTenant), 1, Integer::sum); + if (currentTenant != null && currentTenant.equals(failingTenant)) { + throw new IllegalStateException("cannot load configurations of " + currentTenant); } + return configs.getOrDefault(currentTenant, Collections.emptyList()); + } - @Override - public void executeAsSystem(Runnable operation) { - operation.run(); - } + @Override + public T load(String configId) { + return configs.getOrDefault(currentTenant, Collections.emptyList()).stream() + .filter(config -> configId.equals(((Item) config).getItemId())).findFirst().orElse(null); + } - @Override - public T executeAsTenant(String tenantId, Supplier operation) { - visitedTenants.add(tenantId); - currentTenant.set(tenantId); - try { - return operation.get(); - } finally { - currentTenant.set(null); - } - } + @Override + public T save(T configuration, boolean updateRunningRoute) { + return configuration; + } - @Override - public void executeAsTenant(String tenantId, Runnable operation) { - visitedTenants.add(tenantId); - currentTenant.set(tenantId); - try { - operation.run(); - } finally { - currentTenant.set(null); - } - } + @Override + public void delete(String configId) { + } - @Override - public ExecutionContext createContext(String tenantId) { - return null; - } - }; + @Override + public Map> consumeConfigsToBeRefresh() { + Map> consumed = new HashMap<>(toRefresh); + toRefresh.clear(); + return consumed; + } - RouterCamelContext context = new RouterCamelContext(); - context.setContextManager(contextManager); - context.setTenantService(tenantService); + @Override + public void requeueForRefresh(String tenantId, String configId, + RouterConstants.CONFIG_CAMEL_REFRESH refreshType) { + // like the real service: a refresh already queued for the configuration is kept + toRefresh.computeIfAbsent(tenantId, k -> new HashMap<>()).putIfAbsent(configId, refreshType); + } + } - Method method = RouterCamelContext.class.getDeclaredMethod( - "loadConfigurationsAcrossTenants", ImportExportConfigurationService.class); - method.setAccessible(true); - List loaded = (List) method.invoke(context, service); + private class TenantTrackingContextManager implements ExecutionContextManager { + @Override + public ExecutionContext getCurrentContext() { + return null; + } - assertEquals("expected one config per real tenant", 2, loaded.size()); - assertTrue(loaded.stream().anyMatch(c -> "import-a".equals(c.getItemId()))); - assertTrue(loaded.stream().anyMatch(c -> "import-b".equals(c.getItemId()))); - assertTrue(visitedTenants.contains("tenant-a")); - assertTrue(visitedTenants.contains("tenant-b")); - assertTrue(visitedTenants.contains(TenantService.SYSTEM_TENANT)); - } + @Override + public void setCurrentContext(ExecutionContext context) { + } + + @Override + public T executeAsSystem(Supplier operation) { + return operation.get(); + } + + @Override + public void executeAsSystem(Runnable operation) { + operation.run(); + } + + @Override + public T executeAsTenant(String tenantId, Supplier operation) { + visitedTenants.add(tenantId); + String previous = currentTenant; + currentTenant = tenantId; + try { + return operation.get(); + } finally { + currentTenant = previous; + } + } + + @Override + public void executeAsTenant(String tenantId, Runnable operation) { + executeAsTenant(tenantId, () -> { + operation.run(); + return null; + }); + } - private static Tenant simpleTenant(String id) { - Tenant tenant = new Tenant(); - tenant.setItemId(id); - return tenant; + @Override + public ExecutionContext createContext(String tenantId) { + return null; + } } } diff --git a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/processor/RouteCompletionProcessorTest.java b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/processor/RouteCompletionProcessorTest.java new file mode 100644 index 0000000000..6a77ca1975 --- /dev/null +++ b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/processor/RouteCompletionProcessorTest.java @@ -0,0 +1,127 @@ +/* + * 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.unomi.router.core.processor; + +import org.apache.camel.Exchange; +import org.apache.camel.impl.DefaultCamelContext; +import org.apache.camel.impl.DefaultExchange; +import org.apache.unomi.router.api.ExportConfiguration; +import org.apache.unomi.router.api.ImportConfiguration; +import org.apache.unomi.router.api.ProfileToImport; +import org.apache.unomi.router.api.RouterConstants; +import org.apache.unomi.router.api.services.ImportExportConfigurationService; +import org.apache.unomi.router.api.RouteIds; +import org.junit.Test; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.Date; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertSame; + +/** + * The completion processors find the configuration to update from the route the exchange comes from, + * whose id carries the tenant as well as the configuration id. + */ +public class RouteCompletionProcessorTest { + + @Test + public void importCompletionUpdatesTheConfigurationOfTheRoute() throws Exception { + ImportConfiguration configuration = new ImportConfiguration(); + configuration.setItemId("crm:import"); + configuration.setTenantId("tenant-a"); + StoredConfigurations store = new StoredConfigurations<>(); + store.byId.put("crm:import", configuration); + ImportRouteCompletionProcessor processor = new ImportRouteCompletionProcessor(); + processor.setImportConfigurationService(store); + processor.setExecutionsHistorySize(5); + + Exchange exchange = exchangeFromRoute(RouteIds.of("tenant-a", "crm:import")); + exchange.setProperty("CamelSplitSize", 1); + exchange.getIn().setBody(new ArrayList<>(Collections.singletonList(new ProfileToImport()))); + processor.process(exchange); + + assertSame("the configuration of the route must be the one saved", configuration, store.lastSaved); + assertEquals(RouterConstants.CONFIG_STATUS_COMPLETE_SUCCESS, configuration.getStatus()); + } + + @Test + public void exportCompletionUpdatesTheConfigurationOfTheRoute() throws Exception { + ExportConfiguration configuration = new ExportConfiguration(); + configuration.setItemId("crm:export"); + configuration.setTenantId("tenant-a"); + StoredConfigurations store = new StoredConfigurations<>(); + store.byId.put("crm:export", configuration); + ExportRouteCompletionProcessor processor = new ExportRouteCompletionProcessor(); + processor.setExportConfigurationService(store); + processor.setExecutionsHistorySize(5); + + Exchange exchange = exchangeFromRoute(RouteIds.of("tenant-a", "crm:export")); + exchange.setProperty("CamelSplitSize", 1); + processor.process(exchange); + + assertSame("the configuration of the route must be the one saved", configuration, store.lastSaved); + assertEquals(RouterConstants.CONFIG_STATUS_COMPLETE_SUCCESS, configuration.getStatus()); + } + + private static Exchange exchangeFromRoute(String routeId) { + Exchange exchange = new DefaultExchange(new DefaultCamelContext()); + exchange.setFromRouteId(routeId); + exchange.setProperty("CamelCreatedTimestamp", new Date()); + return exchange; + } + + /** Hands out the configurations it holds by configuration id, as the real service does within a tenant. */ + private static final class StoredConfigurations implements ImportExportConfigurationService { + + private final Map byId = new HashMap<>(); + private T lastSaved; + + @Override + public List getAll() { + return new ArrayList<>(byId.values()); + } + + @Override + public T load(String configId) { + return byId.get(configId); + } + + @Override + public T save(T configuration, boolean updateRunningRoute) { + lastSaved = configuration; + return configuration; + } + + @Override + public void delete(String configId) { + } + + @Override + public Map> consumeConfigsToBeRefresh() { + return Collections.emptyMap(); + } + + @Override + public void requeueForRefresh(String tenantId, String configId, RouterConstants.CONFIG_CAMEL_REFRESH refreshType) { + } + } +} diff --git a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/FileEndpointContainmentTest.java b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/FileEndpointContainmentTest.java index 8d328f04d8..cee4c10473 100644 --- a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/FileEndpointContainmentTest.java +++ b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/FileEndpointContainmentTest.java @@ -20,6 +20,7 @@ import org.apache.camel.impl.DefaultCamelContext; import org.apache.unomi.router.api.ExportConfiguration; import org.apache.unomi.router.api.ImportConfiguration; +import org.apache.unomi.router.api.RouteIds; import org.apache.unomi.router.api.ProfileToImport; import org.apache.unomi.router.api.RouterConstants; import org.apache.unomi.router.api.services.ImportExportConfigurationService; @@ -44,6 +45,7 @@ import static org.apache.unomi.router.core.route.RouterTestFixtures.recurrentExport; import static org.apache.unomi.router.core.route.RouterTestFixtures.recurrentImport; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -461,7 +463,7 @@ public void importRouteIsRefusedWhenSourceIsTheSharedBaseOrAnotherTenantsDirecto assertRouteRefused("shared-base", "the shared base directory is not a tenant directory"); assertRouteRefused("other-tenant", "acme may not read beta's directory"); assertRouteBuilt("own-tenant", "acme may read its own directory"); - assertRouteBuilt("beta-tenant", "beta may read its own directory"); + assertRouteBuilt("beta", "beta-tenant", "beta may read its own directory"); } @Test @@ -767,20 +769,26 @@ public void importRouteIsRefusedWhenTheTenantIdNamesNoDirectoryOfItsOwn() throws // Fixtures // --------------------------------------------------------------------------------------------- - private void assertRouteBuilt(String routeId, String why) { - assertNotNull("no route was built for configuration '" + routeId + "', although " + why, - camelContext.getRouteDefinition(routeId)); + private void assertRouteBuilt(String configId, String why) { + assertRouteBuilt(TENANT, configId, why); + } + + private void assertRouteBuilt(String tenantId, String configId, String why) { + assertNotNull("no route was built for configuration '" + configId + "' of " + tenantId + ", although " + why, + camelContext.getRouteDefinition(RouteIds.of(tenantId, configId))); } /** The endpoint URI the route was actually built on, which is not the one that was configured. */ - private String builtEndpointUri(String routeId) { - assertRouteBuilt(routeId, "there is no endpoint to look at otherwise"); - return camelContext.getRouteDefinition(routeId).getInputs().get(0).getUri(); + private String builtEndpointUri(String configId) { + assertRouteBuilt(configId, "there is no endpoint to look at otherwise"); + return camelContext.getRouteDefinition(RouteIds.of(TENANT, configId)).getInputs().get(0).getUri(); } - private void assertRouteRefused(String routeId, String why) { - assertNull("a route was built for configuration '" + routeId + "', although " + why, - camelContext.getRouteDefinition(routeId)); + /** No tenant may have got a route out of the configuration: route ids carry the tenant. */ + private void assertRouteRefused(String configId, String why) { + assertFalse("a route was built for configuration '" + configId + "', although " + why, + camelContext.getRouteDefinitions().stream() + .anyMatch(route -> configId.equals(RouteIds.configId(route.getId())))); } private void addImportRoutes(ImportConfiguration... configurations) throws Exception { diff --git a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RefusedConfigurationStatusTest.java b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RefusedConfigurationStatusTest.java index 8185ea88b1..fd6f574975 100644 --- a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RefusedConfigurationStatusTest.java +++ b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RefusedConfigurationStatusTest.java @@ -21,6 +21,7 @@ import org.apache.unomi.api.services.ExecutionContextManager; import org.apache.unomi.router.api.ExportConfiguration; import org.apache.unomi.router.api.ImportConfiguration; +import org.apache.unomi.router.api.RouteIds; import org.apache.unomi.router.api.ProfileToImport; import org.apache.unomi.router.api.RouterConstants; import org.apache.unomi.router.api.services.ImportExportConfigurationService; @@ -117,7 +118,7 @@ public void aRefusedImportConfigurationIsRecordedAsFailed() throws Exception { addImportRoutes(configuration); - assertNull("no route should have been built", camelContext.getRouteDefinition("out-of-bounds")); + assertNull("no route should have been built", camelContext.getRouteDefinition(RouteIds.of(TENANT, "out-of-bounds"))); assertEquals("the configuration should be recorded as failed", RouterConstants.CONFIG_STATUS_INVALID_ENDPOINT, configuration.getStatus()); assertTrue("the configuration should have been saved so the failure is visible", @@ -130,7 +131,7 @@ public void aRefusedExportConfigurationIsRecordedAsFailed() throws Exception { addExportRoutes(configuration); - assertNull("no route should have been built", camelContext.getRouteDefinition("out-of-bounds")); + assertNull("no route should have been built", camelContext.getRouteDefinition(RouteIds.of(TENANT, "out-of-bounds"))); assertEquals("the configuration should be recorded as failed", RouterConstants.CONFIG_STATUS_INVALID_ENDPOINT, configuration.getStatus()); assertTrue("the configuration should have been saved so the failure is visible", @@ -178,9 +179,9 @@ public void aContextThatCannotBeSwitchedDoesNotCostTheBatchItsOtherRoutes() thro addImportRoutes(noSystemSubject, recurrentImport(fileUri(arbitraryDir, "?fileName=profiles.csv")), inBoundsImport("in-bounds")); - assertNull("the refused configuration still gets no route", camelContext.getRouteDefinition("out-of-bounds")); + assertNull("the refused configuration still gets no route", camelContext.getRouteDefinition(RouteIds.of(TENANT, "out-of-bounds"))); assertNotNull("failing to switch context must not cost the other configurations their routes", - camelContext.getRouteDefinition("in-bounds")); + camelContext.getRouteDefinition(RouteIds.of(TENANT, "in-bounds"))); } @Test @@ -190,9 +191,9 @@ public void aStoreThatCannotRecordTheRefusalDoesNotCostTheBatchItsOtherRoutes() addImportRoutes(recurrentImport(fileUri(arbitraryDir, "?fileName=profiles.csv")), inBoundsImport("in-bounds")); - assertNull("the refused configuration still gets no route", camelContext.getRouteDefinition("out-of-bounds")); + assertNull("the refused configuration still gets no route", camelContext.getRouteDefinition(RouteIds.of(TENANT, "out-of-bounds"))); assertNotNull("failing to record the refusal must not cost the other configurations their routes", - camelContext.getRouteDefinition("in-bounds")); + camelContext.getRouteDefinition(RouteIds.of(TENANT, "in-bounds"))); } @Test diff --git a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RouteIdsTest.java b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RouteIdsTest.java new file mode 100644 index 0000000000..f3efe9ecfc --- /dev/null +++ b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RouteIdsTest.java @@ -0,0 +1,47 @@ +/* + * 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.unomi.router.core.route; + +import org.apache.unomi.router.api.RouteIds; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; + +public class RouteIdsTest { + + @Test + public void sameConfigurationIdInTwoTenantsGetsTwoRouteIds() { + assertNotEquals(RouteIds.of("tenant-a", "crm-import"), RouteIds.of("tenant-b", "crm-import")); + } + + @Test + public void configurationIdIsRecoveredWhateverItContains() { + assertEquals("crm-import", RouteIds.configId(RouteIds.of("tenant-a", "crm-import"))); + assertEquals("a:b:c", RouteIds.configId(RouteIds.of("tenant_a", "a:b:c"))); + } + + @Test + public void configurationWithoutTenantBelongsToTheSystemTenant() { + assertEquals(RouteIds.of("system", "crm-import"), RouteIds.of(null, "crm-import")); + } + + @Test + public void routeNotBuiltFromAConfigurationKeepsItsId() { + assertEquals("ONE_SHOT_ROUTE", RouteIds.configId("ONE_SHOT_ROUTE")); + } +} diff --git a/itests/src/test/java/org/apache/unomi/itests/BaseIT.java b/itests/src/test/java/org/apache/unomi/itests/BaseIT.java index 05d784176f..74c97b0af6 100644 --- a/itests/src/test/java/org/apache/unomi/itests/BaseIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/BaseIT.java @@ -63,6 +63,7 @@ import org.apache.unomi.rest.authentication.RestAuthenticationConfig; import org.apache.unomi.router.api.ExportConfiguration; import org.apache.unomi.router.api.IRouterCamelContext; +import org.apache.unomi.router.api.RouteIds; import org.apache.unomi.router.api.ImportConfiguration; import org.apache.unomi.router.api.services.ImportExportConfigurationService; import org.apache.unomi.schema.api.SchemaService; @@ -684,6 +685,11 @@ public Option[] config() { // crash recovery repeatedly (mis)reclaims them, logging "Lock verification failed... after // CAS" every checker tick. Widen it for ITs so heartbeats/checker never starve. editConfigurationFilePut("etc/custom.system.properties", "org.apache.unomi.scheduler.thread.poolSize", "10"), + // The scheduled purge runs seconds after startup for every tenant and would delete the + // years-old profiles, sessions and events the migration ITs restore from snapshots before + // they are checked. Purge ITs call the purge methods directly, so switch the schedule off. + editConfigurationFilePut("etc/custom.system.properties", "org.apache.unomi.profile.purge.inactiveTime", "-1"), + editConfigurationFilePut("etc/custom.system.properties", "org.apache.unomi.monthly.index.purge.existTime", "-1"), // The router's base directories have to be set here rather than in // etc/org.apache.unomi.router.cfg: Karaf's configuration plugin overrides every @@ -1795,10 +1801,12 @@ protected CamelContext getCamelContext() { * Checks if a Camel route with the given route ID exists. * Uses official Camel API: CamelContext.getRoute(String routeId) * - * @param routeId The route ID to check (typically the import configuration itemId) + * @param tenantId The tenant owning the import/export configuration + * @param configId The import/export configuration ID; the route ID is built from both * @return true if the route exists, false otherwise */ - protected boolean camelRouteExists(String routeId) { + protected boolean camelRouteExists(String tenantId, String configId) { + String routeId = RouteIds.of(tenantId, configId); CamelContext camelContext = getCamelContext(); if (camelContext == null) { return false; @@ -1812,10 +1820,12 @@ protected boolean camelRouteExists(String routeId) { * Uses Camel 2.23.1 API directly. * Returns ServiceStatus enum: Started, Stopped, Suspended, etc. * - * @param routeId The route ID to get status for + * @param tenantId The tenant owning the import/export configuration + * @param configId The import/export configuration ID; the route ID is built from both * @return The route status, or null if route doesn't exist or status unavailable */ - protected ServiceStatus getCamelRouteStatus(String routeId) { + protected ServiceStatus getCamelRouteStatus(String tenantId, String configId) { + String routeId = RouteIds.of(tenantId, configId); CamelContext camelContext = getCamelContext(); if (camelContext == null) { return null; @@ -1839,11 +1849,12 @@ protected ServiceStatus getCamelRouteStatus(String routeId) { * Checks if a Camel route is started (running). * Uses official Camel API to check route status. * - * @param routeId The route ID to check + * @param tenantId The tenant owning the import/export configuration + * @param configId The import/export configuration ID; the route ID is built from both * @return true if the route exists and is started, false otherwise */ - protected boolean isCamelRouteStarted(String routeId) { - ServiceStatus status = getCamelRouteStatus(routeId); + protected boolean isCamelRouteStarted(String tenantId, String configId) { + ServiceStatus status = getCamelRouteStatus(tenantId, configId); return status != null && status.isStarted(); } @@ -1851,10 +1862,12 @@ protected boolean isCamelRouteStarted(String routeId) { * Gets detailed information about a Camel route including status, endpoints, and configuration. * Uses Camel 2.23.1 API to inspect route definitions and endpoints. * - * @param routeId The route ID to get information for + * @param tenantId The tenant owning the import/export configuration + * @param configId The import/export configuration ID; the route ID is built from both * @return A string describing the route status, endpoints, and configuration, or error message if route doesn't exist */ - protected String getCamelRouteInfo(String routeId) { + protected String getCamelRouteInfo(String tenantId, String configId) { + String routeId = RouteIds.of(tenantId, configId); CamelContext camelContext = getCamelContext(); if (camelContext == null) { return "CamelContext not available"; @@ -1869,7 +1882,7 @@ protected String getCamelRouteInfo(String routeId) { info.append("Route '").append(routeId).append("': "); // Get route status using official API - ServiceStatus status = getCamelRouteStatus(routeId); + ServiceStatus status = getCamelRouteStatus(tenantId, configId); if (status != null) { info.append("status=").append(status); } else { @@ -1929,22 +1942,24 @@ protected String getCamelRouteInfo(String routeId) { * Waits for a Camel route to be created and started. * This is useful for tests that need to verify the route was created by the timer. * - * @param routeId The route ID to wait for + * @param tenantId The tenant owning the import/export configuration + * @param configId The import/export configuration ID; the route ID is built from both * @param timeoutMs Timeout in milliseconds between retries * @param maxRetries Maximum number of retries * @return true if the route exists and is started, false if timeout * @throws InterruptedException if interrupted */ - protected boolean waitForCamelRouteStarted(String routeId, int timeoutMs, int maxRetries) throws InterruptedException { + protected boolean waitForCamelRouteStarted(String tenantId, String configId, int timeoutMs, int maxRetries) throws InterruptedException { + String routeId = RouteIds.of(tenantId, configId); for (int i = 0; i < maxRetries; i++) { - if (isCamelRouteStarted(routeId)) { - String routeInfo = getCamelRouteInfo(routeId); + if (isCamelRouteStarted(tenantId, configId)) { + String routeInfo = getCamelRouteInfo(tenantId, configId); LOGGER.debug("Camel route '{}' is started. {}", routeId, routeInfo); return true; } Thread.sleep(timeoutMs); } - String routeInfo = getCamelRouteInfo(routeId); + String routeInfo = getCamelRouteInfo(tenantId, configId); LOGGER.warn("Camel route '{}' did not start within timeout. {}", routeId, routeInfo); return false; } @@ -1966,10 +1981,8 @@ protected java.util.Map getAllCamelRoutesWithStatus() { // In Camel 2.23.1, Route has getId() method String routeId = route.getId(); if (routeId != null) { - ServiceStatus status = getCamelRouteStatus(routeId); - if (status != null) { - routes.put(routeId, status); - } + // a route that exists in the context counts as started, as for getCamelRouteStatus(tenantId, configId) + routes.put(routeId, ServiceStatus.Started); } } } catch (Exception e) { diff --git a/itests/src/test/java/org/apache/unomi/itests/ProfileImportActorsIT.java b/itests/src/test/java/org/apache/unomi/itests/ProfileImportActorsIT.java index 9e31c291a0..b7e3bed031 100644 --- a/itests/src/test/java/org/apache/unomi/itests/ProfileImportActorsIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/ProfileImportActorsIT.java @@ -97,10 +97,10 @@ public void testImportActors() throws InterruptedException { // Gate: wait for the Camel route to be created and started before polling for results. // The timer fires every ~1 second to pick up config changes, so 15 retries (15 s) is generous. keepTrying("Camel route '" + itemId + "' did not start — timer may not have fired or route creation failed", - () -> isCamelRouteStarted(itemId), + () -> isCamelRouteStarted(TEST_TENANT_ID, itemId), started -> started, 1000, 15); - System.out.println("==== Camel Route Status: " + getCamelRouteInfo(itemId) + " ===="); + System.out.println("==== Camel Route Status: " + getCamelRouteInfo(TEST_TENANT_ID, itemId) + " ===="); // Wait for data to be processed. Force an ES index refresh before each search attempt so // profiles written by UnomiStorageProcessor are visible to the Search API immediately. diff --git a/itests/src/test/java/org/apache/unomi/itests/ProfileImportSurfersIT.java b/itests/src/test/java/org/apache/unomi/itests/ProfileImportSurfersIT.java index 6495cbe95f..f52c39e830 100644 --- a/itests/src/test/java/org/apache/unomi/itests/ProfileImportSurfersIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/ProfileImportSurfersIT.java @@ -98,9 +98,9 @@ public void testImportSurfers() throws InterruptedException { // Wait for Camel route to be created and started (the timer runs every 1 second to process config refreshes) // This gives us visibility into what Camel is doing instead of just waiting for results - boolean routeStarted = waitForCamelRouteStarted(itemId1, 1000, 10); + boolean routeStarted = waitForCamelRouteStarted(TEST_TENANT_ID, itemId1, 1000, 10); if (routeStarted) { - String routeInfo = getCamelRouteInfo(itemId1); + String routeInfo = getCamelRouteInfo(TEST_TENANT_ID, itemId1); LOGGER.info("Camel Route Status: {}", routeInfo); } else { LOGGER.warn("Camel Route '{}' was not started within timeout", itemId1); @@ -166,9 +166,9 @@ public void testImportSurfers() throws InterruptedException { LOGGER.info("ProfileImportSurfersOverwriteIT setup successfully."); // Wait for Camel route to be created and started - boolean routeStarted2 = waitForCamelRouteStarted(itemId2, 1000, 10); + boolean routeStarted2 = waitForCamelRouteStarted(TEST_TENANT_ID, itemId2, 1000, 10); if (routeStarted2) { - String routeInfo = getCamelRouteInfo(itemId2); + String routeInfo = getCamelRouteInfo(TEST_TENANT_ID, itemId2); LOGGER.info("Camel Route Status: {}", routeInfo); } else { LOGGER.warn("Camel Route '{}' was not started within timeout", itemId2); @@ -229,9 +229,9 @@ public void testImportSurfers() throws InterruptedException { LOGGER.info("ProfileImportSurfersDeleteIT setup successfully."); // Wait for Camel route to be created and started - boolean routeStarted3 = waitForCamelRouteStarted(itemId3, 1000, 10); + boolean routeStarted3 = waitForCamelRouteStarted(TEST_TENANT_ID, itemId3, 1000, 10); if (routeStarted3) { - String routeInfo = getCamelRouteInfo(itemId3); + String routeInfo = getCamelRouteInfo(TEST_TENANT_ID, itemId3); LOGGER.info("Camel Route Status: {}", routeInfo); } else { LOGGER.warn("Camel Route '{}' was not started within timeout", itemId3); diff --git a/services-common/src/main/java/org/apache/unomi/services/common/cache/AbstractMultiTypeCachingService.java b/services-common/src/main/java/org/apache/unomi/services/common/cache/AbstractMultiTypeCachingService.java index 8cf4441573..72cd8981b5 100644 --- a/services-common/src/main/java/org/apache/unomi/services/common/cache/AbstractMultiTypeCachingService.java +++ b/services-common/src/main/java/org/apache/unomi/services/common/cache/AbstractMultiTypeCachingService.java @@ -23,6 +23,7 @@ import org.apache.unomi.api.services.cache.CacheableTypeConfig; import org.apache.unomi.api.services.cache.MultiTypeCacheService; import org.apache.unomi.api.tasks.ScheduledTask; +import org.apache.unomi.api.tasks.TaskExecutor; import org.apache.unomi.api.tenants.AuditService; import org.apache.unomi.api.tenants.Tenant; import org.apache.unomi.api.tenants.TenantService; @@ -551,6 +552,50 @@ protected Set getTenants() { return tenants; } + /** + * Runs an action once under every tenant, system tenant included. A tenant whose run fails is logged + * and does not stop the others; once the thread is interrupted the remaining tenants are skipped. + * + * @param taskName short description of the work, used in log messages + * @param action the work to run under each tenant's execution context + * @return identifiers of the tenants for which the action failed or was skipped, empty when all succeeded + */ + protected List executeForEachTenant(String taskName, Runnable action) { + List failedTenants = new ArrayList<>(); + for (String tenantId : getTenants()) { + if (Thread.currentThread().isInterrupted()) { + logger.warn("Interrupted during {}, skipping tenant {}", taskName, tenantId); + failedTenants.add(tenantId); + continue; + } + try { + contextManager.executeAsTenant(tenantId, action); + } catch (Exception e) { + logger.error("Error during {} for tenant {}", taskName, tenantId, e); + failedTenants.add(tenantId); + } + } + return failedTenants; + } + + /** + * Runs a scheduled task's action once under every tenant and reports the outcome to the scheduler: + * the task completes when every tenant was processed, and fails naming the tenants that were not. + * A failed task is retried by the scheduler for every tenant, so the action must be safe to repeat. + * + * @param taskName short description of the work, used in log messages and in the failure reported + * @param action the work to run under each tenant's execution context + * @param callback the scheduler callback of the running task + */ + protected void executeForEachTenant(String taskName, Runnable action, TaskExecutor.TaskStatusCallback callback) { + List failedTenants = executeForEachTenant(taskName, action); + if (failedTenants.isEmpty()) { + callback.complete(); + } else { + callback.fail("The " + taskName + " failed for tenants " + failedTenants); + } + } + /** * Loads predefined JSON items from bundle resources for all configured types. * diff --git a/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java index 04469d94c6..64dba4db36 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java @@ -367,26 +367,16 @@ public void execute(ScheduledTask task, TaskExecutor.TaskStatusCallback callback // Run under each tenant so persistence sees that tenant's data // (system context alone only matches tenantId=system). - for (String tenantId : getTenants()) { - try { - contextManager.executeAsTenant(tenantId, () -> { - purgeProfiles(purgeProfileInactiveTime, purgeProfileExistTime); - if (purgeSessionExistTime > 0) { - purgeSessionItems(purgeSessionExistTime); - } - if (purgeEventExistTime > 0) { - purgeEventItems(purgeEventExistTime); - } - return null; - }); - } catch (Throwable t) { - LOGGER.error("Error while purging profiles, sessions, or events for tenant {}", - tenantId, t); + executeForEachTenant("purge of profiles, sessions and events", () -> { + purgeProfiles(purgeProfileInactiveTime, purgeProfileExistTime); + if (purgeSessionExistTime > 0) { + purgeSessionItems(purgeSessionExistTime); } - } + if (purgeEventExistTime > 0) { + purgeEventItems(purgeEventExistTime); + } + }, callback); LOGGER.info("Purge: executed in {} ms", System.currentTimeMillis() - purgeStartTime); - - callback.complete(); } catch (Throwable t) { // During shutdown, services may be unavailable - only log if not shutting down LOGGER.error("Error while purging profiles, sessions, or events", t); diff --git a/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java index a7aebce9f0..b7ec4b73c6 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java @@ -197,13 +197,7 @@ public void postConstruct() { .withFixedDelay() .withSimpleExecutor(() -> contextManager.executeAsSystem(() -> { // Flush in-memory stats for every tenant; system context alone only persists system. - for (String tenantId : getTenants()) { - try { - contextManager.executeAsTenant(tenantId, () -> syncRuleStatistics()); - } catch (Throwable t) { - LOGGER.error("Error syncing rule statistics for tenant {}", tenantId, t); - } - } + executeForEachTenant("rule statistics sync", () -> syncRuleStatistics()); })) .schedule(); @@ -830,16 +824,9 @@ private void syncRuleStatistics() { persistenceService.save(ruleStatistics, null, true); } } - - // Also sync system tenant statistics if needed - if (!SYSTEM_TENANT.equals(currentTenant)) { - Map systemStats = getRuleStatisticsForTenant(SYSTEM_TENANT); - for (RuleStatistics ruleStatistics : systemStats.values()) { - if (!tenantStats.containsKey(ruleStatistics.getItemId())) { - tenantStats.put(ruleStatistics.getItemId(), ruleStatistics); - } - } - } + // System-tenant statistics are synced by the system tenant's own run and read through the + // fallback in getRuleStatistics()/getAllRuleStatistics(). Copying them into this tenant's map + // would have this tenant's next sync save them under its own tenantId and drop their counts. } public void bind(ServiceReference serviceReference) { diff --git a/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java index 2604a53da3..5e59638cfd 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java @@ -1556,19 +1556,9 @@ public void execute(ScheduledTask task, TaskExecutor.TaskStatusCallback callback LOGGER.info("Running scheduled task to recalculate segments and scoring that contains date relative conditions..."); // Run under each tenant so rules/segments/profiles of real tenants are seen // (system context alone only matches tenantId=system). - for (String tenantId : getTenants()) { - try { - contextManager.executeAsTenant(tenantId, () -> { - recalculatePastEventConditions(); - return null; - }); - } catch (Throwable t) { - LOGGER.error("Error recalculating date-relative segments/scoring for tenant {}", - tenantId, t); - } - } + executeForEachTenant("recalculation of date-relative segments and scoring", + () -> recalculatePastEventConditions(), callback); LOGGER.info("...Finished recalculate segments and scoring that contains date relative conditions in {}ms. ", System.currentTimeMillis() - currentTimeMillis); - callback.complete(); } catch (Throwable t) { LOGGER.error("Error while updating profiles for segments and scoring that contains date relative conditions", t); callback.fail(t.getMessage()); diff --git a/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java b/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java index 9a52c79ce9..b64c15e133 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java @@ -17,7 +17,11 @@ package org.apache.unomi.services.impl; import org.apache.unomi.api.ExecutionContext; +import org.apache.unomi.api.Metadata; import org.apache.unomi.api.Profile; +import org.apache.unomi.api.conditions.Condition; +import org.apache.unomi.api.rules.RuleStatistics; +import org.apache.unomi.api.segments.Segment; import org.apache.unomi.api.services.EventService; import org.apache.unomi.api.services.RulesService; import org.apache.unomi.api.services.SegmentService; @@ -45,20 +49,22 @@ import org.osgi.framework.Bundle; import java.util.Collections; +import java.util.Date; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Supplier; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; @@ -66,9 +72,11 @@ /** * UNOMI-1000: background jobs must visit every tenant (not only {@code system}). *

- * These tests fail on the 4.0.0 RC behaviour where purge / segment recalculation / - * rule-statistics refresh ran solely under {@code executeAsSystem}, so persistence - * only ever saw {@code tenantId=system}. + * These tests fail when purge / segment recalculation / rule-statistics refresh run + * solely under {@code executeAsSystem}, where persistence only ever sees + * {@code tenantId=system}. They also cover what running per tenant adds: + * one tenant's failure is reported without stopping the others, and system-owned + * statistics stay in the system tenant. */ @ExtendWith(MockitoExtension.class) @MockitoSettings(strictness = Strictness.LENIENT) @@ -124,43 +132,72 @@ public void tearDown() throws Exception { } @Test - public void profilePurgeTaskRunsUnderEveryTenantAndRestoresContext() throws Exception { - Set visitedTenants = ConcurrentHashMap.newKeySet(); + public void profilePurgeTaskPurgesEveryTenantAndRestoresContext() throws Exception { + AtomicReference purgeExecutor = new AtomicReference<>(); ExecutionContextManagerImpl spiedContext = spy(executionContextManager); - trackTenantVisits(spiedContext, visitedTenants); + newProfileService(spiedContext, capturingScheduler("profile-purge", purgeExecutor)); + saveOldProfile(TENANT_A, "a-old"); + saveOldProfile(TENANT_B, "b-old"); - AtomicReference purgeExecutor = new AtomicReference<>(); - SchedulerServiceImpl spiedScheduler = spy(schedulerService); - captureExecutor(spiedScheduler, "profile-purge", purgeExecutor); + ExecutionContext before = spiedContext.getCurrentContext(); + String beforeTenant = before != null ? before.getTenantId() : null; - ProfileServiceImpl profileService = new ProfileServiceImpl(); - profileService.setBundleContext(bundleContext); - profileService.setPersistenceService(persistenceService); - profileService.setDefinitionsService(definitionsService); - profileService.setContextManager(spiedContext); - profileService.setSchedulerService(spiedScheduler); - profileService.setCacheService(multiTypeCacheService); - profileService.setTenantService(tenantService); - profileService.setSegmentService(mock(SegmentService.class)); - profileService.setPurgeProfileExistTime(30); - profileService.setPurgeProfileInterval(1); - profileService.postConstruct(); + assertNull(runExecutor(purgeExecutor.get()), "purge task should complete"); + persistenceService.refresh(); + + assertNull(loadProfile(TENANT_A, "a-old"), "purge must remove tenant1's old profile"); + assertNull(loadProfile(TENANT_B, "b-old"), "purge must remove tenant2's old profile"); + + ExecutionContext after = spiedContext.getCurrentContext(); + String afterTenant = after != null ? after.getTenantId() : null; + assertEquals(beforeTenant, afterTenant, "tenant context must be restored after the purge task"); + } - assertNotNull(purgeExecutor.get(), "profile-purge executor must be registered"); + @Test + public void profilePurgeFailureInOneTenantIsReportedAndDoesNotStopTheOthers() throws Exception { + AtomicReference purgeExecutor = new AtomicReference<>(); + ExecutionContextManagerImpl spiedContext = spy(executionContextManager); + newProfileService(spiedContext, capturingScheduler("profile-purge", purgeExecutor)); + failTenant(spiedContext, TENANT_A); + saveOldProfile(TENANT_A, "a-old"); + saveOldProfile(TENANT_B, "b-old"); ExecutionContext before = spiedContext.getCurrentContext(); String beforeTenant = before != null ? before.getTenantId() : null; - runExecutor(purgeExecutor.get()); + String failure = runExecutor(purgeExecutor.get()); + persistenceService.refresh(); - assertTrue(visitedTenants.contains(TENANT_A), "purge must visit tenant1, visited=" + visitedTenants); - assertTrue(visitedTenants.contains(TENANT_B), "purge must visit tenant2, visited=" + visitedTenants); - assertTrue(visitedTenants.contains(TenantService.SYSTEM_TENANT), - "purge must also visit system, visited=" + visitedTenants); + assertNotNull(failure, "a tenant failure must fail the task instead of being reported as success"); + assertTrue(failure.contains(TENANT_A), "failure must name the tenant: " + failure); + assertFalse(failure.contains(TENANT_B), "failure must not name a tenant that succeeded: " + failure); + assertNotNull(loadProfile(TENANT_A, "a-old"), "the failing tenant keeps its data"); + assertNull(loadProfile(TENANT_B, "b-old"), "the other tenant must still be purged"); ExecutionContext after = spiedContext.getCurrentContext(); String afterTenant = after != null ? after.getTenantId() : null; - assertEquals(beforeTenant, afterTenant, "tenant context must be restored after the purge task"); + assertEquals(beforeTenant, afterTenant, "tenant context must be restored after a tenant failure"); + } + + @Test + public void profilePurgeInterruptedBeforeItsTenantsIsNotReportedAsDone() throws Exception { + AtomicReference purgeExecutor = new AtomicReference<>(); + newProfileService(executionContextManager, capturingScheduler("profile-purge", purgeExecutor)); + saveOldProfile(TENANT_A, "a-old"); + + String failure; + Thread.currentThread().interrupt(); + try { + failure = runExecutor(purgeExecutor.get()); + } finally { + Thread.interrupted(); + } + + assertNotNull(failure, "tenants skipped by an interruption must not be reported as purged"); + for (String tenantId : new String[]{TENANT_A, TENANT_B, TenantService.SYSTEM_TENANT}) { + assertTrue(failure.contains(tenantId), "every skipped tenant must be named: " + failure); + } + assertNotNull(loadProfile(TENANT_A, "a-old"), "an interrupted purge must not have run"); } @Test @@ -168,66 +205,89 @@ public void segmentDateRecalculationTaskRunsUnderEveryTenant() throws Exception Set visitedTenants = ConcurrentHashMap.newKeySet(); ExecutionContextManagerImpl spiedContext = spy(executionContextManager); trackTenantVisits(spiedContext, visitedTenants); - AtomicReference segmentExecutor = new AtomicReference<>(); - SchedulerServiceImpl spiedScheduler = spy(schedulerService); - captureExecutor(spiedScheduler, "segment-date-recalculation", segmentExecutor); + newSegmentService(spiedContext, capturingScheduler("segment-date-recalculation", segmentExecutor)); - RulesService rulesService = mock(RulesService.class); - when(rulesService.getAllRules()).thenReturn(Collections.emptyList()); + assertNull(runExecutor(segmentExecutor.get()), "segment recalculation should complete"); - SegmentServiceImpl segmentService = new SegmentServiceImpl(); - segmentService.setBundleContext(bundleContext); - segmentService.setPersistenceService(persistenceService); - segmentService.setDefinitionsService(definitionsService); - segmentService.setContextManager(spiedContext); - segmentService.setSchedulerService(spiedScheduler); - segmentService.setCacheService(multiTypeCacheService); - segmentService.setTenantService(tenantService); - segmentService.setRulesService(rulesService); - segmentService.setEventService(mock(EventService.class)); - segmentService.setTracerService(TestHelper.createTracerService()); - segmentService.postConstruct(); + assertEquals(Set.of(TENANT_A, TENANT_B, TenantService.SYSTEM_TENANT), visitedTenants); + } + + @Test + public void segmentDateRecalculationTaskUpdatesProfilesOfEveryTenant() throws Exception { + AtomicReference segmentExecutor = new AtomicReference<>(); + SegmentServiceImpl segmentService = newSegmentService(executionContextManager, + capturingScheduler("segment-date-recalculation", segmentExecutor)); + saveInactiveProfilesAndTheirSegments(segmentService); + assertFalse(loadProfile(TENANT_A, "inactive-profile").getSegments().contains("inactive-" + TENANT_A), + "the profile must not be in the segment before the task runs"); - assertNotNull(segmentExecutor.get(), "segment-date-recalculation executor must be registered"); - runExecutor(segmentExecutor.get()); + assertNull(runExecutor(segmentExecutor.get()), "segment recalculation should complete"); + persistenceService.refresh(); - assertTrue(visitedTenants.contains(TENANT_A), "segment recalc must visit tenant1, visited=" + visitedTenants); - assertTrue(visitedTenants.contains(TENANT_B), "segment recalc must visit tenant2, visited=" + visitedTenants); + assertEquals(Set.of("inactive-" + TENANT_A), loadProfile(TENANT_A, "inactive-profile").getSegments(), + "tenant1's profile must enter tenant1's date-relative segment only"); + assertEquals(Set.of("inactive-" + TENANT_B), loadProfile(TENANT_B, "inactive-profile").getSegments(), + "tenant2's profile must enter tenant2's date-relative segment only"); } @Test - public void ruleStatisticsRefreshVisitsEveryTenant() throws Exception { - Set visitedTenants = ConcurrentHashMap.newKeySet(); + public void segmentDateRecalculationFailureInOneTenantIsReportedAndDoesNotStopTheOthers() throws Exception { ExecutionContextManagerImpl spiedContext = spy(executionContextManager); - trackTenantVisits(spiedContext, visitedTenants); + AtomicReference segmentExecutor = new AtomicReference<>(); + SegmentServiceImpl segmentService = newSegmentService(spiedContext, + capturingScheduler("segment-date-recalculation", segmentExecutor)); + saveInactiveProfilesAndTheirSegments(segmentService); + failTenant(spiedContext, TENANT_B); + + String failure = runExecutor(segmentExecutor.get()); + persistenceService.refresh(); + assertNotNull(failure, "a tenant failure must fail the task instead of being reported as success"); + assertTrue(failure.contains(TENANT_B), "failure must name the tenant: " + failure); + assertFalse(failure.contains(TENANT_A), "failure must not name a tenant that succeeded: " + failure); + assertEquals(Set.of("inactive-" + TENANT_A), loadProfile(TENANT_A, "inactive-profile").getSegments(), + "the other tenant must still be recalculated"); + assertTrue(loadProfile(TENANT_B, "inactive-profile").getSegments().isEmpty(), + "the failing tenant's profiles are left as they were"); + } + + @Test + public void ruleStatisticsRefreshPersistsEachTenantsOwnStatistics() throws Exception { AtomicReference statsExecutor = new AtomicReference<>(); - SchedulerServiceImpl spiedScheduler = spy(schedulerService); - captureExecutor(spiedScheduler, "rules-statistics-refresh", statsExecutor); + RulesServiceImpl rulesService = newRulesService(capturingScheduler("rules-statistics-refresh", statsExecutor)); + saveRuleStatistics(TENANT_A, "a-rule", 10); + // getRuleStatistics() hands out the live in-memory statistics the rule engine increments + RuleStatistics liveStats = executionContextManager.executeAsTenant(TENANT_A, () -> rulesService.getRuleStatistics("a-rule")); + liveStats.setLocalExecutionCount(4); - RulesServiceImpl rulesService = new RulesServiceImpl(); - rulesService.setBundleContext(bundleContext); - rulesService.setPersistenceService(persistenceService); - rulesService.setDefinitionsService(definitionsService); - rulesService.setContextManager(spiedContext); - rulesService.setSchedulerService(spiedScheduler); - rulesService.setCacheService(multiTypeCacheService); - rulesService.setTenantService(tenantService); - rulesService.setEventService(mock(EventService.class)); - rulesService.setActionExecutorDispatcher(mock(org.apache.unomi.services.actions.ActionExecutorDispatcher.class)); - rulesService.setTracerService(TestHelper.createTracerService()); - rulesService.setRulesRefreshInterval(60000); - rulesService.setRulesStatisticsRefreshInterval(60000); - rulesService.postConstruct(); + assertNull(runExecutor(statsExecutor.get()), "statistics refresh should complete"); + persistenceService.refresh(); - assertNotNull(statsExecutor.get(), "rules-statistics-refresh executor must be registered"); - runExecutor(statsExecutor.get()); + assertEquals(14L, loadRuleStatistics(TENANT_A, "a-rule").getExecutionCount(), + "tenant1's pending executions must be flushed to tenant1's statistics"); + assertNull(loadRuleStatistics(TENANT_B, "a-rule"), "tenant2 must not receive tenant1's statistics"); + } - assertTrue(visitedTenants.contains(TENANT_A), "stats refresh must visit tenant1, visited=" + visitedTenants); - assertTrue(visitedTenants.contains(TENANT_B), "stats refresh must visit tenant2, visited=" + visitedTenants); - assertTrue(visitedTenants.contains(TenantService.SYSTEM_TENANT), - "stats refresh must visit system, visited=" + visitedTenants); + @Test + public void ruleStatisticsRefreshKeepsSystemStatisticsInTheSystemTenant() throws Exception { + AtomicReference statsExecutor = new AtomicReference<>(); + RulesServiceImpl rulesService = newRulesService(capturingScheduler("rules-statistics-refresh", statsExecutor)); + saveRuleStatistics(TenantService.SYSTEM_TENANT, "system-rule", 5); + RuleStatistics liveStats = executionContextManager.executeAsSystem(() -> rulesService.getRuleStatistics("system-rule")); + + // Several ticks: a tenant run must never pick up the system statistics a previous tick exposed to it. + for (int tick = 1; tick <= 3; tick++) { + liveStats.setLocalExecutionCount(liveStats.getLocalExecutionCount() + 1); + assertNull(runExecutor(statsExecutor.get()), "statistics refresh should complete"); + persistenceService.refresh(); + + assertEquals(5L + tick, loadRuleStatistics(TenantService.SYSTEM_TENANT, "system-rule").getExecutionCount(), + "system rule executions must be flushed to the system statistics on tick " + tick); + assertNull(loadRuleStatistics(TENANT_A, "system-rule"), "tenant1 must not get a copy of system statistics"); + assertNull(loadRuleStatistics(TENANT_B, "system-rule"), "tenant2 must not get a copy of system statistics"); + assertEquals(TenantService.SYSTEM_TENANT, liveStats.getTenantId(), "system statistics must stay in the system tenant"); + } } @Test @@ -253,44 +313,120 @@ public void systemContextPersistenceStillOnlySeesSystemTenant() { }); } - @Test - public void purgeForOneTenantDoesNotRemoveAnotherTenantsProfiles() { + private ProfileServiceImpl newProfileService(ExecutionContextManagerImpl contextManager, SchedulerServiceImpl scheduler) { ProfileServiceImpl profileService = new ProfileServiceImpl(); profileService.setBundleContext(bundleContext); profileService.setPersistenceService(persistenceService); profileService.setDefinitionsService(definitionsService); - profileService.setContextManager(executionContextManager); - profileService.setSchedulerService(schedulerService); + profileService.setContextManager(contextManager); + profileService.setSchedulerService(scheduler); profileService.setCacheService(multiTypeCacheService); profileService.setTenantService(tenantService); profileService.setSegmentService(mock(SegmentService.class)); + profileService.setPurgeProfileExistTime(30); + profileService.setPurgeProfileInterval(1); profileService.postConstruct(); + return profileService; + } - executionContextManager.executeAsTenant(TENANT_A, () -> { - Profile old = new Profile("a-old"); - old.setProperty("firstVisit", new java.util.Date(0)); - persistenceService.save(old); - return null; - }); - executionContextManager.executeAsTenant(TENANT_B, () -> { - Profile old = new Profile("b-old"); - old.setProperty("firstVisit", new java.util.Date(0)); + private SegmentServiceImpl newSegmentService(ExecutionContextManagerImpl contextManager, SchedulerServiceImpl scheduler) { + RulesService rulesService = mock(RulesService.class); + when(rulesService.getAllRules()).thenReturn(Collections.emptyList()); + + SegmentServiceImpl segmentService = new SegmentServiceImpl(); + segmentService.setBundleContext(bundleContext); + segmentService.setPersistenceService(persistenceService); + segmentService.setDefinitionsService(definitionsService); + segmentService.setContextManager(contextManager); + segmentService.setSchedulerService(scheduler); + segmentService.setCacheService(multiTypeCacheService); + segmentService.setTenantService(tenantService); + segmentService.setRulesService(rulesService); + segmentService.setEventService(mock(EventService.class)); + segmentService.setTracerService(TestHelper.createTracerService()); + segmentService.postConstruct(); + return segmentService; + } + + private RulesServiceImpl newRulesService(SchedulerServiceImpl scheduler) { + RulesServiceImpl rulesService = new RulesServiceImpl(); + rulesService.setBundleContext(bundleContext); + rulesService.setPersistenceService(persistenceService); + rulesService.setDefinitionsService(definitionsService); + rulesService.setContextManager(executionContextManager); + rulesService.setSchedulerService(scheduler); + rulesService.setCacheService(multiTypeCacheService); + rulesService.setTenantService(tenantService); + rulesService.setEventService(mock(EventService.class)); + rulesService.setActionExecutorDispatcher(mock(org.apache.unomi.services.actions.ActionExecutorDispatcher.class)); + rulesService.setTracerService(TestHelper.createTracerService()); + rulesService.setRulesRefreshInterval(60000); + rulesService.setRulesStatisticsRefreshInterval(60000); + rulesService.postConstruct(); + return rulesService; + } + + /** + * Gives each tenant a date-relative segment, then a profile matching it. The profile is saved after the + * segment was defined, straight to persistence: only the scheduled recalculation can put it in the + * segment, as when time makes a profile match. + */ + private void saveInactiveProfilesAndTheirSegments(SegmentServiceImpl segmentService) { + for (String tenantId : new String[]{TENANT_A, TENANT_B}) { + executionContextManager.executeAsTenant(tenantId, () -> { + segmentService.setSegmentDefinition(inactiveSinceSegment("inactive-" + tenantId)); + Profile inactive = new Profile("inactive-profile"); + inactive.setProperty("lastVisit", new Date(0)); + persistenceService.save(inactive); + }); + } + persistenceService.refresh(); + } + + /** A segment of the profiles whose last visit is more than 30 days old: its membership changes as time passes. */ + private Segment inactiveSinceSegment(String segmentId) { + Metadata metadata = new Metadata(); + metadata.setId(segmentId); + metadata.setName(segmentId); + metadata.setScope("systemscope"); + metadata.setEnabled(true); + + Condition condition = new Condition(definitionsService.getConditionType("profilePropertyCondition")); + condition.setParameter("propertyName", "properties.lastVisit"); + condition.setParameter("comparisonOperator", "lessThanOrEqualTo"); + condition.setParameter("propertyValueDateExpr", "now-30d"); + + Segment segment = new Segment(); + segment.setItemId(segmentId); + segment.setMetadata(metadata); + segment.setCondition(condition); + return segment; + } + + private void saveOldProfile(String tenantId, String profileId) { + executionContextManager.executeAsTenant(tenantId, () -> { + Profile old = new Profile(profileId); + old.setProperty("firstVisit", new Date(0)); persistenceService.save(old); - return null; }); persistenceService.refresh(); + } - executionContextManager.executeAsTenant(TENANT_A, () -> { - profileService.purgeProfiles(0, 1); - return null; + private Profile loadProfile(String tenantId, String profileId) { + return executionContextManager.executeAsTenant(tenantId, () -> persistenceService.load(profileId, Profile.class)); + } + + private void saveRuleStatistics(String tenantId, String ruleId, long executionCount) { + executionContextManager.executeAsTenant(tenantId, () -> { + RuleStatistics statistics = new RuleStatistics(ruleId); + statistics.setExecutionCount(executionCount); + persistenceService.save(statistics); }); persistenceService.refresh(); + } - executionContextManager.executeAsTenant(TENANT_A, () -> - assertNull(persistenceService.load("a-old", Profile.class))); - executionContextManager.executeAsTenant(TENANT_B, () -> - assertNotNull(persistenceService.load("b-old", Profile.class), - "tenant B profiles must stay when purge runs only for tenant A")); + private RuleStatistics loadRuleStatistics(String tenantId, String ruleId) { + return executionContextManager.executeAsTenant(tenantId, () -> persistenceService.load(ruleId, RuleStatistics.class)); } @SuppressWarnings("unchecked") @@ -301,19 +437,34 @@ private static void trackTenantVisits(ExecutionContextManagerImpl spiedContext, }).when(spiedContext).executeAsTenant(any(String.class), any(Supplier.class)); } - private static void captureExecutor(SchedulerServiceImpl spiedScheduler, String taskType, - AtomicReference target) { + /** Makes every later switch into the given tenant fail, as if that tenant's work had thrown. */ + @SuppressWarnings("unchecked") + private static void failTenant(ExecutionContextManagerImpl spiedContext, String tenantId) { + doThrow(new IllegalStateException("simulated failure for " + tenantId)) + .when(spiedContext).executeAsTenant(eq(tenantId), any(Supplier.class)); + } + + /** + * Returns a scheduler that hands the executor registered for the given task type to the test instead of + * registering it: only the test runs that task, the scheduler's own threads cannot run it concurrently. + */ + private SchedulerServiceImpl capturingScheduler(String taskType, AtomicReference target) { + SchedulerServiceImpl spiedScheduler = spy(schedulerService); doAnswer(invocation -> { TaskExecutor executor = invocation.getArgument(0); if (taskType.equals(executor.getTaskType())) { target.set(executor); + return null; } return invocation.callRealMethod(); }).when(spiedScheduler).registerTaskExecutor(any(TaskExecutor.class)); + return spiedScheduler; } - private static void runExecutor(TaskExecutor executor) throws Exception { - CountDownLatch done = new CountDownLatch(1); + /** Runs the executor to its end and returns the failure it reported, or {@code null} when it completed. */ + private String runExecutor(TaskExecutor executor) throws Exception { + assertNotNull(executor, "the task executor must be registered"); + AtomicReference done = new AtomicReference<>(false); AtomicReference failure = new AtomicReference<>(); executor.execute(mock(ScheduledTask.class), new TaskExecutor.TaskStatusCallback() { @Override @@ -330,16 +481,16 @@ public void updateStatusDetails(Map details) { @Override public void complete() { - done.countDown(); + done.set(true); } @Override public void fail(String error) { failure.set(error); - done.countDown(); + done.set(true); } }); - assertTrue(done.await(30, TimeUnit.SECONDS), "task should finish"); - assertNull(failure.get(), "task should not fail: " + failure.get()); + assertTrue(done.get(), "task should report its outcome before returning"); + return failure.get(); } } From 2fdd99954a693600738e43ad1c2020c84462b8b1 Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Fri, 9 Oct 2026 08:47:52 +0200 Subject: [PATCH 3/6] UNOMI-1000: Keep one tenant from blocking route refreshes of the others A tenant that cannot be switched to during a route refresh no longer drops the refreshes already taken off the queue for the other tenants, nor skips the export pass of that tick. Also quiet the startup retry (stack trace once per tenant), stop the purge and segment tasks from logging "executed" after a tenant failed, log a missing export configuration as an error like the import one, and tighten two tests that could pass for the wrong reason. Co-Authored-By: Claude Opus 5.5 --- .../core/context/RouterCamelContext.java | 26 +++++++++++-- .../ExportRouteCompletionProcessor.java | 2 +- .../RouterCamelContextTenantLoadTest.java | 39 ++++++++++++++++--- .../java/org/apache/unomi/itests/BaseIT.java | 2 +- .../AbstractMultiTypeCachingService.java | 8 ++-- .../impl/profiles/ProfileServiceImpl.java | 5 ++- .../impl/segments/SegmentServiceImpl.java | 5 ++- .../impl/BackgroundTenantTasksTest.java | 6 +++ 8 files changed, 74 insertions(+), 19 deletions(-) diff --git a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java index 53e2ba1706..d240e69554 100644 --- a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java +++ b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java @@ -55,6 +55,7 @@ import java.util.*; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; +import java.util.function.Supplier; /** * The main Camel context manager for the Unomi Router component. @@ -105,6 +106,7 @@ public class RouterCamelContext implements IRouterCamelContext { private final Set importTenantsToRetry = ConcurrentHashMap.newKeySet(); private final Set exportTenantsToRetry = ConcurrentHashMap.newKeySet(); private volatile boolean tenantListingToRetry; + private final Set tenantsWithLoggedLoadFailure = ConcurrentHashMap.newKeySet(); static final long MAX_STARTUP_RETRY_DELAY_MS = 60_000; private long startupRetryDelay; private long nextStartupRetryTime; @@ -219,7 +221,7 @@ void refreshRoutes() { for (Map.Entry> tenantImportConfigsToRefresh : tenantsImportConfigsToRefresh.entrySet()) { String tenantId = tenantImportConfigsToRefresh.getKey(); - contextManager.executeAsTenant(tenantId, () -> { + refreshRoutesOfTenant(tenantId, () -> { try { for (Map.Entry importConfigToRefresh : tenantImportConfigsToRefresh.getValue().entrySet()) { String configId = importConfigToRefresh.getKey(); @@ -257,7 +259,7 @@ void refreshRoutes() { Map> tenantsExportConfigsToRefresh = exportConfigurationService.consumeConfigsToBeRefresh(); for (Map.Entry> tenantExportConfigsToRefresh : tenantsExportConfigsToRefresh.entrySet()) { String tenantId = tenantExportConfigsToRefresh.getKey(); - contextManager.executeAsTenant(tenantId, () -> { + refreshRoutesOfTenant(tenantId, () -> { try { for (Map.Entry exportConfigToRefresh : tenantExportConfigsToRefresh.getValue().entrySet()) { String configId = exportConfigToRefresh.getKey(); @@ -296,6 +298,15 @@ void refreshRoutes() { } } + /** Runs the refresh of one tenant's routes; a tenant that cannot be switched to must not keep the others from being refreshed. */ + private void refreshRoutesOfTenant(String tenantId, Supplier refresh) { + try { + contextManager.executeAsTenant(tenantId, refresh); + } catch (Exception e) { + LOGGER.error("Could not refresh the camel routes of tenant {}", tenantId, e); + } + } + /** Creates the Camel context the routes are added to; a seam for running the route setup outside OSGi. */ CamelContext createCamelContext() { return new OsgiDefaultCamelContext(bundleContext); @@ -582,6 +593,7 @@ List loadConfigurationsAcrossTenants(ImportExportConfigurationService LOGGER.error("Could not load router configurations for tenant {}, its recurring routes are not " + "started; the load will be retried", tenantId, e); tenantsToRetry.add(tenantId); + tenantsWithLoggedLoadFailure.add(tenantId); } } return all; @@ -645,7 +657,8 @@ boolean retryStartupLoads() { } tenantListingToRetry = false; } catch (Exception e) { - LOGGER.warn("Still cannot list tenants to start their recurring routes", e); + // the stack trace was logged at startup + LOGGER.warn("Still cannot list tenants to start their recurring routes: {}", e.toString()); } } retryStartupLoad(importTenantsToRetry, importConfigurationService); @@ -668,7 +681,12 @@ private void retryStartupLoad(Set tenantsToRetry, Impor tenantsToRetry.remove(tenantId); LOGGER.info("Router configurations of tenant {} loaded, its recurring routes are being started", tenantId); } catch (Exception e) { - LOGGER.warn("Still cannot load router configurations for tenant {}", tenantId, e); + if (tenantsWithLoggedLoadFailure.add(tenantId)) { + LOGGER.warn("Still cannot load router configurations for tenant {}", tenantId, e); + } else { + // the stack trace was logged with the first failure + LOGGER.warn("Still cannot load router configurations for tenant {}: {}", tenantId, e.toString()); + } } } } diff --git a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/processor/ExportRouteCompletionProcessor.java b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/processor/ExportRouteCompletionProcessor.java index 0c07861bf1..199ac69f29 100644 --- a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/processor/ExportRouteCompletionProcessor.java +++ b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/processor/ExportRouteCompletionProcessor.java @@ -75,7 +75,7 @@ public void process(Exchange exchange) throws Exception { // We load the conf from ES because we are going to increment the execution number ExportConfiguration exportConfiguration = exportConfigurationService.load(RouteIds.configId(exchange.getFromRouteId())); if (exportConfiguration == null) { - LOGGER.warn("Unable to complete export, config cannot not found: {}", exchange.getFromRouteId()); + LOGGER.error("Unable to complete export, config cannot be found: {}. Its status is left as it is.", exchange.getFromRouteId()); return; } diff --git a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java index 9a180d7da6..518cd5a9d6 100644 --- a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java +++ b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java @@ -49,6 +49,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -81,6 +82,8 @@ public class RouterCamelContextTenantLoadTest { private RouterCamelContext context; private DefaultCamelContext camelContext; private TenantScopedService importService; + private TenantScopedService exportService; + private String unswitchableTenant; @Before public void setUp() { @@ -209,11 +212,30 @@ public void startupRetryKeepsARefreshAlreadyQueuedForAConfiguration() throws Exc router.refreshRoutes(); - assertNull("the queued removal must win over the startup retry", - camelContext.getRouteDefinition(RouteIds.of("tenant-a", "crm-import"))); + assertEquals("the queued removal must win over the startup retry", + RouterConstants.CONFIG_CAMEL_REFRESH.REMOVED, importService.lastConsumed.get("tenant-a").get("crm-import")); + assertNull(camelContext.getRouteDefinition(RouteIds.of("tenant-a", "crm-import"))); assertNotNull(camelContext.getRouteDefinition(RouteIds.of("tenant-a", "crm-export"))); } + @Test + public void tenantThatCannotBeSwitchedToDoesNotKeepTheOthersFromBeingRefreshed() throws Exception { + RouterCamelContext router = startRouter(); + router.killExistingRoute("tenant-b", "crm-import", false); + router.killExistingRoute("tenant-b", "crm-export", false); + // tenant-a is refreshed first and cannot be switched to, as a tenant that was just deleted + for (TenantScopedService service : Arrays.asList(importService, exportService)) { + String configId = service == importService ? "crm-import" : "crm-export"; + service.requeueForRefresh("tenant-a", configId, RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED); + service.requeueForRefresh("tenant-b", configId, RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED); + } + unswitchableTenant = "tenant-a"; + + router.refreshRoutes(); + + assertRoutesBuilt("tenant-b"); + } + @Test public void tenantListingFailureAtStartupIsRetried() throws Exception { tenantListingFails = true; @@ -266,7 +288,8 @@ CamelContext createCamelContext() { router.setTenantService(tenantService("tenant-a", "tenant-b")); importService = new TenantScopedService<>(configsByTenant); router.setImportConfigurationService(importService); - router.setExportConfigurationService(new TenantScopedService<>(exportConfigsByTenant)); + exportService = new TenantScopedService<>(exportConfigsByTenant); + router.setExportConfigurationService(exportService); router.setProfileService(noOpProfileService()); router.setKafkaProps(NO_KAFKA); router.setConfigType(RouterConstants.CONFIG_TYPE_NOBROKER); @@ -362,7 +385,8 @@ private TenantService tenantService(String... tenantIds) { /** Returns only the configurations of the tenant the context manager is currently switched to. */ private class TenantScopedService implements ImportExportConfigurationService { private final Map> configs; - private final Map> toRefresh = new HashMap<>(); + private final Map> toRefresh = new LinkedHashMap<>(); + private Map> lastConsumed = new LinkedHashMap<>(); /** Calls to getAll() per tenant, the startup load included. */ private final Map loadAttempts = new HashMap<>(); @@ -396,9 +420,9 @@ public void delete(String configId) { @Override public Map> consumeConfigsToBeRefresh() { - Map> consumed = new HashMap<>(toRefresh); + lastConsumed = new LinkedHashMap<>(toRefresh); toRefresh.clear(); - return consumed; + return lastConsumed; } @Override @@ -431,6 +455,9 @@ public void executeAsSystem(Runnable operation) { @Override public T executeAsTenant(String tenantId, Supplier operation) { + if (tenantId.equals(unswitchableTenant)) { + throw new IllegalStateException("cannot switch to " + tenantId); + } visitedTenants.add(tenantId); String previous = currentTenant; currentTenant = tenantId; diff --git a/itests/src/test/java/org/apache/unomi/itests/BaseIT.java b/itests/src/test/java/org/apache/unomi/itests/BaseIT.java index 74c97b0af6..8484ee4d0e 100644 --- a/itests/src/test/java/org/apache/unomi/itests/BaseIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/BaseIT.java @@ -1981,7 +1981,7 @@ protected java.util.Map getAllCamelRoutesWithStatus() { // In Camel 2.23.1, Route has getId() method String routeId = route.getId(); if (routeId != null) { - // a route that exists in the context counts as started, as for getCamelRouteStatus(tenantId, configId) + // every route that exists in the context is assumed to be started routes.put(routeId, ServiceStatus.Started); } } diff --git a/services-common/src/main/java/org/apache/unomi/services/common/cache/AbstractMultiTypeCachingService.java b/services-common/src/main/java/org/apache/unomi/services/common/cache/AbstractMultiTypeCachingService.java index 72cd8981b5..3e2e88addc 100644 --- a/services-common/src/main/java/org/apache/unomi/services/common/cache/AbstractMultiTypeCachingService.java +++ b/services-common/src/main/java/org/apache/unomi/services/common/cache/AbstractMultiTypeCachingService.java @@ -586,14 +586,16 @@ protected List executeForEachTenant(String taskName, Runnable action) { * @param taskName short description of the work, used in log messages and in the failure reported * @param action the work to run under each tenant's execution context * @param callback the scheduler callback of the running task + * @return {@code true} when the task completed, {@code false} when it was reported as failed */ - protected void executeForEachTenant(String taskName, Runnable action, TaskExecutor.TaskStatusCallback callback) { + protected boolean executeForEachTenant(String taskName, Runnable action, TaskExecutor.TaskStatusCallback callback) { List failedTenants = executeForEachTenant(taskName, action); if (failedTenants.isEmpty()) { callback.complete(); - } else { - callback.fail("The " + taskName + " failed for tenants " + failedTenants); + return true; } + callback.fail("The " + taskName + " failed for tenants " + failedTenants); + return false; } /** diff --git a/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java index 64dba4db36..f8fd201b84 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java @@ -367,7 +367,7 @@ public void execute(ScheduledTask task, TaskExecutor.TaskStatusCallback callback // Run under each tenant so persistence sees that tenant's data // (system context alone only matches tenantId=system). - executeForEachTenant("purge of profiles, sessions and events", () -> { + boolean purged = executeForEachTenant("purge of profiles, sessions and events", () -> { purgeProfiles(purgeProfileInactiveTime, purgeProfileExistTime); if (purgeSessionExistTime > 0) { purgeSessionItems(purgeSessionExistTime); @@ -376,7 +376,8 @@ public void execute(ScheduledTask task, TaskExecutor.TaskStatusCallback callback purgeEventItems(purgeEventExistTime); } }, callback); - LOGGER.info("Purge: executed in {} ms", System.currentTimeMillis() - purgeStartTime); + LOGGER.info("Purge: {} in {} ms", purged ? "executed" : "failed for some tenants", + System.currentTimeMillis() - purgeStartTime); } catch (Throwable t) { // During shutdown, services may be unavailable - only log if not shutting down LOGGER.error("Error while purging profiles, sessions, or events", t); diff --git a/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java index 5e59638cfd..98b1772c44 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java @@ -1556,9 +1556,10 @@ public void execute(ScheduledTask task, TaskExecutor.TaskStatusCallback callback LOGGER.info("Running scheduled task to recalculate segments and scoring that contains date relative conditions..."); // Run under each tenant so rules/segments/profiles of real tenants are seen // (system context alone only matches tenantId=system). - executeForEachTenant("recalculation of date-relative segments and scoring", + boolean recalculated = executeForEachTenant("recalculation of date-relative segments and scoring", () -> recalculatePastEventConditions(), callback); - LOGGER.info("...Finished recalculate segments and scoring that contains date relative conditions in {}ms. ", System.currentTimeMillis() - currentTimeMillis); + LOGGER.info("...{} recalculate segments and scoring that contains date relative conditions in {}ms. ", + recalculated ? "Finished" : "Failed for some tenants to", System.currentTimeMillis() - currentTimeMillis); } catch (Throwable t) { LOGGER.error("Error while updating profiles for segments and scoring that contains date relative conditions", t); callback.fail(t.getMessage()); diff --git a/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java b/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java index b64c15e133..9c9985f1f5 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java @@ -207,6 +207,8 @@ public void segmentDateRecalculationTaskRunsUnderEveryTenant() throws Exception trackTenantVisits(spiedContext, visitedTenants); AtomicReference segmentExecutor = new AtomicReference<>(); newSegmentService(spiedContext, capturingScheduler("segment-date-recalculation", segmentExecutor)); + // starting the service already visits every tenant to load its cache + visitedTenants.clear(); assertNull(runExecutor(segmentExecutor.get()), "segment recalculation should complete"); @@ -290,6 +292,10 @@ public void ruleStatisticsRefreshKeepsSystemStatisticsInTheSystemTenant() throws } } + /** + * The tests above rely on the in-memory persistence to keep tenants apart the way the real one does: + * the system context sees the system tenant only, which is why the tasks have to switch tenant. + */ @Test public void systemContextPersistenceStillOnlySeesSystemTenant() { executionContextManager.executeAsTenant(TENANT_A, () -> { From 16c17bbcfe7f2d73c994547151277c42d6c61e94 Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Fri, 9 Oct 2026 09:12:34 +0200 Subject: [PATCH 4/6] UNOMI-1000: Start recurrent routes one by one and cover tenants end to end Router startup adds each recurrent route on its own once the Camel context runs, so a configuration whose route cannot be built is handed to the route refresh instead of stopping the routes of every tenant. Removing a route also removes a definition left by a failed start, and the refresh retries, then gives up on, a tenant it cannot switch to rather than dropping its queued updates. The date-relative recalculation now includes the segments and scoring plans inherited from the system tenant, which apply to every tenant's profiles. The rule statistics sync skips tenants with nothing to sync. Adds BackgroundTenantTasksIT (tenant rule statistics, system segment applied to tenant profiles, same import configuration id in two tenants), unit tests for the per-tenant helper, session and event purge and the router failure paths, and an upgrade note in the 3.0 to 4.0 migration guide. Co-Authored-By: Claude Opus 5.5 --- .../org/apache/unomi/router/api/RouteIds.java | 11 +- .../core/context/RouterCamelContext.java | 289 +++++++++--------- .../RouterCamelContextTenantLoadTest.java | 138 +++++---- .../RouteCompletionProcessorTest.java | 28 ++ .../unomi/router/core/route/RouteIdsTest.java | 11 + .../router/core/route/RouterTestFixtures.java | 4 +- .../java/org/apache/unomi/itests/AllITs.java | 1 + .../unomi/itests/BackgroundTenantTasksIT.java | 201 ++++++++++++ .../unomi/itests/CorePersistenceITs.java | 1 + .../migrations/migrate-3.0-to-4.0.adoc | 20 ++ .../AbstractMultiTypeCachingServiceTest.java | 80 +++++ .../impl/profiles/ProfileServiceImpl.java | 7 +- .../services/impl/rules/RulesServiceImpl.java | 12 +- .../impl/segments/SegmentServiceImpl.java | 19 +- .../impl/BackgroundTenantTasksTest.java | 82 ++++- 15 files changed, 687 insertions(+), 217 deletions(-) create mode 100644 itests/src/test/java/org/apache/unomi/itests/BackgroundTenantTasksIT.java diff --git a/extensions/router/router-api/src/main/java/org/apache/unomi/router/api/RouteIds.java b/extensions/router/router-api/src/main/java/org/apache/unomi/router/api/RouteIds.java index 617e8fc972..c9105606fd 100644 --- a/extensions/router/router-api/src/main/java/org/apache/unomi/router/api/RouteIds.java +++ b/extensions/router/router-api/src/main/java/org/apache/unomi/router/api/RouteIds.java @@ -18,6 +18,8 @@ import org.apache.unomi.api.ExecutionContext; +import java.util.Objects; + /** * Identifiers of the Camel routes built from recurrent import/export configurations. *

A configuration identifier is only unique within its tenant, while Camel route identifiers are @@ -37,9 +39,16 @@ private RouteIds() { * @param tenantId the tenant owning the configuration, the system tenant when {@code null} * @param configId the configuration identifier * @return the Camel route identifier + * @throws IllegalArgumentException if the tenant identifier contains a colon, which would make the + * configuration identifier impossible to recover from the route identifier */ public static String of(String tenantId, String configId) { - return (tenantId != null ? tenantId : ExecutionContext.SYSTEM_TENANT) + SEPARATOR + configId; + Objects.requireNonNull(configId, "configId"); + String owner = tenantId != null ? tenantId : ExecutionContext.SYSTEM_TENANT; + if (owner.indexOf(SEPARATOR) >= 0) { + throw new IllegalArgumentException("Tenant identifier must not contain '" + SEPARATOR + "': " + owner); + } + return owner + SEPARATOR + configId; } /** diff --git a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java index d240e69554..ee4675e72c 100644 --- a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java +++ b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java @@ -18,7 +18,6 @@ import org.apache.camel.CamelContext; import org.apache.camel.Exchange; -import org.apache.camel.Route; import org.apache.camel.component.jackson.JacksonDataFormat; import org.apache.camel.core.osgi.OsgiDefaultCamelContext; import org.apache.camel.management.event.ExchangeCompletedEvent; @@ -55,7 +54,7 @@ import java.util.*; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; -import java.util.function.Supplier; +import java.util.function.Consumer; /** * The main Camel context manager for the Unomi Router component. @@ -102,7 +101,11 @@ public class RouterCamelContext implements IRouterCamelContext { private SecurityService securityService; private TenantService tenantService; - /** Tenants whose recurring routes could not be built at startup; the refresh timer retries them. */ + /** + * Tenants whose recurring routes could not be built at startup; the refresh timer retries them. + * The sets and the flag are filled at startup and emptied by the refresh timer; the two retry times + * below are only read and written by the refresh timer, which runs one tick at a time. + */ private final Set importTenantsToRetry = ConcurrentHashMap.newKeySet(); private final Set exportTenantsToRetry = ConcurrentHashMap.newKeySet(); private volatile boolean tenantListingToRetry; @@ -191,7 +194,10 @@ public void destroy() throws Exception { } //This is to shutdown Camel context //(will stop all routes/components/endpoints etc and clear internal state/cache) - this.camelContext.stop(); + if (camelContext != null) { + // null when init() failed before the context could be started + camelContext.stop(); + } LOGGER.info("Camel context for profile import is shutdown."); } @@ -217,93 +223,78 @@ void refreshRoutes() { LOGGER.warn("Unexpected error while retrying to start the recurring routes that could not be started at startup", e); } try { - Map> tenantsImportConfigsToRefresh = importConfigurationService.consumeConfigsToBeRefresh(); - - for (Map.Entry> tenantImportConfigsToRefresh : tenantsImportConfigsToRefresh.entrySet()) { - String tenantId = tenantImportConfigsToRefresh.getKey(); - refreshRoutesOfTenant(tenantId, () -> { - try { - for (Map.Entry importConfigToRefresh : tenantImportConfigsToRefresh.getValue().entrySet()) { - String configId = importConfigToRefresh.getKey(); - RouterConstants.CONFIG_CAMEL_REFRESH refreshType = importConfigToRefresh.getValue(); - RetryKey retryKey = new RetryKey("import", tenantId, configId); - try { - if (refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED)) { - updateProfileImportReaderRoute(tenantId, configId, true); - routeCreationRetryCount.remove(retryKey); - } else if (refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.REMOVED)) { - killExistingRoute(tenantId, configId, true); - routeCreationRetryCount.remove(retryKey); - } - } catch (Exception e) { - int attempt = routeCreationRetryCount.merge(retryKey, 1, Integer::sum); - if (attempt <= MAX_ROUTE_CREATION_RETRIES) { - LOGGER.error("Refreshing({}) camel route {} failed (attempt {}/{}) — will retry on next tick", - refreshType, configId, attempt, MAX_ROUTE_CREATION_RETRIES, e); - importConfigurationService.requeueForRefresh(tenantId, configId, refreshType); - } else { - LOGGER.error("Refreshing({}) camel route {} failed after {} attempts — giving up", - refreshType, configId, MAX_ROUTE_CREATION_RETRIES, e); - routeCreationRetryCount.remove(retryKey); - markImportRouteCreationFailed(configId); - } - } - } - } catch (Exception e) { - LOGGER.error("Unexpected error while refreshing import/export camel routes for tenant {}", tenantId, e); - } - return null; - }); - } + refreshRoutes("import", importConfigurationService, this::updateProfileImportReaderRoute, this::markImportRouteCreationFailed); + refreshRoutes("export", exportConfigurationService, this::updateProfileExportReaderRoute, this::markExportRouteCreationFailed); + } catch (Exception e) { + LOGGER.error("Unexpected error while refreshing import/export camel routes", e); + } + } - Map> tenantsExportConfigsToRefresh = exportConfigurationService.consumeConfigsToBeRefresh(); - for (Map.Entry> tenantExportConfigsToRefresh : tenantsExportConfigsToRefresh.entrySet()) { - String tenantId = tenantExportConfigsToRefresh.getKey(); - refreshRoutesOfTenant(tenantId, () -> { - try { - for (Map.Entry exportConfigToRefresh : tenantExportConfigsToRefresh.getValue().entrySet()) { - String configId = exportConfigToRefresh.getKey(); - RouterConstants.CONFIG_CAMEL_REFRESH refreshType = exportConfigToRefresh.getValue(); - RetryKey retryKey = new RetryKey("export", tenantId, configId); - try { - if (refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED)) { - updateProfileExportReaderRoute(tenantId, configId, true); - routeCreationRetryCount.remove(retryKey); - } else if (refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.REMOVED)) { - killExistingRoute(tenantId, configId, true); - routeCreationRetryCount.remove(retryKey); - } - } catch (Exception e) { - int attempt = routeCreationRetryCount.merge(retryKey, 1, Integer::sum); - if (attempt <= MAX_ROUTE_CREATION_RETRIES) { - LOGGER.error("Refreshing({}) camel route {} failed (attempt {}/{}) — will retry on next tick", - refreshType, configId, attempt, MAX_ROUTE_CREATION_RETRIES, e); - exportConfigurationService.requeueForRefresh(tenantId, configId, refreshType); - } else { - LOGGER.error("Refreshing({}) camel route {} failed after {} attempts — giving up", - refreshType, configId, MAX_ROUTE_CREATION_RETRIES, e); - routeCreationRetryCount.remove(retryKey); - markExportRouteCreationFailed(configId); - } + /** Rebuilds or removes the routes of the import or of the export configurations queued for a refresh. */ + private void refreshRoutes(String direction, ImportExportConfigurationService service, RouteUpdater routeUpdater, + Consumer routeCreationFailureMarker) { + Map> tenantsConfigsToRefresh = service.consumeConfigsToBeRefresh(); + for (Map.Entry> tenantConfigsToRefresh : tenantsConfigsToRefresh.entrySet()) { + String tenantId = tenantConfigsToRefresh.getKey(); + try { + contextManager.executeAsTenant(tenantId, () -> { + for (Map.Entry configToRefresh : tenantConfigsToRefresh.getValue().entrySet()) { + String configId = configToRefresh.getKey(); + RouterConstants.CONFIG_CAMEL_REFRESH refreshType = configToRefresh.getValue(); + RetryKey retryKey = new RetryKey(direction, tenantId, configId); + try { + if (refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED)) { + routeUpdater.update(tenantId, configId, true); + routeCreationRetryCount.remove(retryKey); + } else if (refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.REMOVED)) { + killExistingRoute(tenantId, configId, true); + routeCreationRetryCount.remove(retryKey); } + } catch (Exception e) { + retryRefreshOrGiveUp(service, retryKey, refreshType, routeCreationFailureMarker, e); } - } catch (Exception e) { - LOGGER.error("Unexpected error while refreshing import/export camel routes for tenant {}", tenantId, e); } - return null; }); + } catch (Exception e) { + // The tenant could not be switched to: none of its refreshes ran. They were already taken off + // the queue, so they are counted and queued again like any other failed refresh, and the + // other tenants are still refreshed. + LOGGER.error("Could not switch to tenant {} to refresh its {} camel routes", tenantId, direction, e); + for (Map.Entry configToRefresh : tenantConfigsToRefresh.getValue().entrySet()) { + // no failure status can be recorded on a configuration of a tenant that cannot be switched to + retryRefreshOrGiveUp(service, new RetryKey(direction, tenantId, configToRefresh.getKey()), configToRefresh.getValue(), + null, e); + } } - } catch (Exception e) { - LOGGER.error("Unexpected error while refreshing import/export camel routes", e); } } - /** Runs the refresh of one tenant's routes; a tenant that cannot be switched to must not keep the others from being refreshed. */ - private void refreshRoutesOfTenant(String tenantId, Supplier refresh) { + /** + * Queues a failed refresh again until it has failed {@link #MAX_ROUTE_CREATION_RETRIES} times, then gives up + * and records the failure on the configuration. Never throws: the refreshes that follow must still run. + */ + private void retryRefreshOrGiveUp(ImportExportConfigurationService service, RetryKey retryKey, + RouterConstants.CONFIG_CAMEL_REFRESH refreshType, Consumer routeCreationFailureMarker, + Exception failure) { + String configId = retryKey.configId(); try { - contextManager.executeAsTenant(tenantId, refresh); + int attempt = routeCreationRetryCount.merge(retryKey, 1, Integer::sum); + if (attempt <= MAX_ROUTE_CREATION_RETRIES) { + LOGGER.error("Refreshing({}) camel route {} failed (attempt {}/{}) — will retry on next tick", + refreshType, configId, attempt, MAX_ROUTE_CREATION_RETRIES, failure); + service.requeueForRefresh(retryKey.tenantId(), configId, refreshType); + } else { + LOGGER.error("Refreshing({}) camel route {} failed after {} attempts — giving up", + refreshType, configId, MAX_ROUTE_CREATION_RETRIES, failure); + routeCreationRetryCount.remove(retryKey); + // drop what the last failed attempt may have left in the context + killExistingRoute(retryKey.tenantId(), configId, false); + if (routeCreationFailureMarker != null) { + routeCreationFailureMarker.accept(configId); + } + } } catch (Exception e) { - LOGGER.error("Could not refresh the camel routes of tenant {}", tenantId, e); + LOGGER.error("Could not record the failed refresh of camel route {} of tenant {}", configId, retryKey.tenantId(), e); } } @@ -339,25 +330,8 @@ public boolean isEnabled(EventObject event) { } }); - // Listed once for the import and the export routes, so that both start for the same tenants. - Set startupTenantIds = contextManager.executeAsSystem(() -> startupTenantIds()); - //--IMPORT ROUTES - //Source - ProfileImportFromSourceRouteBuilder builderReader = new ProfileImportFromSourceRouteBuilder(kafkaProps, configType); - builderReader.setProfileService(profileService); - builderReader.setImportConfigurationService(importConfigurationService); - // Load configs under each tenant: getAll() alone only sees the current (system) context. - builderReader.setImportConfigurationList(loadConfigurationsAcrossTenants(importConfigurationService, startupTenantIds, importTenantsToRetry)); - builderReader.setJacksonDataFormat(jacksonDataFormat); - builderReader.setAllowedEndpoints(allowedEndpoints); - builderReader.setPermittedImportBaseDirs(permittedImportBaseDirs); - builderReader.setContext(camelContext); - builderReader.setExecutionContextManager(contextManager); - builderReader.setSecurityService(securityService); - camelContext.addRoutes(builderReader); - //One shot import route ProfileImportOneShotRouteBuilder builderOneShot = new ProfileImportOneShotRouteBuilder(kafkaProps, configType); builderOneShot.setProfileService(profileService); @@ -377,18 +351,6 @@ public boolean isEnabled(EventObject event) { //--EXPORT ROUTES - //Profiles collect - ProfileExportCollectRouteBuilder profileExportCollectRouteBuilder = new ProfileExportCollectRouteBuilder(kafkaProps, configType); - profileExportCollectRouteBuilder.setExportConfigurationList(loadConfigurationsAcrossTenants(exportConfigurationService, startupTenantIds, exportTenantsToRetry)); - profileExportCollectRouteBuilder.setExportConfigurationService(exportConfigurationService); - profileExportCollectRouteBuilder.setPersistenceService(persistenceService); - profileExportCollectRouteBuilder.setAllowedEndpoints(allowedEndpoints); - profileExportCollectRouteBuilder.setPermittedExportBaseDirs(permittedExportBaseDirs); - profileExportCollectRouteBuilder.setJacksonDataFormat(jacksonDataFormat); - profileExportCollectRouteBuilder.setContext(camelContext); - profileExportCollectRouteBuilder.setExecutionContextManager(contextManager); - camelContext.addRoutes(profileExportCollectRouteBuilder); - //Write to destination ProfileExportProducerRouteBuilder profileExportProducerRouteBuilder = new ProfileExportProducerRouteBuilder(kafkaProps, configType); profileExportProducerRouteBuilder.setProfileService(profileService); @@ -400,6 +362,50 @@ public boolean isEnabled(EventObject event) { camelContext.addRoutes(profileExportProducerRouteBuilder); camelContext.start(); + + startRecurrentRoutes(); + } + + /** + * Starts the routes of the recurrent import/export configurations of every real tenant plus the system + * tenant. {@link ImportExportConfigurationService#getAll()} is context-scoped, so a single call under the + * system context at startup would miss recurring configs of migrated tenants. + *

Each route is added on its own, once the context runs: a configuration whose route cannot be built + * is handed to the refresh timer, with its retries and failure status, and does not keep the routes of + * the other configurations, or of the other tenants, from starting. A tenant whose configurations cannot + * be loaded is retried by the refresh timer too.

+ */ + private void startRecurrentRoutes() { + // Startup has no request subject; nest tenant switches under system like other router paths. + contextManager.executeAsSystem(() -> { + // Listed once for the import and the export routes, so that both start for the same tenants. + for (String tenantId : startupTenantIds()) { + startRecurrentRoutes(tenantId, importConfigurationService, importTenantsToRetry, this::addImportReaderRoute); + startRecurrentRoutes(tenantId, exportConfigurationService, exportTenantsToRetry, this::addExportReaderRoute); + } + }); + } + + private void startRecurrentRoutes(String tenantId, ImportExportConfigurationService service, + Set tenantsToRetry, RouteAdder routeAdder) { + try { + contextManager.executeAsTenant(tenantId, () -> { + for (T config : service.getAll()) { + try { + routeAdder.add(config); + } catch (Exception e) { + LOGGER.error("Could not start the camel route of configuration {} of tenant {}, the route refresh " + + "will retry it", config.getItemId(), tenantId, e); + service.requeueForRefresh(tenantId, config.getItemId(), RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED); + } + } + }); + } catch (Exception e) { + LOGGER.error("Could not load router configurations for tenant {}, its recurring routes are not " + + "started; the load will be retried", tenantId, e); + tenantsToRetry.add(tenantId); + tenantsWithLoggedLoadFailure.add(tenantId); + } } /** Persists a visible failure status on the configuration once route creation has exhausted all retries, instead of only logging it. */ @@ -424,13 +430,11 @@ private void markExportRouteCreationFailed(String configId) { @Override public void killExistingRoute(String tenantId, String configId, boolean fireEvent) throws Exception { String routeId = RouteIds.of(tenantId, configId); - //Active routes - Route route = camelContext.getRoute(routeId); - if (route != null) { - RouteDefinition routeDefinition = camelContext.getRouteDefinition(routeId); - if (routeDefinition != null) { - camelContext.removeRouteDefinition(routeDefinition); - } + // The definition is removed whether or not it has a running route: a route that failed to start + // leaves its definition behind, which must not survive the configuration or pile up over retries. + RouteDefinition routeDefinition = camelContext.getRouteDefinition(routeId); + if (routeDefinition != null) { + camelContext.removeRouteDefinition(routeDefinition); } } @@ -444,6 +448,11 @@ public void updateProfileImportReaderRoute(String tenantId, String configId, boo throw new IllegalStateException("Cannot update profile import reader route, config: " + configId + " not found — will be retried"); } + addImportReaderRoute(importConfiguration); + } + + /** Adds the route of a recurrent import configuration to the running context; other configurations get none. */ + private void addImportReaderRoute(ImportConfiguration importConfiguration) throws Exception { if (RouterConstants.IMPORT_EXPORT_CONFIG_TYPE_RECURRENT.equals(importConfiguration.getConfigType())) { ProfileImportFromSourceRouteBuilder builder = new ProfileImportFromSourceRouteBuilder(kafkaProps, configType); builder.setImportConfigurationList(Arrays.asList(importConfiguration)); @@ -469,6 +478,11 @@ public void updateProfileExportReaderRoute(String tenantId, String configId, boo throw new IllegalStateException("Cannot update profile export reader route, config: " + configId + " not found — will be retried"); } + addExportReaderRoute(exportConfiguration); + } + + /** Adds the collect route of a recurrent export configuration to the running context; other configurations get none. */ + private void addExportReaderRoute(ExportConfiguration exportConfiguration) throws Exception { if (RouterConstants.IMPORT_EXPORT_CONFIG_TYPE_RECURRENT.equals(exportConfiguration.getConfigType())) { ProfileExportCollectRouteBuilder profileExportCollectRouteBuilder = new ProfileExportCollectRouteBuilder(kafkaProps, configType); profileExportCollectRouteBuilder.setExportConfigurationList(Collections.singletonList(exportConfiguration)); @@ -574,34 +588,8 @@ public void setPermittedExportBaseDirs(String permittedExportBaseDirs) { this.permittedExportBaseDirs = permittedExportBaseDirs; } - /** - * Loads import/export configurations for the given tenants. - * {@link ImportExportConfigurationService#getAll()} is context-scoped, so a single call - * under the system context at startup would miss recurring configs of migrated tenants. - *

A tenant whose configurations cannot be loaded does not block the others: it is added to - * {@code tenantsToRetry} and the refresh timer builds its routes once it can be loaded.

- */ - List loadConfigurationsAcrossTenants(ImportExportConfigurationService service, Set tenantIds, - Set tenantsToRetry) { - // Startup has no request subject; nest tenant switches under system like other router paths. - return contextManager.executeAsSystem(() -> { - List all = new ArrayList<>(); - for (String tenantId : tenantIds) { - try { - all.addAll(contextManager.executeAsTenant(tenantId, service::getAll)); - } catch (Exception e) { - LOGGER.error("Could not load router configurations for tenant {}, its recurring routes are not " - + "started; the load will be retried", tenantId, e); - tenantsToRetry.add(tenantId); - tenantsWithLoggedLoadFailure.add(tenantId); - } - } - return all; - }); - } - /** Every tenant plus the system tenant, or the system tenant alone until tenants can be listed. */ - Set startupTenantIds() { + private Set startupTenantIds() { Set tenantIds = new LinkedHashSet<>(); try { for (Tenant tenant : tenantService.getAllTenants()) { @@ -644,7 +632,7 @@ void retryStartupLoadsWhenDue(long now) { * @return {@code true} when nothing is left to retry */ boolean retryStartupLoads() { - if (!tenantListingToRetry && importTenantsToRetry.isEmpty() && exportTenantsToRetry.isEmpty()) { + if (nothingToRetry()) { return true; } contextManager.executeAsSystem(() -> { @@ -664,6 +652,10 @@ boolean retryStartupLoads() { retryStartupLoad(importTenantsToRetry, importConfigurationService); retryStartupLoad(exportTenantsToRetry, exportConfigurationService); }); + return nothingToRetry(); + } + + private boolean nothingToRetry() { return !tenantListingToRetry && importTenantsToRetry.isEmpty() && exportTenantsToRetry.isEmpty(); } @@ -679,6 +671,7 @@ private void retryStartupLoad(Set tenantsToRetry, Impor service.requeueForRefresh(tenantId, config.getItemId(), RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED); } tenantsToRetry.remove(tenantId); + tenantsWithLoggedLoadFailure.remove(tenantId); LOGGER.info("Router configurations of tenant {} loaded, its recurring routes are being started", tenantId); } catch (Exception e) { if (tenantsWithLoggedLoadFailure.add(tenantId)) { @@ -690,4 +683,12 @@ private void retryStartupLoad(Set tenantsToRetry, Impor } } } + + private interface RouteUpdater { + void update(String tenantId, String configId, boolean fireEvent) throws Exception; + } + + private interface RouteAdder { + void add(T config) throws Exception; + } } diff --git a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java index 518cd5a9d6..add424dbbd 100644 --- a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java +++ b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java @@ -22,7 +22,6 @@ import org.apache.unomi.api.ExecutionContext; import org.apache.unomi.api.Item; import org.apache.unomi.api.services.ExecutionContextManager; -import org.apache.unomi.api.services.ProfileService; import org.apache.unomi.api.tenants.Tenant; import org.apache.unomi.api.tenants.TenantService; import org.apache.unomi.router.api.ExportConfiguration; @@ -34,9 +33,9 @@ import org.apache.unomi.router.core.processor.ImportConfigByFileNameProcessor; import org.apache.unomi.router.core.processor.ImportRouteCompletionProcessor; import org.apache.unomi.router.core.processor.UnomiStorageProcessor; +import org.apache.unomi.router.core.route.RouterTestFixtures; import org.apache.unomi.router.api.RouteIds; import org.junit.After; -import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; @@ -45,16 +44,12 @@ import java.lang.reflect.Proxy; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.function.Supplier; -import java.util.stream.Collectors; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -79,19 +74,11 @@ public class RouterCamelContextTenantLoadTest { private String failingTenant; private boolean tenantListingFails; - private RouterCamelContext context; private DefaultCamelContext camelContext; private TenantScopedService importService; private TenantScopedService exportService; private String unswitchableTenant; - @Before - public void setUp() { - context = new RouterCamelContext(); - context.setContextManager(new TenantTrackingContextManager()); - context.setTenantService(tenantService("tenant-a", "tenant-b")); - } - @After public void tearDown() throws Exception { if (camelContext != null) { @@ -99,22 +86,6 @@ public void tearDown() throws Exception { } } - @Test - public void loadsConfigurationsOfEveryTenantIncludingSystem() { - configsByTenant.put("tenant-a", Collections.singletonList(config("import-a", "tenant-a"))); - configsByTenant.put("tenant-b", Collections.singletonList(config("import-b", "tenant-b"))); - configsByTenant.put(TenantService.SYSTEM_TENANT, Collections.singletonList(config("import-system", TenantService.SYSTEM_TENANT))); - Set tenantsToRetry = new HashSet<>(); - - List loaded = context.loadConfigurationsAcrossTenants( - new TenantScopedService<>(configsByTenant), context.startupTenantIds(), tenantsToRetry); - - assertEquals(Arrays.asList("import-a", "import-b", "import-system"), ids(loaded)); - assertEquals(Arrays.asList("tenant-a", "tenant-b", TenantService.SYSTEM_TENANT), visitedTenants); - assertTrue(tenantsToRetry.isEmpty()); - assertNull("tenant context must be restored after the load", currentTenant); - } - /** * The startup path itself: after a restart, the routes of every tenant's recurrent configurations * must exist without anyone saving the configurations again. @@ -123,9 +94,10 @@ public void loadsConfigurationsOfEveryTenantIncludingSystem() { public void startupBuildsTheRecurrentRoutesOfEveryTenant() throws Exception { RouterCamelContext router = startRouter(); - for (String tenantId : Arrays.asList("tenant-a", "tenant-b")) { + for (String tenantId : Arrays.asList("tenant-a", "tenant-b", TenantService.SYSTEM_TENANT)) { assertRoutesBuilt(tenantId); } + assertNull("tenant context must be restored after startup", currentTenant); // Removing one tenant's route must leave the other tenant's route of the same configuration id. router.killExistingRoute("tenant-a", "crm-import", false); @@ -236,6 +208,69 @@ public void tenantThatCannotBeSwitchedToDoesNotKeepTheOthersFromBeingRefreshed() assertRoutesBuilt("tenant-b"); } + @Test + public void configurationWhoseRouteCannotBeBuiltDoesNotKeepTheOthersFromStarting() throws Exception { + // an export without a period cannot be turned into a timer route + ExportConfiguration broken = recurrentExport("broken-export", "tenant-a", new File(exportRoot(), "tenant-a")); + broken.getProperties().remove("period"); + // active, so that the route is started, which is when its timer endpoint is found to be unusable + broken.setActive(true); + exportConfigsByTenant.computeIfAbsent("tenant-a", k -> new ArrayList<>()).add(broken); + + RouterCamelContext router = startRouter(); + + for (String tenantId : Arrays.asList("tenant-a", "tenant-b", TenantService.SYSTEM_TENANT)) { + assertRoutesBuilt(tenantId); + } + assertNull(camelContext.getRoute(RouteIds.of("tenant-a", "broken-export"))); + + // the refresh retries it, then gives up and records the failure on the configuration + for (int tick = 0; tick < 10; tick++) { + router.refreshRoutes(); + } + assertEquals(RouterConstants.CONFIG_STATUS_ROUTE_CREATION_FAILED, broken.getStatus()); + assertTrue("a configuration that was given up on is not retried forever", exportService.toRefresh.isEmpty()); + assertEquals("failed attempts must not leave route definitions behind", 0, camelContext.getRouteDefinitions().stream() + .filter(route -> RouteIds.of("tenant-a", "broken-export").equals(route.getId())).count()); + assertRoutesBuilt("tenant-a"); + } + + @Test + public void refreshesOfATenantThatCannotBeSwitchedToAreRetriedOnTheNextTicks() throws Exception { + RouterCamelContext router = startRouter(); + router.killExistingRoute("tenant-a", "crm-import", false); + importService.requeueForRefresh("tenant-a", "crm-import", RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED); + unswitchableTenant = "tenant-a"; + + router.refreshRoutes(); + assertNull(camelContext.getRouteDefinition(RouteIds.of("tenant-a", "crm-import"))); + + unswitchableTenant = null; + router.refreshRoutes(); + assertNotNull("the refresh that could not run must not be lost", + camelContext.getRouteDefinition(RouteIds.of("tenant-a", "crm-import"))); + } + + @Test + public void refreshesOfATenantThatCanNeverBeSwitchedToAreGivenUpOn() throws Exception { + RouterCamelContext router = startRouter(); + importService.requeueForRefresh("tenant-a", "crm-import", RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED); + unswitchableTenant = "tenant-a"; + + for (int tick = 0; tick < 10; tick++) { + router.refreshRoutes(); + } + + assertTrue("a tenant that is gone must not be retried on every tick forever", importService.toRefresh.isEmpty()); + } + + @Test + public void shutdownAfterAFailedStartDoesNotFail() throws Exception { + RouterCamelContext neverStarted = new RouterCamelContext(); + + neverStarted.destroy(); + } + @Test public void tenantListingFailureAtStartupIsRetried() throws Exception { tenantListingFails = true; @@ -264,17 +299,18 @@ public void nothingIsReloadedOnceEveryTenantIsStarted() throws Exception { } /** - * Starts a router over two tenants that both own a recurrent import and a recurrent export, under the same - * configuration ids: ids are only unique within a tenant. + * Starts a router over two tenants and the system tenant, which all own a recurrent import and a recurrent + * export under the same configuration ids: ids are only unique within a tenant. Configurations a test put + * in place beforehand are kept. */ private RouterCamelContext startRouter() throws Exception { - File importRoot = tmp.newFolder("permitted-import"); - File exportRoot = tmp.newFolder("permitted-export"); - for (String tenantId : Arrays.asList("tenant-a", "tenant-b")) { - configsByTenant.put(tenantId, Collections.singletonList( - recurrentImport("crm-import", tenantId, new File(importRoot, tenantId)))); - exportConfigsByTenant.put(tenantId, Collections.singletonList( - recurrentExport("crm-export", tenantId, new File(exportRoot, tenantId)))); + File importRoot = new File(tmp.getRoot(), "permitted-import"); + File exportRoot = exportRoot(); + for (String tenantId : Arrays.asList("tenant-a", "tenant-b", TenantService.SYSTEM_TENANT)) { + configsByTenant.computeIfAbsent(tenantId, k -> new ArrayList<>()) + .add(recurrentImport("crm-import", tenantId, new File(importRoot, tenantId))); + exportConfigsByTenant.computeIfAbsent(tenantId, k -> new ArrayList<>()) + .add(recurrentExport("crm-export", tenantId, new File(exportRoot, tenantId))); } camelContext = new DefaultCamelContext(); @@ -290,7 +326,7 @@ CamelContext createCamelContext() { router.setImportConfigurationService(importService); exportService = new TenantScopedService<>(exportConfigsByTenant); router.setExportConfigurationService(exportService); - router.setProfileService(noOpProfileService()); + router.setProfileService(RouterTestFixtures.noOpProfileService()); router.setKafkaProps(NO_KAFKA); router.setConfigType(RouterConstants.CONFIG_TYPE_NOBROKER); router.setJacksonDataFormat(new JacksonDataFormat(ProfileToImport.class)); @@ -310,6 +346,10 @@ private int retriesOf(String tenantId) { return importService.loadAttempts.getOrDefault(tenantId, 0) - 1; } + private File exportRoot() { + return new File(tmp.getRoot(), "permitted-export"); + } + private void assertRoutesBuilt(String tenantId) { for (String configId : Arrays.asList("crm-import", "crm-export")) { assertNotNull("the route of " + configId + " must be built for " + tenantId, @@ -318,7 +358,7 @@ private void assertRoutesBuilt(String tenantId) { } private static ImportConfiguration recurrentImport(String id, String tenantId, File directory) { - assertTrue(directory.mkdirs()); + assertTrue(directory.isDirectory() || directory.mkdirs()); ImportConfiguration configuration = config(id, tenantId); configuration.setConfigType(RouterConstants.IMPORT_EXPORT_CONFIG_TYPE_RECURRENT); // inactive: the route is built but does not start polling during the test @@ -329,7 +369,7 @@ private static ImportConfiguration recurrentImport(String id, String tenantId, F } private static ExportConfiguration recurrentExport(String id, String tenantId, File directory) { - assertTrue(directory.mkdirs()); + assertTrue(directory.isDirectory() || directory.mkdirs()); ExportConfiguration configuration = new ExportConfiguration(); configuration.setItemId(id); configuration.setTenantId(tenantId); @@ -342,18 +382,6 @@ private static ExportConfiguration recurrentExport(String id, String tenantId, F return configuration; } - private static ProfileService noOpProfileService() { - return (ProfileService) Proxy.newProxyInstance( - ProfileService.class.getClassLoader(), - new Class[]{ProfileService.class}, - (proxy, method, args) -> Collection.class.isAssignableFrom(method.getReturnType()) - ? Collections.emptyList() : null); - } - - private static List ids(List configs) { - return configs.stream().map(ImportConfiguration::getItemId).collect(Collectors.toList()); - } - private static ImportConfiguration config(String id, String tenantId) { ImportConfiguration config = new ImportConfiguration(); config.setItemId(id); diff --git a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/processor/RouteCompletionProcessorTest.java b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/processor/RouteCompletionProcessorTest.java index 6a77ca1975..d4ceeb358f 100644 --- a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/processor/RouteCompletionProcessorTest.java +++ b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/processor/RouteCompletionProcessorTest.java @@ -35,6 +35,7 @@ import java.util.Map; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; /** @@ -82,6 +83,33 @@ public void exportCompletionUpdatesTheConfigurationOfTheRoute() throws Exception assertEquals(RouterConstants.CONFIG_STATUS_COMPLETE_SUCCESS, configuration.getStatus()); } + @Test + public void importCompletionOfAConfigurationThatIsGoneSavesNothing() throws Exception { + StoredConfigurations store = new StoredConfigurations<>(); + ImportRouteCompletionProcessor processor = new ImportRouteCompletionProcessor(); + processor.setImportConfigurationService(store); + + Exchange exchange = exchangeFromRoute(RouteIds.of("tenant-a", "deleted-import")); + exchange.setProperty("CamelSplitSize", 1); + exchange.getIn().setBody(new ArrayList<>(Collections.singletonList(new ProfileToImport()))); + processor.process(exchange); + + assertNull(store.lastSaved); + } + + @Test + public void exportCompletionOfAConfigurationThatIsGoneSavesNothing() throws Exception { + StoredConfigurations store = new StoredConfigurations<>(); + ExportRouteCompletionProcessor processor = new ExportRouteCompletionProcessor(); + processor.setExportConfigurationService(store); + + Exchange exchange = exchangeFromRoute(RouteIds.of("tenant-a", "deleted-export")); + exchange.setProperty("CamelSplitSize", 1); + processor.process(exchange); + + assertNull(store.lastSaved); + } + private static Exchange exchangeFromRoute(String routeId) { Exchange exchange = new DefaultExchange(new DefaultCamelContext()); exchange.setFromRouteId(routeId); diff --git a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RouteIdsTest.java b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RouteIdsTest.java index f3efe9ecfc..bcd7ae9b9a 100644 --- a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RouteIdsTest.java +++ b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RouteIdsTest.java @@ -21,6 +21,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertThrows; public class RouteIdsTest { @@ -40,6 +41,16 @@ public void configurationWithoutTenantBelongsToTheSystemTenant() { assertEquals(RouteIds.of("system", "crm-import"), RouteIds.of(null, "crm-import")); } + @Test + public void tenantIdThatWouldMakeTheRouteIdAmbiguousIsRefused() { + assertThrows(IllegalArgumentException.class, () -> RouteIds.of("tenant:a", "crm-import")); + } + + @Test + public void routeIdNeedsAConfigurationId() { + assertThrows(NullPointerException.class, () -> RouteIds.of("tenant-a", null)); + } + @Test public void routeNotBuiltFromAConfigurationKeepsItsId() { assertEquals("ONE_SHOT_ROUTE", RouteIds.configId("ONE_SHOT_ROUTE")); diff --git a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RouterTestFixtures.java b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RouterTestFixtures.java index ff1ca1b8d8..af8e0a9646 100644 --- a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RouterTestFixtures.java +++ b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RouterTestFixtures.java @@ -31,7 +31,7 @@ * place so that the tests of route construction and of the recorded status cannot come to disagree * about it. */ -final class RouterTestFixtures { +public final class RouterTestFixtures { static final String TENANT = "acme"; @@ -70,7 +70,7 @@ static ExportConfiguration recurrentExport(String itemId, String destination) { * The route builders ask the profile service for the profile property types while they build. * Nothing in these tests depends on what it answers. */ - static ProfileService noOpProfileService() { + public static ProfileService noOpProfileService() { return (ProfileService) Proxy.newProxyInstance( ProfileService.class.getClassLoader(), new Class[]{ProfileService.class}, diff --git a/itests/src/test/java/org/apache/unomi/itests/AllITs.java b/itests/src/test/java/org/apache/unomi/itests/AllITs.java index 76c1ad0542..5e07fde443 100644 --- a/itests/src/test/java/org/apache/unomi/itests/AllITs.java +++ b/itests/src/test/java/org/apache/unomi/itests/AllITs.java @@ -94,6 +94,7 @@ OtherCommandsIT.class, LegacyQueryBuilderMappingIT.class, TenantIT.class, + BackgroundTenantTasksIT.class, SchedulerIT.class, EventsCollectorIT.class, RolloverIT.class, diff --git a/itests/src/test/java/org/apache/unomi/itests/BackgroundTenantTasksIT.java b/itests/src/test/java/org/apache/unomi/itests/BackgroundTenantTasksIT.java new file mode 100644 index 0000000000..6d9cb36907 --- /dev/null +++ b/itests/src/test/java/org/apache/unomi/itests/BackgroundTenantTasksIT.java @@ -0,0 +1,201 @@ +/* + * 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.unomi.itests; + +import org.apache.unomi.api.Event; +import org.apache.unomi.api.Metadata; +import org.apache.unomi.api.Profile; +import org.apache.unomi.api.actions.Action; +import org.apache.unomi.api.conditions.Condition; +import org.apache.unomi.api.rules.Rule; +import org.apache.unomi.api.rules.RuleStatistics; +import org.apache.unomi.api.segments.Segment; +import org.apache.unomi.api.services.EventService; +import org.apache.unomi.api.tenants.Tenant; +import org.apache.unomi.router.api.ImportConfiguration; +import org.apache.unomi.router.api.RouterConstants; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.ops4j.pax.exam.junit.PaxExam; +import org.ops4j.pax.exam.spi.reactors.ExamReactorStrategy; +import org.ops4j.pax.exam.spi.reactors.PerSuite; + +import java.io.File; +import java.time.LocalDate; +import java.time.ZoneId; +import java.util.Collections; +import java.util.Date; +import java.util.Objects; + +/** + * UNOMI-1000: background work must reach every tenant, not only the system tenant. The test tenant the + * suite runs in is a real tenant, so these tests fail when a task only sees {@code tenantId=system}. + */ +@RunWith(PaxExam.class) +@ExamReactorStrategy(PerSuite.class) +public class BackgroundTenantTasksIT extends BaseIT { + + private static final String RULE_ID = "unomi1000-statistics-rule"; + private static final String EVENT_TYPE = "unomi1000StatisticsEvent"; + private static final String SEGMENT_ID = "unomi1000-system-inactive-segment"; + private static final String IMPORT_CONFIG_ID = "unomi1000-shared-import"; + private static final String OTHER_TENANT_ID = "unomi1000-router-tenant"; + + /** + * The statistics of a tenant's rule are only persisted when the periodic statistics refresh runs + * under that tenant. + */ + @Test + public void ruleStatisticsOfATenantRuleArePersisted() throws InterruptedException { + String profileId = "unomi1000-statistics-profile"; + Profile profile = new Profile(profileId); + profileService.save(profile); + + Condition condition = new Condition(definitionsService.getConditionType("eventTypeCondition")); + condition.setParameter("eventTypeId", EVENT_TYPE); + Action action = new Action(definitionsService.getActionType("setPropertyAction")); + action.setParameter("propertyName", "unomi1000Statistics"); + action.setParameter("propertyValue", "fired"); + Rule rule = new Rule(new Metadata("test-scope", RULE_ID, RULE_ID, "Counts its executions for UNOMI-1000")); + rule.setCondition(condition); + rule.setActions(Collections.singletonList(action)); + try { + createAndWaitForRule(rule); + rulesService.refreshRules(); + + // The refresh runs every 10 seconds by default; keep the rule firing until its count is stored. + RuleStatistics statistics = keepTrying("Statistics of a tenant rule were not persisted for the tenant", + () -> { + Event event = new Event(EVENT_TYPE, null, profile, null, null, profile, new Date()); + event.setPersistent(false); + Assert.assertNotEquals(EventService.ERROR, eventService.send(event)); + return persistenceService.load(RULE_ID, RuleStatistics.class); + }, + stored -> stored != null && stored.getExecutionCount() > 0, + 1000, 60); + Assert.assertEquals("Statistics must be stored in the tenant owning the rule", TEST_TENANT_ID, statistics.getTenantId()); + } finally { + rulesService.removeRule(RULE_ID); + persistenceService.remove(RULE_ID, RuleStatistics.class); + profileService.delete(profileId, false); + waitForNullValue("Rule still present after deletion", () -> rulesService.getRule(RULE_ID), DEFAULT_TRYING_TIMEOUT, + DEFAULT_TRYING_TRIES); + } + } + + /** + * A date-relative segment owned by the system tenant applies to the profiles of every tenant, so the + * recalculation of a tenant has to re-evaluate it for that tenant's profiles. + */ + @Test + public void systemDateRelativeSegmentIsRecalculatedForTenantProfiles() throws InterruptedException { + String profileId = "unomi1000-inactive-profile"; + Profile profile = new Profile(profileId); + LocalDate twoMonthsAgo = LocalDate.now().minusDays(60); + profile.setProperty("lastVisit", Date.from(twoMonthsAgo.atStartOfDay(ZoneId.systemDefault()).toInstant())); + profileService.save(profile); + persistenceService.refreshIndex(Profile.class); + + Condition inactiveForAMonth = new Condition(definitionsService.getConditionType("profilePropertyCondition")); + inactiveForAMonth.setParameter("propertyName", "properties.lastVisit"); + inactiveForAMonth.setParameter("comparisonOperator", "lessThanOrEqualTo"); + inactiveForAMonth.setParameter("propertyValueDateExpr", "now-30d"); + Segment segment = new Segment(new Metadata(SEGMENT_ID)); + segment.setCondition(inactiveForAMonth); + try { + // Defining the segment under the system tenant only evaluates the system tenant's own profiles. + executionContextManager.executeAsSystem(() -> segmentService.setSegmentDefinition(segment)); + keepTrying("The system segment is not visible from the tenant", () -> segmentService.getSegmentDefinition(SEGMENT_ID), + Objects::nonNull, DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES); + Assert.assertFalse("The profile was saved directly, it cannot be in the segment yet", + profileService.load(profileId).getSegments().contains(SEGMENT_ID)); + + // What the daily task runs under each tenant; events are disabled to keep the test free of races. + segmentService.recalculatePastEventConditions(false); + persistenceService.refreshIndex(Profile.class); + + keepTrying("The tenant profile did not enter the system tenant's date-relative segment", + () -> profileService.load(profileId), + updated -> updated != null && updated.getSegments().contains(SEGMENT_ID), + 1000, 20); + } finally { + executionContextManager.executeAsSystem(() -> { + segmentService.removeSegmentDefinition(SEGMENT_ID, false); + }); + profileService.delete(profileId, false); + waitForNullValue("Profile still present after deletion", () -> profileService.load(profileId), DEFAULT_TRYING_TIMEOUT, + DEFAULT_TRYING_TRIES); + } + } + + /** + * Configuration ids are only unique within a tenant: two tenants using the same id each get their + * own route, and removing one tenant's configuration leaves the other tenant's route running. + */ + @Test + public void sameImportConfigurationIdInTwoTenantsGetsTwoRoutes() throws InterruptedException { + Tenant otherTenant = tenantService.createTenant(OTHER_TENANT_ID, Collections.emptyMap()); + String otherTenantId = otherTenant.getItemId(); + try { + importConfigurationService.save(recurrentImport(TEST_TENANT_ID), true); + executionContextManager.executeAsTenant(otherTenantId, () -> { + importConfigurationService.save(recurrentImport(otherTenantId), true); + }); + + keepTrying("No route was started for the configuration of the test tenant", + () -> camelRouteExists(TEST_TENANT_ID, IMPORT_CONFIG_ID), exists -> exists, 1000, 30); + keepTrying("No route was started for the configuration of the other tenant, which uses the same id", + () -> camelRouteExists(otherTenantId, IMPORT_CONFIG_ID), exists -> exists, 1000, 30); + + executionContextManager.executeAsTenant(otherTenantId, () -> importConfigurationService.delete(IMPORT_CONFIG_ID)); + + keepTrying("The route of the deleted configuration is still there", + () -> camelRouteExists(otherTenantId, IMPORT_CONFIG_ID), exists -> !exists, 1000, 30); + Assert.assertTrue("Deleting one tenant's configuration must not remove the route of another tenant's configuration", + camelRouteExists(TEST_TENANT_ID, IMPORT_CONFIG_ID)); + } finally { + try { + importConfigurationService.delete(IMPORT_CONFIG_ID); + keepTrying("The route of the test tenant's configuration is still there after its deletion", + () -> camelRouteExists(TEST_TENANT_ID, IMPORT_CONFIG_ID), exists -> !exists, 1000, 30); + } finally { + tenantService.deleteTenant(otherTenantId); + } + } + } + + /** A recurrent import polling the tenant's own directory for a file that never comes. */ + private ImportConfiguration recurrentImport(String tenantId) { + File tenantImportDirectory = new File("data/tmp/recurrent_import/" + tenantId); + tenantImportDirectory.mkdirs(); + + ImportConfiguration configuration = new ImportConfiguration(); + configuration.setItemId(IMPORT_CONFIG_ID); + configuration.setConfigType(RouterConstants.IMPORT_EXPORT_CONFIG_TYPE_RECURRENT); + configuration.setMergingProperty("email"); + configuration.setOverwriteExistingProfiles(true); + configuration.setColumnSeparator(";"); + configuration.setHasHeader(true); + configuration.setHasDeleteColumn(false); + configuration.getProperties().put("mapping", Collections.singletonMap("email", 0)); + configuration.getProperties().put("source", + "file://" + tenantImportDirectory.getAbsolutePath() + "?fileName=unomi1000-never-delivered.csv&move=.done"); + configuration.setActive(true); + return configuration; + } +} diff --git a/itests/src/test/java/org/apache/unomi/itests/CorePersistenceITs.java b/itests/src/test/java/org/apache/unomi/itests/CorePersistenceITs.java index 38d88b17a6..f7a8d8fa0a 100644 --- a/itests/src/test/java/org/apache/unomi/itests/CorePersistenceITs.java +++ b/itests/src/test/java/org/apache/unomi/itests/CorePersistenceITs.java @@ -91,6 +91,7 @@ OtherCommandsIT.class, LegacyQueryBuilderMappingIT.class, TenantIT.class, + BackgroundTenantTasksIT.class, SchedulerIT.class, EventsCollectorIT.class, RolloverIT.class, diff --git a/manual/src/main/asciidoc/migrations/migrate-3.0-to-4.0.adoc b/manual/src/main/asciidoc/migrations/migrate-3.0-to-4.0.adoc index e93cd64275..d8e967239b 100644 --- a/manual/src/main/asciidoc/migrations/migrate-3.0-to-4.0.adoc +++ b/manual/src/main/asciidoc/migrations/migrate-3.0-to-4.0.adoc @@ -292,6 +292,26 @@ files under `{baseDir}/{tenantId}` and update its `source` or `destination` acco base-directory properties at the directories already in use. The mark clears on its own once the endpoint is accepted again. The "Profile import and export" chapter describes the rule in full. +==== Background tasks and recurrent routes run for every tenant + +The scheduled purge, the daily recalculation of date-relative segments and scoring plans, the rule statistics +refresh and the start of recurrent import and export routes apply to every tenant, the tenant holding the +migrated data included. + +- The first purge after the upgrade applies the configured retention (`org.apache.unomi.profile.purge.*`, + `org.apache.unomi.session.purge.existTime`, `org.apache.unomi.event.purge.existTime`, + `org.apache.unomi.monthly.index.purge.existTime`) to the migrated tenant in one pass, shortly after startup. + Review these values before starting 4.0 if the 3.0 deployment ran with the purge disabled or with a longer + retention. +- A purge or recalculation that fails for one tenant is still run for the others; the task is then reported as + failed, naming the tenants concerned, and the scheduler retries it. +- The Camel route of a recurrent import or export configuration is identified by `{tenantId}:{configId}` + instead of the configuration identifier alone, since configuration identifiers are only unique within a + tenant. Code that looks routes up in the router's Camel context must build the identifier with + `org.apache.unomi.router.api.RouteIds`. The `killExistingRoute`, `updateProfileImportReaderRoute` and + `updateProfileExportReaderRoute` methods of `IRouterCamelContext` take the tenant identifier as their first + parameter. + ==== Migration Steps 1. **Understand Multi-Tenancy Impact** diff --git a/services-common/src/test/java/org/apache/unomi/services/common/cache/AbstractMultiTypeCachingServiceTest.java b/services-common/src/test/java/org/apache/unomi/services/common/cache/AbstractMultiTypeCachingServiceTest.java index ad962c9091..4729badf33 100644 --- a/services-common/src/test/java/org/apache/unomi/services/common/cache/AbstractMultiTypeCachingServiceTest.java +++ b/services-common/src/test/java/org/apache/unomi/services/common/cache/AbstractMultiTypeCachingServiceTest.java @@ -517,4 +517,84 @@ public void testSaveItemAuditsCreateWhenExistingItemHasNoCreateAudit() { verify(auditService).auditCreate(same(fromBundle), eq("system-bundle")); verify(auditService, never()).auditUpdate(any(), anyString()); } + + @Test + public void testExecuteForEachTenantRunsUnderEveryTenantAndSystem() { + List tenantsRun = new ArrayList<>(); + recordTenantsRun(tenantsRun, null); + + List failed = testCachingService.executeForEachTenant("test task", () -> { }); + + assertTrue("no tenant failed", failed.isEmpty()); + assertEquals(new HashSet<>(Arrays.asList(TEST_TENANT, SYSTEM_TENANT)), new HashSet<>(tenantsRun)); + assertEquals("each tenant runs once", 2, tenantsRun.size()); + } + + @Test + public void testExecuteForEachTenantReportsTheFailingTenantAndRunsTheOthers() { + List tenantsRun = new ArrayList<>(); + recordTenantsRun(tenantsRun, TEST_TENANT); + + List failed = testCachingService.executeForEachTenant("test task", () -> { }); + + assertEquals(Collections.singletonList(TEST_TENANT), failed); + assertEquals(Collections.singletonList(SYSTEM_TENANT), tenantsRun); + } + + @Test + public void testExecuteForEachTenantReportsTenantsSkippedByAnInterruption() { + List tenantsRun = new ArrayList<>(); + recordTenantsRun(tenantsRun, null); + + List failed; + Thread.currentThread().interrupt(); + try { + failed = testCachingService.executeForEachTenant("test task", () -> { }); + } finally { + Thread.interrupted(); + } + + assertEquals(new HashSet<>(Arrays.asList(TEST_TENANT, SYSTEM_TENANT)), new HashSet<>(failed)); + assertTrue("no tenant runs once the thread is interrupted", tenantsRun.isEmpty()); + } + + @Test + public void testExecuteForEachTenantCompletesTheTaskWhenEveryTenantRan() { + recordTenantsRun(new ArrayList<>(), null); + org.apache.unomi.api.tasks.TaskExecutor.TaskStatusCallback callback = + mock(org.apache.unomi.api.tasks.TaskExecutor.TaskStatusCallback.class); + + assertTrue(testCachingService.executeForEachTenant("test task", () -> { }, callback)); + + verify(callback).complete(); + verify(callback, never()).fail(anyString()); + } + + @Test + public void testExecuteForEachTenantFailsTheTaskNamingTheTenantsThatFailed() { + recordTenantsRun(new ArrayList<>(), TEST_TENANT); + org.apache.unomi.api.tasks.TaskExecutor.TaskStatusCallback callback = + mock(org.apache.unomi.api.tasks.TaskExecutor.TaskStatusCallback.class); + + assertFalse(testCachingService.executeForEachTenant("test task", () -> { }, callback)); + + ArgumentCaptor failure = ArgumentCaptor.forClass(String.class); + verify(callback).fail(failure.capture()); + verify(callback, never()).complete(); + assertTrue(failure.getValue(), failure.getValue().contains(TEST_TENANT)); + assertFalse(failure.getValue(), failure.getValue().contains(SYSTEM_TENANT)); + } + + /** Records the tenants the action runs under; switching to {@code failingTenant} throws instead. */ + private void recordTenantsRun(List tenantsRun, String failingTenant) { + doAnswer(invocation -> { + String tenantId = invocation.getArgument(0); + if (tenantId.equals(failingTenant)) { + throw new IllegalStateException("simulated failure for " + tenantId); + } + tenantsRun.add(tenantId); + ((Runnable) invocation.getArgument(1)).run(); + return null; + }).when(contextManager).executeAsTenant(anyString(), any(Runnable.class)); + } } diff --git a/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java index f8fd201b84..0feb57c658 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java @@ -376,8 +376,11 @@ public void execute(ScheduledTask task, TaskExecutor.TaskStatusCallback callback purgeEventItems(purgeEventExistTime); } }, callback); - LOGGER.info("Purge: {} in {} ms", purged ? "executed" : "failed for some tenants", - System.currentTimeMillis() - purgeStartTime); + if (purged) { + LOGGER.info("Purge: executed in {} ms", System.currentTimeMillis() - purgeStartTime); + } else { + LOGGER.warn("Purge: failed for some tenants after {} ms", System.currentTimeMillis() - purgeStartTime); + } } catch (Throwable t) { // During shutdown, services may be unavailable - only log if not shutting down LOGGER.error("Error while purging profiles, sessions, or events", t); diff --git a/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java index b7ec4b73c6..e5dd4f972d 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java @@ -767,16 +767,20 @@ public void removeRule(String ruleId) { } private void syncRuleStatistics() { + String currentTenant = contextManager.getCurrentContext().getTenantId(); + + Map tenantStats = getRuleStatisticsForTenant(currentTenant); + if (tenantStats.isEmpty()) { + // nothing to flush or to refresh for this tenant: spare the query, the sync runs for every tenant + return; + } + List allPersistedRuleStatisticsList = persistenceService.getAllItems(RuleStatistics.class); Map allPersistedRuleStatistics = new HashMap<>(); for (RuleStatistics ruleStatistics : allPersistedRuleStatisticsList) { allPersistedRuleStatistics.put(ruleStatistics.getItemId(), ruleStatistics); } - String currentTenant = contextManager.getCurrentContext().getTenantId(); - - Map tenantStats = getRuleStatisticsForTenant(currentTenant); - // Sync tenant statistics for (RuleStatistics ruleStatistics : tenantStats.values()) { boolean mustPersist = false; diff --git a/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java index 98b1772c44..b977f7329d 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java @@ -1273,20 +1273,20 @@ public void recalculatePastEventConditions(boolean sendProfileUpdateEvents) { int pastEventSegmentsAndScoringsSize = segmentOrScoringIdsToReevaluate.size(); LOGGER.info("Found {} segments or scoring plans containing pastEventCondition conditions", pastEventSegmentsAndScoringsSize); - // get Segments and Scoring that contains relative date expressions - String currentTenant = contextManager.getCurrentContext().getTenantId(); - Map tenantSegments = cacheService.getTenantCache(currentTenant, Segment.class); - Map tenantScoring = cacheService.getTenantCache(currentTenant, Scoring.class); + // get Segments and Scoring that contains relative date expressions, those inherited from the system + // tenant included: they apply to this tenant's profiles too (see getSegmentsAndScoresForProfile) + Collection tenantSegments = getAllItems(Segment.class, true); + Collection tenantScoring = getAllItems(Scoring.class, true); if (tenantSegments != null) { - segmentOrScoringIdsToReevaluate.addAll(tenantSegments.values().stream() + segmentOrScoringIdsToReevaluate.addAll(tenantSegments.stream() .filter(segment -> segment.getCondition() != null && segment.getCondition().toString().contains("propertyValueDateExpr")) .map(Item::getItemId) .collect(Collectors.toList())); } if (tenantScoring != null) { - segmentOrScoringIdsToReevaluate.addAll(tenantScoring.values().stream() + segmentOrScoringIdsToReevaluate.addAll(tenantScoring.stream() .filter(scoring -> scoring.getElements() != null && !scoring.getElements().isEmpty() && scoring.getElements().stream() .anyMatch(scoringElement -> scoringElement != null && scoringElement.getCondition() != null && scoringElement.getCondition().toString().contains("propertyValueDateExpr"))) .map(Item::getItemId) @@ -1558,8 +1558,11 @@ public void execute(ScheduledTask task, TaskExecutor.TaskStatusCallback callback // (system context alone only matches tenantId=system). boolean recalculated = executeForEachTenant("recalculation of date-relative segments and scoring", () -> recalculatePastEventConditions(), callback); - LOGGER.info("...{} recalculate segments and scoring that contains date relative conditions in {}ms. ", - recalculated ? "Finished" : "Failed for some tenants to", System.currentTimeMillis() - currentTimeMillis); + if (recalculated) { + LOGGER.info("...Finished recalculate segments and scoring that contains date relative conditions in {}ms. ", System.currentTimeMillis() - currentTimeMillis); + } else { + LOGGER.warn("...Failed for some tenants to recalculate segments and scoring that contains date relative conditions after {}ms. ", System.currentTimeMillis() - currentTimeMillis); + } } catch (Throwable t) { LOGGER.error("Error while updating profiles for segments and scoring that contains date relative conditions", t); callback.fail(t.getMessage()); diff --git a/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java b/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java index 9c9985f1f5..5a06b63321 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java @@ -17,8 +17,10 @@ package org.apache.unomi.services.impl; import org.apache.unomi.api.ExecutionContext; +import org.apache.unomi.api.Event; import org.apache.unomi.api.Metadata; import org.apache.unomi.api.Profile; +import org.apache.unomi.api.Session; import org.apache.unomi.api.conditions.Condition; import org.apache.unomi.api.rules.RuleStatistics; import org.apache.unomi.api.segments.Segment; @@ -153,6 +155,36 @@ public void profilePurgeTaskPurgesEveryTenantAndRestoresContext() throws Excepti assertEquals(beforeTenant, afterTenant, "tenant context must be restored after the purge task"); } + @Test + public void profilePurgeTaskPurgesSessionsAndEventsOfEveryTenant() throws Exception { + AtomicReference purgeExecutor = new AtomicReference<>(); + ProfileServiceImpl profileService = new ProfileServiceImpl(); + profileService.setPurgeSessionExistTime(30); + profileService.setPurgeEventExistTime(30); + initProfileService(profileService, executionContextManager, capturingScheduler("profile-purge", purgeExecutor)); + for (String tenantId : new String[]{TENANT_A, TENANT_B}) { + executionContextManager.executeAsTenant(tenantId, () -> { + persistenceService.save(session("old-session", new Date(0))); + persistenceService.save(session("recent-session", new Date())); + persistenceService.save(event("old-event", new Date(0))); + persistenceService.save(event("recent-event", new Date())); + }); + } + persistenceService.refresh(); + + assertNull(runExecutor(purgeExecutor.get()), "purge task should complete"); + persistenceService.refresh(); + + for (String tenantId : new String[]{TENANT_A, TENANT_B}) { + executionContextManager.executeAsTenant(tenantId, () -> { + assertNull(persistenceService.load("old-session", Session.class), "old session of " + tenantId + " must be purged"); + assertNotNull(persistenceService.load("recent-session", Session.class), "recent session of " + tenantId + " must be kept"); + assertNull(persistenceService.load("old-event", Event.class), "old event of " + tenantId + " must be purged"); + assertNotNull(persistenceService.load("recent-event", Event.class), "recent event of " + tenantId + " must be kept"); + }); + } + } + @Test public void profilePurgeFailureInOneTenantIsReportedAndDoesNotStopTheOthers() throws Exception { AtomicReference purgeExecutor = new AtomicReference<>(); @@ -233,6 +265,32 @@ public void segmentDateRecalculationTaskUpdatesProfilesOfEveryTenant() throws Ex "tenant2's profile must enter tenant2's date-relative segment only"); } + @Test + public void segmentDateRecalculationTaskAppliesSystemSegmentsToTenantProfiles() throws Exception { + AtomicReference segmentExecutor = new AtomicReference<>(); + SegmentServiceImpl segmentService = newSegmentService(executionContextManager, + capturingScheduler("segment-date-recalculation", segmentExecutor)); + // a date-relative segment owned by the system tenant applies to the profiles of every tenant + executionContextManager.executeAsSystem(() -> segmentService.setSegmentDefinition(inactiveSinceSegment("inactive-everywhere"))); + executionContextManager.executeAsTenant(TENANT_A, () -> { + Profile inactive = new Profile("inactive-profile"); + inactive.setProperty("lastVisit", new Date(0)); + persistenceService.save(inactive); + Profile active = new Profile("active-profile"); + active.setProperty("lastVisit", new Date()); + persistenceService.save(active); + }); + persistenceService.refresh(); + + assertNull(runExecutor(segmentExecutor.get()), "segment recalculation should complete"); + persistenceService.refresh(); + + assertEquals(Set.of("inactive-everywhere"), loadProfile(TENANT_A, "inactive-profile").getSegments(), + "a tenant profile must enter the system tenant's date-relative segment"); + assertTrue(loadProfile(TENANT_A, "active-profile").getSegments().isEmpty(), + "a profile that does not match must stay out of it"); + } + @Test public void segmentDateRecalculationFailureInOneTenantIsReportedAndDoesNotStopTheOthers() throws Exception { ExecutionContextManagerImpl spiedContext = spy(executionContextManager); @@ -321,6 +379,12 @@ public void systemContextPersistenceStillOnlySeesSystemTenant() { private ProfileServiceImpl newProfileService(ExecutionContextManagerImpl contextManager, SchedulerServiceImpl scheduler) { ProfileServiceImpl profileService = new ProfileServiceImpl(); + profileService.setPurgeProfileExistTime(30); + return initProfileService(profileService, contextManager, scheduler); + } + + private ProfileServiceImpl initProfileService(ProfileServiceImpl profileService, ExecutionContextManagerImpl contextManager, + SchedulerServiceImpl scheduler) { profileService.setBundleContext(bundleContext); profileService.setPersistenceService(persistenceService); profileService.setDefinitionsService(definitionsService); @@ -329,7 +393,6 @@ private ProfileServiceImpl newProfileService(ExecutionContextManagerImpl context profileService.setCacheService(multiTypeCacheService); profileService.setTenantService(tenantService); profileService.setSegmentService(mock(SegmentService.class)); - profileService.setPurgeProfileExistTime(30); profileService.setPurgeProfileInterval(1); profileService.postConstruct(); return profileService; @@ -409,6 +472,23 @@ private Segment inactiveSinceSegment(String segmentId) { return segment; } + private static Session session(String sessionId, Date timeStamp) { + Session session = new Session(sessionId, new Profile("profile-of-" + sessionId), timeStamp, "scope"); + // the in-memory persistence purges on the creation date + session.setCreationDate(timeStamp); + return session; + } + + private static Event event(String eventId, Date timeStamp) { + Event event = new Event(); + event.setItemId(eventId); + event.setEventType("view"); + event.setProfileId("profile-of-" + eventId); + event.setTimeStamp(timeStamp); + event.setCreationDate(timeStamp); + return event; + } + private void saveOldProfile(String tenantId, String profileId) { executionContextManager.executeAsTenant(tenantId, () -> { Profile old = new Profile(profileId); From 8f10aad3f8b773bf0a9ca59aa849c16292e5c950 Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Fri, 9 Oct 2026 09:51:25 +0200 Subject: [PATCH 5/6] UNOMI-1000: Always record a route refresh that was given up on Removing what a failed attempt left in the Camel context no longer keeps the configuration from being marked ROUTE_CREATION_FAILED when the refresh gives up. A tenant that cannot be switched to logs its stack trace once instead of once per queued configuration, and the related error messages name what actually failed. Covers the statistics sync skipping tenants with nothing to sync, makes the rule statistics IT wait until nothing is left to flush, keeps the routes IT cleanup from hiding the test's own failure, and notes in the migration guide that the migration tenant identifier must follow the tenant identifier rules. Co-Authored-By: Claude Opus 5.5 --- .../core/context/RouterCamelContext.java | 49 +++++++++++++------ .../RouterCamelContextTenantLoadTest.java | 2 +- .../unomi/itests/BackgroundTenantTasksIT.java | 33 +++++++++++-- .../migrations/migrate-3.0-to-4.0.adoc | 4 +- .../impl/segments/SegmentServiceImpl.java | 24 ++++----- .../impl/BackgroundTenantTasksTest.java | 25 ++++++++++ 6 files changed, 101 insertions(+), 36 deletions(-) diff --git a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java index ee4675e72c..4f4c18c937 100644 --- a/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java +++ b/extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java @@ -194,10 +194,12 @@ public void destroy() throws Exception { } //This is to shutdown Camel context //(will stop all routes/components/endpoints etc and clear internal state/cache) - if (camelContext != null) { - // null when init() failed before the context could be started - camelContext.stop(); + if (camelContext == null) { + // init() failed before the context could be started + LOGGER.info("Camel context for profile import was not started, nothing to shut down."); + return; } + camelContext.stop(); LOGGER.info("Camel context for profile import is shutdown."); } @@ -251,7 +253,7 @@ private void refreshRoutes(String direction, ImportExportConfigurationService routeCreationRetryCount.remove(retryKey); } } catch (Exception e) { - retryRefreshOrGiveUp(service, retryKey, refreshType, routeCreationFailureMarker, e); + retryRefreshOrGiveUp(service, retryKey, refreshType, routeCreationFailureMarker, e, true); } } }); @@ -262,8 +264,9 @@ private void refreshRoutes(String direction, ImportExportConfigurationService LOGGER.error("Could not switch to tenant {} to refresh its {} camel routes", tenantId, direction, e); for (Map.Entry configToRefresh : tenantConfigsToRefresh.getValue().entrySet()) { // no failure status can be recorded on a configuration of a tenant that cannot be switched to + // the stack trace was logged once above, for the tenant retryRefreshOrGiveUp(service, new RetryKey(direction, tenantId, configToRefresh.getKey()), configToRefresh.getValue(), - null, e); + null, e, false); } } } @@ -275,26 +278,43 @@ private void refreshRoutes(String direction, ImportExportConfigurationService */ private void retryRefreshOrGiveUp(ImportExportConfigurationService service, RetryKey retryKey, RouterConstants.CONFIG_CAMEL_REFRESH refreshType, Consumer routeCreationFailureMarker, - Exception failure) { + Exception failure, boolean logStackTrace) { String configId = retryKey.configId(); try { int attempt = routeCreationRetryCount.merge(retryKey, 1, Integer::sum); if (attempt <= MAX_ROUTE_CREATION_RETRIES) { - LOGGER.error("Refreshing({}) camel route {} failed (attempt {}/{}) — will retry on next tick", - refreshType, configId, attempt, MAX_ROUTE_CREATION_RETRIES, failure); + if (logStackTrace) { + LOGGER.error("Refreshing({}) camel route {} failed (attempt {}/{}) — will retry on next tick", + refreshType, configId, attempt, MAX_ROUTE_CREATION_RETRIES, failure); + } else { + LOGGER.error("Refreshing({}) camel route {} failed (attempt {}/{}) — will retry on next tick: {}", + refreshType, configId, attempt, MAX_ROUTE_CREATION_RETRIES, failure.toString()); + } service.requeueForRefresh(retryKey.tenantId(), configId, refreshType); } else { - LOGGER.error("Refreshing({}) camel route {} failed after {} attempts — giving up", - refreshType, configId, MAX_ROUTE_CREATION_RETRIES, failure); + if (logStackTrace) { + LOGGER.error("Refreshing({}) camel route {} failed after {} attempts — giving up", + refreshType, configId, MAX_ROUTE_CREATION_RETRIES, failure); + } else { + LOGGER.error("Refreshing({}) camel route {} failed after {} attempts — giving up: {}", + refreshType, configId, MAX_ROUTE_CREATION_RETRIES, failure.toString()); + } routeCreationRetryCount.remove(retryKey); - // drop what the last failed attempt may have left in the context - killExistingRoute(retryKey.tenantId(), configId, false); + try { + // drop what the last failed attempt may have left in the context + killExistingRoute(retryKey.tenantId(), configId, false); + } catch (Exception e) { + // must not keep the failure from being recorded on the configuration below + LOGGER.warn("Could not remove what is left of {} camel route {} of tenant {}", retryKey.direction(), configId, + retryKey.tenantId(), e); + } if (routeCreationFailureMarker != null) { routeCreationFailureMarker.accept(configId); } } } catch (Exception e) { - LOGGER.error("Could not record the failed refresh of camel route {} of tenant {}", configId, retryKey.tenantId(), e); + LOGGER.error("Could not record the failed refresh({}) of {} camel route {} of tenant {}", refreshType, retryKey.direction(), + configId, retryKey.tenantId(), e); } } @@ -401,8 +421,7 @@ private void startRecurrentRoutes(String tenantId, ImportExport } }); } catch (Exception e) { - LOGGER.error("Could not load router configurations for tenant {}, its recurring routes are not " - + "started; the load will be retried", tenantId, e); + LOGGER.error("Could not start the recurring routes of tenant {}; this will be retried", tenantId, e); tenantsToRetry.add(tenantId); tenantsWithLoggedLoadFailure.add(tenantId); } diff --git a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java index add424dbbd..dcc8b0b191 100644 --- a/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java +++ b/extensions/router/router-core/src/test/java/org/apache/unomi/router/core/context/RouterCamelContextTenantLoadTest.java @@ -265,7 +265,7 @@ public void refreshesOfATenantThatCanNeverBeSwitchedToAreGivenUpOn() throws Exce } @Test - public void shutdownAfterAFailedStartDoesNotFail() throws Exception { + public void shutdownOfARouterThatNeverStartedDoesNotFail() throws Exception { RouterCamelContext neverStarted = new RouterCamelContext(); neverStarted.destroy(); diff --git a/itests/src/test/java/org/apache/unomi/itests/BackgroundTenantTasksIT.java b/itests/src/test/java/org/apache/unomi/itests/BackgroundTenantTasksIT.java index 6d9cb36907..119f9e8018 100644 --- a/itests/src/test/java/org/apache/unomi/itests/BackgroundTenantTasksIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/BackgroundTenantTasksIT.java @@ -34,6 +34,8 @@ import org.ops4j.pax.exam.junit.PaxExam; import org.ops4j.pax.exam.spi.reactors.ExamReactorStrategy; import org.ops4j.pax.exam.spi.reactors.PerSuite; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.io.File; import java.time.LocalDate; @@ -50,6 +52,8 @@ @ExamReactorStrategy(PerSuite.class) public class BackgroundTenantTasksIT extends BaseIT { + private static final Logger LOGGER = LoggerFactory.getLogger(BackgroundTenantTasksIT.class); + private static final String RULE_ID = "unomi1000-statistics-rule"; private static final String EVENT_TYPE = "unomi1000StatisticsEvent"; private static final String SEGMENT_ID = "unomi1000-system-inactive-segment"; @@ -78,15 +82,23 @@ public void ruleStatisticsOfATenantRuleArePersisted() throws InterruptedExceptio createAndWaitForRule(rule); rulesService.refreshRules(); - // The refresh runs every 10 seconds by default; keep the rule firing until its count is stored. - RuleStatistics statistics = keepTrying("Statistics of a tenant rule were not persisted for the tenant", + // Fire the rule; its execution is first counted in memory. + keepTrying("The rule did not fire", () -> { Event event = new Event(EVENT_TYPE, null, profile, null, null, profile, new Date()); event.setPersistent(false); Assert.assertNotEquals(EventService.ERROR, eventService.send(event)); - return persistenceService.load(RULE_ID, RuleStatistics.class); + return rulesService.getRuleStatistics(RULE_ID); }, - stored -> stored != null && stored.getExecutionCount() > 0, + counted -> counted != null && counted.getLocalExecutionCount() + counted.getExecutionCount() > 0, + DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES); + + // The rule statistics refresh runs every 10 seconds by default. Wait until it has stored the count + // and nothing is left to flush, so that no later refresh stores statistics for the removed rule. + RuleStatistics statistics = keepTrying("Statistics of a tenant rule were not persisted for the tenant", + () -> persistenceService.load(RULE_ID, RuleStatistics.class), + stored -> stored != null && stored.getExecutionCount() > 0 + && rulesService.getRuleStatistics(RULE_ID).getLocalExecutionCount() == 0, 1000, 60); Assert.assertEquals("Statistics must be stored in the tenant owning the rule", TEST_TENANT_ID, statistics.getTenantId()); } finally { @@ -169,12 +181,23 @@ public void sameImportConfigurationIdInTwoTenantsGetsTwoRoutes() throws Interrup Assert.assertTrue("Deleting one tenant's configuration must not remove the route of another tenant's configuration", camelRouteExists(TEST_TENANT_ID, IMPORT_CONFIG_ID)); } finally { + // Clean up without hiding what the test itself reported. try { importConfigurationService.delete(IMPORT_CONFIG_ID); keepTrying("The route of the test tenant's configuration is still there after its deletion", () -> camelRouteExists(TEST_TENANT_ID, IMPORT_CONFIG_ID), exists -> !exists, 1000, 30); - } finally { + } catch (Exception | AssertionError cleanupFailure) { + LOGGER.warn("Could not remove the import configuration of the test tenant", cleanupFailure); + } + try { + executionContextManager.executeAsTenant(otherTenantId, () -> { + if (importConfigurationService.load(IMPORT_CONFIG_ID) != null) { + importConfigurationService.delete(IMPORT_CONFIG_ID); + } + }); tenantService.deleteTenant(otherTenantId); + } catch (Exception cleanupFailure) { + LOGGER.warn("Could not remove tenant {}", otherTenantId, cleanupFailure); } } } diff --git a/manual/src/main/asciidoc/migrations/migrate-3.0-to-4.0.adoc b/manual/src/main/asciidoc/migrations/migrate-3.0-to-4.0.adoc index d8e967239b..5444d8cf6a 100644 --- a/manual/src/main/asciidoc/migrations/migrate-3.0-to-4.0.adoc +++ b/manual/src/main/asciidoc/migrations/migrate-3.0-to-4.0.adoc @@ -310,7 +310,9 @@ migrated data included. tenant. Code that looks routes up in the router's Camel context must build the identifier with `org.apache.unomi.router.api.RouteIds`. The `killExistingRoute`, `updateProfileImportReaderRoute` and `updateProfileExportReaderRoute` methods of `IRouterCamelContext` take the tenant identifier as their first - parameter. + parameter. A tenant identifier containing a colon gets no recurrent route: the identifier given to + `org.apache.unomi.migration.tenant.id` must follow the tenant identifier rules (letters, digits, hyphens and + underscores). ==== Migration Steps diff --git a/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java index b977f7329d..ed1b71911c 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java @@ -1278,20 +1278,16 @@ public void recalculatePastEventConditions(boolean sendProfileUpdateEvents) { Collection tenantSegments = getAllItems(Segment.class, true); Collection tenantScoring = getAllItems(Scoring.class, true); - if (tenantSegments != null) { - segmentOrScoringIdsToReevaluate.addAll(tenantSegments.stream() - .filter(segment -> segment.getCondition() != null && segment.getCondition().toString().contains("propertyValueDateExpr")) - .map(Item::getItemId) - .collect(Collectors.toList())); - } - - if (tenantScoring != null) { - segmentOrScoringIdsToReevaluate.addAll(tenantScoring.stream() - .filter(scoring -> scoring.getElements() != null && !scoring.getElements().isEmpty() && scoring.getElements().stream() - .anyMatch(scoringElement -> scoringElement != null && scoringElement.getCondition() != null && scoringElement.getCondition().toString().contains("propertyValueDateExpr"))) - .map(Item::getItemId) - .collect(Collectors.toList())); - } + segmentOrScoringIdsToReevaluate.addAll(tenantSegments.stream() + .filter(segment -> segment.getCondition() != null && segment.getCondition().toString().contains("propertyValueDateExpr")) + .map(Item::getItemId) + .collect(Collectors.toList())); + + segmentOrScoringIdsToReevaluate.addAll(tenantScoring.stream() + .filter(scoring -> scoring.getElements() != null && !scoring.getElements().isEmpty() && scoring.getElements().stream() + .anyMatch(scoringElement -> scoringElement != null && scoringElement.getCondition() != null && scoringElement.getCondition().toString().contains("propertyValueDateExpr"))) + .map(Item::getItemId) + .collect(Collectors.toList())); LOGGER.info("Found {} segments or scoring plans containing date relative expressions", segmentOrScoringIdsToReevaluate.size() - pastEventSegmentsAndScoringsSize); // reevaluate segments and scoring. diff --git a/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java b/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java index 5a06b63321..4870e79d11 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/BackgroundTenantTasksTest.java @@ -65,10 +65,14 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doThrow; 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; import static org.mockito.Mockito.when; /** @@ -329,6 +333,23 @@ public void ruleStatisticsRefreshPersistsEachTenantsOwnStatistics() throws Excep assertNull(loadRuleStatistics(TENANT_B, "a-rule"), "tenant2 must not receive tenant1's statistics"); } + @Test + public void ruleStatisticsRefreshOnlyQueriesPersistenceForTenantsWithStatistics() throws Exception { + AtomicReference statsExecutor = new AtomicReference<>(); + PersistenceService spiedPersistence = spy(persistenceService); + RulesServiceImpl rulesService = newRulesService(capturingScheduler("rules-statistics-refresh", statsExecutor), spiedPersistence); + + clearInvocations(spiedPersistence); + assertNull(runExecutor(statsExecutor.get()), "statistics refresh should complete"); + verify(spiedPersistence, never()).getAllItems(RuleStatistics.class); + + saveRuleStatistics(TENANT_A, "a-rule", 10); + executionContextManager.executeAsTenant(TENANT_A, () -> rulesService.getRuleStatistics("a-rule")); + clearInvocations(spiedPersistence); + assertNull(runExecutor(statsExecutor.get()), "statistics refresh should complete"); + verify(spiedPersistence, times(1)).getAllItems(RuleStatistics.class); + } + @Test public void ruleStatisticsRefreshKeepsSystemStatisticsInTheSystemTenant() throws Exception { AtomicReference statsExecutor = new AtomicReference<>(); @@ -418,6 +439,10 @@ private SegmentServiceImpl newSegmentService(ExecutionContextManagerImpl context } private RulesServiceImpl newRulesService(SchedulerServiceImpl scheduler) { + return newRulesService(scheduler, persistenceService); + } + + private RulesServiceImpl newRulesService(SchedulerServiceImpl scheduler, PersistenceService persistenceService) { RulesServiceImpl rulesService = new RulesServiceImpl(); rulesService.setBundleContext(bundleContext); rulesService.setPersistenceService(persistenceService); From 4e5ba0c7c6c8105eb2934c223c38fa3aa98f25c6 Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Fri, 9 Oct 2026 10:55:26 +0200 Subject: [PATCH 6/6] UNOMI-1000: Send the statistics IT event in the rule's scope A rule only applies to events of its own scope, so the event sent by ruleStatisticsOfATenantRuleArePersisted never fired the rule and the test failed before reaching the statistics refresh it is about. Co-Authored-By: Claude Opus 5.5 --- .../org/apache/unomi/itests/BackgroundTenantTasksIT.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/itests/src/test/java/org/apache/unomi/itests/BackgroundTenantTasksIT.java b/itests/src/test/java/org/apache/unomi/itests/BackgroundTenantTasksIT.java index 119f9e8018..ccf0a8728b 100644 --- a/itests/src/test/java/org/apache/unomi/itests/BackgroundTenantTasksIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/BackgroundTenantTasksIT.java @@ -56,6 +56,7 @@ public class BackgroundTenantTasksIT extends BaseIT { private static final String RULE_ID = "unomi1000-statistics-rule"; private static final String EVENT_TYPE = "unomi1000StatisticsEvent"; + private static final String SCOPE = "test-scope"; private static final String SEGMENT_ID = "unomi1000-system-inactive-segment"; private static final String IMPORT_CONFIG_ID = "unomi1000-shared-import"; private static final String OTHER_TENANT_ID = "unomi1000-router-tenant"; @@ -75,17 +76,17 @@ public void ruleStatisticsOfATenantRuleArePersisted() throws InterruptedExceptio Action action = new Action(definitionsService.getActionType("setPropertyAction")); action.setParameter("propertyName", "unomi1000Statistics"); action.setParameter("propertyValue", "fired"); - Rule rule = new Rule(new Metadata("test-scope", RULE_ID, RULE_ID, "Counts its executions for UNOMI-1000")); + Rule rule = new Rule(new Metadata(SCOPE, RULE_ID, RULE_ID, "Counts its executions for UNOMI-1000")); rule.setCondition(condition); rule.setActions(Collections.singletonList(action)); try { createAndWaitForRule(rule); rulesService.refreshRules(); - // Fire the rule; its execution is first counted in memory. + // Fire the rule; its execution is first counted in memory. A rule only applies to events of its scope. keepTrying("The rule did not fire", () -> { - Event event = new Event(EVENT_TYPE, null, profile, null, null, profile, new Date()); + Event event = new Event(EVENT_TYPE, null, profile, SCOPE, null, profile, new Date()); event.setPersistent(false); Assert.assertNotEquals(EventService.ERROR, eventService.send(event)); return rulesService.getRuleStatistics(RULE_ID);