gnodet-bot commented on code in PR #27524:
URL: https://github.com/apache/camel/pull/27524#discussion_r4215501727
##########
core/camel-api/src/main/java/org/apache/camel/spi/RoutesLoader.java:
##########
@@ -124,6 +126,10 @@ default Set<String> updateRoutes(Resource... resources)
throws Exception {
* If a route is loaded with a route id for an existing route, then the
existing route is stopped and remove, so it
* can be updated.
*
+ * The default implementation runs concurrent calls of the {@code
updateRoutes} methods one at a time, so they can
Review Comment:
⚠️ **Misleading Javadoc:** `updateRoutes(Collection)` is an **abstract**
method — it has no "default implementation". The Javadoc here describes the
contract/expectation of implementations, but "The default implementation"
implies a concrete `default` method body. Replace with something like:
```suggestion
* Implementations of this method should serialize concurrent calls,
e.g. via a {@link java.util.concurrent.locks.ReentrantLock},
```
##########
core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/DefaultRoutesLoader.java:
##########
@@ -292,6 +297,16 @@ public Set<String> updateRoutes(Collection<Resource>
resources) throws Exception
return answer;
}
+ updateLock.lock();
Review Comment:
💡 **Concurrency:** Consider `lockInterruptibly()` instead of `lock()`. With
`lock()`, a thread blocked waiting for the lock cannot be interrupted (e.g.
during Camel context shutdown). `lockInterruptibly()` allows the waiting thread
to be unblocked if interrupted, which makes shutdown more responsive when
multiple threads are competing for route updates.
```suggestion
updateLock.lockInterruptibly();
```
If you use `lockInterruptibly()`, you'd need to add `InterruptedException`
to the `throws` clause (or wrap it), but it's worth considering given this is
called during live route reloading.
##########
core/camel-api/src/main/java/org/apache/camel/spi/RoutesLoader.java:
##########
@@ -110,6 +110,8 @@ default void loadRoutes(Resource... resources) throws
Exception {
* If a route is loaded with a route id for an existing route, then the
existing route is stopped and remove, so it
* can be updated.
*
+ * The default implementation runs concurrent calls one at a time, see
{@link #updateRoutes(Collection)}.
Review Comment:
⚠️ **Misleading Javadoc:** This `default` varargs method says _"The default
implementation runs concurrent calls one at a time, see
`#updateRoutes(Collection)`"_ — but `updateRoutes(Collection)` is an
**abstract** method, not a `default` one. The "default implementation" that
actually serializes calls is `DefaultRoutesLoader`. Consider naming it
explicitly:
```suggestion
* Concurrent calls are serialized by {@link
org.apache.camel.impl.engine.DefaultRoutesLoader}.
```
--
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]
