[GitHub] [camel] essobedo commented on a diff in pull request #8785: CAMEL-18766 Fixes bug in camel-support

[GitBox <gi...@apache.org>]

essobedo commented on code in PR #8785:
URL: https://github.com/apache/camel/pull/8785#discussion_r1033903601


##########
core/camel-support/src/main/java/org/apache/camel/support/task/BackgroundTask.java:
##########
@@ -85,78 +85,58 @@ public BackgroundTask build() {
         this.name = name;
     }
 
-    private <T> void runTaskWrapper(CountDownLatch latch, Predicate<T> predicate, T payload) {
+    private void runTaskWrapper(CountDownLatch latch, BooleanSupplier supplier, ExecutionResult result) {
         LOG.trace("Current latch value: {}", latch.getCount());
-
         if (latch.getCount() == 0) {
             return;
         }
 
         if (!budget.next()) {
             LOG.warn("The task {} does not have more budget to continue running", name);
-
-            return;
-        }
-
-        if (predicate.test(payload)) {
+            result.completed = false;
             latch.countDown();
-            LOG.trace("Task {} has succeeded and the current task won't be schedulable anymore: {}", name, latch.getCount());
-        }
-    }
-
-    private void runTaskWrapper(CountDownLatch latch, BooleanSupplier supplier) {
-        LOG.trace("Current latch value: {}", latch.getCount());
-        if (latch.getCount() == 0) {
-            return;
-        }
-
-        if (!budget.next()) {
-            LOG.warn("The task {} does not have more budget to continue running", name);
-
             return;
         }
 
         if (supplier.getAsBoolean()) {
+            result.completed = true;
             latch.countDown();
             LOG.trace("Task {} succeeded and the current task won't be schedulable anymore: {}", name, latch.getCount());
         }
     }
 
     @Override
     public <T> boolean run(Predicate<T> predicate, T payload) {
-        CountDownLatch latch = new CountDownLatch(1);
-
-        Future<?> task = service.scheduleAtFixedRate(() -> runTaskWrapper(latch, predicate, payload),
-                budget.initialDelay(), budget.interval(), TimeUnit.MILLISECONDS);
-
-        return waitForTaskCompletion(latch, task);
+        return this.run(() -> predicate.test(payload));

Review Comment:
   I'm wondering if it could be moved into the interface `BlockingTask` as the default implementation?



##########
core/camel-support/src/test/java/org/apache/camel/support/task/BackgroundIterationTimeTaskTest.java:
##########
@@ -42,7 +42,8 @@ void testRunNoMoreSupplier() {
                         .withMaxIterations(3)
                         .withInterval(Duration.ofSeconds(1))
                         .withInitialDelay(Duration.ZERO)
-                        .withMaxDuration(Duration.ofSeconds(5))
+                        // use unlimitedduration so we're sure that the task is really canceled after maxIterations

Review Comment:
   ```suggestion
                           // use unlimited duration so we're sure that the task is really canceled after maxIterations
   ```



##########
core/camel-support/src/main/java/org/apache/camel/support/task/BackgroundTask.java:
##########
@@ -167,12 +147,14 @@ private boolean waitForTaskCompletion(CountDownLatch latch, Future<?> task) {
         } finally {
             elapsed = budget.elapsed();
         }
-
-        return completed;
     }
 
     @Override
     public Duration elapsed() {
         return elapsed;
     }
+
+    private static class ExecutionResult {
+        private boolean completed;

Review Comment:
   @orpiske This makes me wonder if `BackgroundTask` is stateful or stateless. I mean can run several times the same task? because if not, I guess that the `CountDownLatch` and this field could be moved directly as fields of `BackgroundTask` otherwise maybe we could add the `CountDownLatch` to that class and rename it to something like `ExecutionState`



##########
core/camel-support/src/main/java/org/apache/camel/support/task/BackgroundTask.java:
##########
@@ -85,78 +85,58 @@ public BackgroundTask build() {
         this.name = name;
     }
 
-    private <T> void runTaskWrapper(CountDownLatch latch, Predicate<T> predicate, T payload) {
+    private void runTaskWrapper(CountDownLatch latch, BooleanSupplier supplier, ExecutionResult result) {
         LOG.trace("Current latch value: {}", latch.getCount());
-
         if (latch.getCount() == 0) {
             return;
         }
 
         if (!budget.next()) {
             LOG.warn("The task {} does not have more budget to continue running", name);
-
-            return;
-        }
-
-        if (predicate.test(payload)) {
+            result.completed = false;

Review Comment:
   Is it really needed since `false` is the default value for a `boolean`?



-- 
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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org