gnodet-bot commented on code in PR #26641:
URL: https://github.com/apache/camel/pull/26641#discussion_r4059739259
##########
core/camel-support/src/main/java/org/apache/camel/support/RouteWatcherReloadStrategy.java:
##########
@@ -385,10 +385,52 @@ protected void onRouteReload(Collection<Resource>
resources, boolean removeEvery
}
}
} catch (Exception e) {
+ // the routes that ran before were removed above and the new ones
failed to load: the app has no routes
+ // until the next successful reload. Restore the previous routes
now, without the failed resources, so a
+ // mistake in one file leaves the rest running (CAMEL-24860); the
failed file loads on its next save
+ restorePreviousRoutes(resources, e);
throw RuntimeCamelException.wrapRuntimeException(e);
}
}
+ /**
+ * Reloads the sources of the routes that ran before a failed reload,
without the resources that failed, so a
+ * mistake in one file does not leave the application without routes. The
previous set stays remembered, so the next
+ * successful reload of the failed file brings everything back together.
+ */
+ protected void restorePreviousRoutes(Collection<Resource> failed,
Exception cause) {
Review Comment:
⚠️ **Unused parameter `cause`** — `Exception cause` is declared in the
signature but never referenced in the method body. The inner `catch` block
introduces its own `e`. On a `protected` method this is leaked API: subclasses
that override the method and add their own logic will be confused by a
parameter that the base implementation ignores.
Either log the original failure cause in the WARN messages (useful to
correlate the restore attempt with the load error), or remove the parameter:
```suggestion
protected void restorePreviousRoutes(Collection<Resource> failed) {
```
And at the call site:
```suggestion
restorePreviousRoutes(resources);
```
##########
core/camel-support/src/main/java/org/apache/camel/support/RouteWatcherReloadStrategy.java:
##########
@@ -385,10 +385,52 @@ protected void onRouteReload(Collection<Resource>
resources, boolean removeEvery
}
}
} catch (Exception e) {
+ // the routes that ran before were removed above and the new ones
failed to load: the app has no routes
+ // until the next successful reload. Restore the previous routes
now, without the failed resources, so a
+ // mistake in one file leaves the rest running (CAMEL-24860); the
failed file loads on its next save
+ restorePreviousRoutes(resources, e);
throw RuntimeCamelException.wrapRuntimeException(e);
}
}
+ /**
+ * Reloads the sources of the routes that ran before a failed reload,
without the resources that failed, so a
+ * mistake in one file does not leave the application without routes. The
previous set stays remembered, so the next
+ * successful reload of the failed file brings everything back together.
Review Comment:
📝 **Inaccurate Javadoc** — `"The previous set stays remembered, so the next
successful reload of the failed file brings everything back together."` is
wrong: `previousSources.clear()` is called unconditionally after a successful
restore (line 423), so the set is **not** remembered. The next reload collects
its source list fresh from the running routes via
`getCamelContext().getRoutes()`.
Suggested correction:
```suggestion
* mistake in one file does not leave the application without routes.
After a successful restore,
* {@code previousSources} is cleared; the next reload re-collects its
source list from the
* running routes as usual.
```
--
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]
