Copilot commented on code in PR #779:
URL: https://github.com/apache/unomi/pull/779#discussion_r3448870932
##########
extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java:
##########
@@ -163,15 +166,27 @@ public void run() {
contextManager.executeAsTenant(tenantId, () -> {
try {
for (Map.Entry<String,
RouterConstants.CONFIG_CAMEL_REFRESH> importConfigToRefresh :
tenantImportConfigsToRefresh.getValue().entrySet()) {
+ String configId =
importConfigToRefresh.getKey();
+ RouterConstants.CONFIG_CAMEL_REFRESH
refreshType = importConfigToRefresh.getValue();
try {
- if
(importConfigToRefresh.getValue().equals(RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED))
{
-
updateProfileImportReaderRoute(importConfigToRefresh.getKey(), true);
- } else if
(importConfigToRefresh.getValue().equals(RouterConstants.CONFIG_CAMEL_REFRESH.REMOVED))
{
-
killExistingRoute(importConfigToRefresh.getKey(), true);
+ if
(refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED)) {
+
updateProfileImportReaderRoute(configId, true);
+
routeCreationRetryCount.remove(configId);
+ } else if
(refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.REMOVED)) {
+ killExistingRoute(configId, true);
+
routeCreationRetryCount.remove(configId);
}
} catch (Exception e) {
- LOGGER.error("Unexpected error while
refreshing({}) camel route: {}", importConfigToRefresh.getValue(),
-
importConfigToRefresh.getKey(), e);
+ int attempt =
routeCreationRetryCount.merge(configId, 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);
Review Comment:
The retry counter is keyed only by `configId`, which can collide across
tenants and between import vs export configs (same id), causing retries to
interfere with each other. Also, the `attempt <= MAX_ROUTE_CREATION_RETRIES`
condition allows 6 failures when MAX=5 (attempts 1..5 still requeue, then gives
up on 6). Key the counter by tenant+direction+configId and stop re-queuing when
the failure count reaches MAX.
##########
extensions/router/router-core/src/main/java/org/apache/unomi/router/core/route/ProfileImportFromSourceRouteBuilder.java:
##########
@@ -137,7 +137,8 @@ public void configure() throws Exception {
lineSplitProcessor.setProfilePropertyTypes(profileService.getTargetPropertyTypes("profiles"));
String endpoint = (String)
importConfiguration.getProperties().get("source");
- endpoint += "&moveFailed=.error";
+ // Poll immediately when the route starts rather than waiting
the default 1-second initialDelay.
+ endpoint += "&initialDelay=0&moveFailed=.error";
if (StringUtils.isNotBlank(endpoint) &&
allowedEndpoints.contains(endpoint.substring(0, endpoint.indexOf(':')))) {
Review Comment:
`endpoint` can be null when the import configuration properties don’t
contain a `source` key (the current guard only checks `properties.size() > 0`).
The new `endpoint += ...` will then throw an NPE during route configuration,
which can prevent other routes from being created.
##########
extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java:
##########
@@ -187,15 +202,27 @@ public void run() {
contextManager.executeAsTenant(tenantId, () -> {
try {
for (Map.Entry<String,
RouterConstants.CONFIG_CAMEL_REFRESH> exportConfigToRefresh :
tenantExportConfigsToRefresh.getValue().entrySet()) {
+ String configId =
exportConfigToRefresh.getKey();
+ RouterConstants.CONFIG_CAMEL_REFRESH
refreshType = exportConfigToRefresh.getValue();
try {
- if
(exportConfigToRefresh.getValue().equals(RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED))
{
-
updateProfileExportReaderRoute(exportConfigToRefresh.getKey(), true);
- } else if
(exportConfigToRefresh.getValue().equals(RouterConstants.CONFIG_CAMEL_REFRESH.REMOVED))
{
-
killExistingRoute(exportConfigToRefresh.getKey(), true);
+ if
(refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.UPDATED)) {
+
updateProfileExportReaderRoute(configId, true);
+
routeCreationRetryCount.remove(configId);
+ } else if
(refreshType.equals(RouterConstants.CONFIG_CAMEL_REFRESH.REMOVED)) {
+ killExistingRoute(configId, true);
+
routeCreationRetryCount.remove(configId);
}
Review Comment:
In the export refresh loop, failures are counted by `configId` only (same
collision risk as imports), and the `attempt <= MAX_ROUTE_CREATION_RETRIES`
check allows one more failure than intended. Additionally,
`updateProfileExportReaderRoute()` returns normally when the config cannot be
loaded, which means the retry logic won’t trigger and the retry counter is
cleared even though the route wasn’t recreated. Pre-check `load()` on UPDATED
to force a retry when the config isn’t available, and key/cap retries
consistently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
