sergehuber commented on code in PR #779:
URL: https://github.com/apache/unomi/pull/779#discussion_r3472727909
##########
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:
Good catch, though it wasn't quite an NPE — `String += null` just produces
the literal `"null..."` string in Java, no exception there. But you're right
that this was unsafe: a missing `source` property would flow through and blow
up later with a `StringIndexOutOfBoundsException` once we hit
`endpoint.substring(0, endpoint.indexOf(':'))`, since there's no `:` in
`"null&..."`. Fixed by bailing out early (with a log line) when the endpoint is
blank or has no scheme, before any of that runs.
##########
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:
Fixed the key collision — replaced the bare `configId` key with a proper
`RetryKey` record scoped by direction + tenant + config, so import/export and
different tenants can never collide. In practice config IDs are random UUIDs so
this was always low-risk, but the record makes it correct by construction
instead of by luck.
On the off-by-one: I don't think that one's a bug.
`MAX_ROUTE_CREATION_RETRIES` is retries, not total attempts — 5 retries after
the first failure means 6 tries total, which is what the code does. Happy to
rename the constant if the current semantics are confusing, but I'd rather not
change the actual cap.
##########
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:
Same key-collision fix as above applies here too (shared `RetryKey`). Good
catch on `updateProfileExportReaderRoute()` though — it was silently returning
instead of throwing on a missing config, so the retry logic never actually
kicked in for exports. Made it throw `IllegalStateException` just like the
import path now does, so a missing config gets retried instead of quietly
giving up on the first try.
--
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]
