Dhairya-007 commented on code in PR #29681:
URL: https://github.com/apache/camel/pull/29681#discussion_r4552351211
##########
components/camel-vertx/camel-vertx/src/main/java/org/apache/camel/component/vertx/VertxComponent.java:
##########
@@ -162,34 +164,62 @@ protected void doStart() throws Exception {
createdVertx = true;
final CountDownLatch latch = new CountDownLatch(1);
+ final AtomicReference<Throwable> failure = new AtomicReference<>();
+ // the callback writes here rather than to the field, so a build
that completes after the timeout
+ // cannot leave a half-published Vert.x behind
+ final AtomicReference<Vertx> created = new AtomicReference<>();
+ Future<Vertx> clusteredVertx = null;
// lets using a host / port if a host name is specified
if (clustered) {
+ if (timeout <= 0) {
+ throw new IllegalArgumentException(
+ "The timeout option must be a positive number of
seconds, was: " + timeout);
+ }
+
LOG.info("Creating Clustered Vertx {}:{}",
vertxOptions.getEventBusOptions().getHost(),
vertxOptions.getEventBusOptions().getPort());
// use the async api as we want to wait for the eventbus to be
ready before we are in started state
- vertxFactory.buildClustered().onComplete(event -> {
+ clusteredVertx = vertxFactory.buildClustered();
+ clusteredVertx.onComplete(event -> {
if (event.cause() != null) {
- LOG.warn("Error creating Clustered Vertx {}:{} due
{}", host, port,
- event.cause().getMessage(), event.cause());
+ failure.set(event.cause());
} else if (event.succeeded()) {
- vertx = event.result();
- LOG.info("EventBus is ready: {}", vertx);
+ created.set(event.result());
}
latch.countDown();
});
} else {
LOG.info("Creating Non-Clustered Vertx");
- vertx = vertxFactory.build();
- LOG.info("EventBus is ready: {}", vertx);
+ created.set(vertxFactory.build());
latch.countDown();
}
if (latch.getCount() > 0) {
LOG.info("Waiting for EventBus to be ready using {} sec as
timeout", timeout);
- latch.await(timeout, TimeUnit.SECONDS);
+ if (!latch.await(timeout, TimeUnit.SECONDS)) {
+ // the build is still running: a node that joins the
cluster after we gave up would keep its
+ // event-loop threads alive with nobody left to close it.
onSuccess also fires at once if the
+ // future completed between the await and this
registration, so no window is left open
+ clusteredVertx.onSuccess(Vertx::close);
Review Comment:
Agreed, and applied — a close with no explanation is the worst of both
worlds, since the operator sees the threads go away and has nothing to
correlate it with.
I kept your logic and changed the message on two counts:
```java
clusteredVertx.onSuccess(v -> {
LOG.warn("Closing the Vert.x that became ready after the timeout of {}
seconds,"
+ " as the component start has already failed",
timeout);
v.close();
});
```
1. **ASCII.** The em dash in the suggestion was, after applying it, the only
non-ASCII byte in the whole of `camel-vertx/src/main/java` — I checked with
`grep -l '[^ -~\t]'` over the tree. It compiles either way, but I would rather
not be the commit that introduces one.
2. **The timeout value.** `{}` seconds ties the warning to the
`IllegalStateException` thrown immediately after it, which quotes the same
number. Without it the two lines in the log cannot be told apart from an
ordinary shutdown closing a Vert.x.
I also dropped "to prevent resource leak": at that point nothing has leaked,
which is the whole purpose of the block, so naming the leak in the message
reads like a report of one.
**Also rebased on main** — the branch had gone `CONFLICTING` on
`camel-4x-upgrade-guide-4_24.adoc`, where main added the camel-docling
OCR-headers entry in the same place. Both entries are kept, main's first.
--
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]
