You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2022/11/28 14:17:41 UTC
[GitHub] [camel] thuri opened a new pull request, #8785: CAMEL-18766 Fixes bug in camel-support
thuri opened a new pull request, #8785:
URL: https://github.com/apache/camel/pull/8785
- [X] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
- [X] Each commit in the pull request should have a meaningful subject line and body.
- [X] If you're unsure, you can format the pull request title like `[CAMEL-XXX] Fixes bug in camel-file component`, where you replace `CAMEL-XXX` with the appropriate JIRA issue.
- [X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [X] Run `mvn clean install -Psourcecheck` in your module with source check enabled to make sure basic checks pass and there are no checkstyle violations. A more thorough check will be performed on your pull request automatically.
Below are the contribution guidelines:
https://github.com/apache/camel/blob/main/CONTRIBUTING.md
The changes in this PR fix a bug in the BackgroundTask class which results in a thread that loops forever doing nothing but log a warning.
The fix is essentially ending the thread when the budget as been consumed. What that means is dependent on the actual implementation of the Budget instance that is passed to the BackgroundThread. It may mean that the `maxDuration` has been exceeded or that the task has been run for `maxIteration` times.
The new implementation saves the result of the runTaskWrapper method in an wrapper instance so that it can be used as return value of the run method implementation.
This is necessary because the Future, returned by the scheduleAtFixedRate method can't be used for that as the invocation of its `get` method would result in an exception.
--
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
[GitHub] [camel] github-actions[bot] commented on pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #8785:
URL: https://github.com/apache/camel/pull/8785#issuecomment-1331165005
### Components tested:
| Total | Tested | Failed :x: | Passed :white_check_mark: |
| --- | --- | --- | --- |
| 6 | 6 | 1 | 6 |
--
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
[GitHub] [camel] github-actions[bot] commented on pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #8785:
URL: https://github.com/apache/camel/pull/8785#issuecomment-1329189712
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:warning: Please note that the changes on this PR may be **tested automatically**.
If necessary Apache Camel Committers may access logs and test results in the job summaries!
--
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
[GitHub] [camel] github-actions[bot] commented on pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #8785:
URL: https://github.com/apache/camel/pull/8785#issuecomment-1329645274
:no_entry_sign: There are (likely) no components to be tested in this PR
--
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
[GitHub] [camel] essobedo commented on a diff in pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
essobedo commented on code in PR #8785:
URL: https://github.com/apache/camel/pull/8785#discussion_r1033912735
##########
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 we run several times the same background 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`
--
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
[GitHub] [camel] orpiske commented on a diff in pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
orpiske commented on code in PR #8785:
URL: https://github.com/apache/camel/pull/8785#discussion_r1034416912
##########
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:
Yes, it's stateful because of `budget` and `elapsed`.
I think your suggestion is +/- in line with what I commented on the Zulip chat: to investigate moving the latch into the budget and consider it as part of that. Granted, I did not comment on the elapsed, but I think it would eventually become a natural move as well.
IMHO, making (all) the tasks stateless - keeping the state on the budget itself - could potentially be a better approach for the future (maybe as part of Camel 4?), but I think the current proposed solution is fine.
--
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
[GitHub] [camel] essobedo commented on pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
essobedo commented on PR #8785:
URL: https://github.com/apache/camel/pull/8785#issuecomment-1331860527
Click on Details then Summary, you will then see the link to download the different log files at the bottom of the page.
In your case the checkstyle error is:
```
[camel-support] [INFO] Starting audit...
[ERROR] /home/runner/work/camel/camel/core/camel-support/src/main/java/org/apache/camel/support/task/BlockingTask.java:40:5: File contains tab characters (this is the first instance). [FileTabCharacter]
```
Always tries to launch the command `mvn clean install -Psourcecheck` against the modules in which you have updated files
--
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
[GitHub] [camel] orpiske commented on pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
orpiske commented on PR #8785:
URL: https://github.com/apache/camel/pull/8785#issuecomment-1333305543
> ### Components tested:
> Total Tested Failed ❌ Passed ✅
> 13 13 1 13
Failed caused by a well known flakiness on the core-management tests. Therefore, merging.
--
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
[GitHub] [camel] essobedo commented on a diff in pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
essobedo commented on code in PR #8785:
URL: https://github.com/apache/camel/pull/8785#discussion_r1033918273
##########
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:
@orpiske any opinion on this?
--
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
[GitHub] [camel] github-actions[bot] commented on pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #8785:
URL: https://github.com/apache/camel/pull/8785#issuecomment-1332747389
### Components tested:
| Total | Tested | Failed :x: | Passed :white_check_mark: |
| --- | --- | --- | --- |
| 13 | 13 | 1 | 13 |
--
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
[GitHub] [camel] essobedo commented on pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
essobedo commented on PR #8785:
URL: https://github.com/apache/camel/pull/8785#issuecomment-1331900317
> Actually, only us committters have access to those files (due to Apache rules). So, in cases like these, we have to upload them in the comments.
I was not aware of it, thx for the info. This can be annoying in the long term
--
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
[GitHub] [camel] orpiske commented on pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
orpiske commented on PR #8785:
URL: https://github.com/apache/camel/pull/8785#issuecomment-1333310268
BTW, I am going to backport this. I already have a build in progress and will open the PR soon.
@thuri thank you for contributing with this fix!
--
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
[GitHub] [camel] orpiske commented on a diff in pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
orpiske commented on code in PR #8785:
URL: https://github.com/apache/camel/pull/8785#discussion_r1034417403
##########
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:
+1. That would be better / cleaner.
--
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
[GitHub] [camel] orpiske merged pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
orpiske merged PR #8785:
URL: https://github.com/apache/camel/pull/8785
--
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
[GitHub] [camel] essobedo commented on a diff in pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by 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
[GitHub] [camel] orpiske commented on pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
orpiske commented on PR #8785:
URL: https://github.com/apache/camel/pull/8785#issuecomment-1331131377
>
Thanks for the heads up. I'll retest tonight and I'll comment tomorrow morning about how it is going.
--
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
[GitHub] [camel] github-actions[bot] commented on pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #8785:
URL: https://github.com/apache/camel/pull/8785#issuecomment-1329364349
:no_entry_sign: There are (likely) no components to be tested in this PR
--
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
[GitHub] [camel] orpiske commented on a diff in pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
orpiske commented on code in PR #8785:
URL: https://github.com/apache/camel/pull/8785#discussion_r1034416912
##########
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:
Yes, it's stateful because of `budget` and `elapsed`.
I think your suggestion is +/- in line with what I commented on the Zulip chat: to investigate moving the latch into the budget and consider it as part of that. Granted, I did not comment on the elapsed, but I think it would eventually become a natural move as well.
IMHO, making (all) the tasks stateless - keeping the state on the budget itself - could potentially be a better approach for the future (maybe as part of Camel 4?), but I think the current approach is fine.
--
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
[GitHub] [camel] thuri commented on pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
thuri commented on PR #8785:
URL: https://github.com/apache/camel/pull/8785#issuecomment-1333462522
Happy to help.
Thank you for your reviews.
That was fun. I hope we can repeat this :-D
--
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
[GitHub] [camel] thuri commented on pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
thuri commented on PR #8785:
URL: https://github.com/apache/camel/pull/8785#issuecomment-1331094435
I have squashed the changes that actual fix the issue and the small commit that has been made by github.
I'd leave the other changes as separate commits because they're not really related to the issue.
One commit is the default implementation of the run(Predicate) method.
The other commit is moving the latch and the completed flag to the object level.
@orpiske If possible it would be good to repeat the manual test especially because the completed flag and latch are now member variables.
--
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
[GitHub] [camel] thuri commented on pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
thuri commented on PR #8785:
URL: https://github.com/apache/camel/pull/8785#issuecomment-1331804774
Not sure why the build fails here. I need to have a deeper look into this. The checksource maven profile didn't show me any error and i can't find the build log for the failing test in camel-cdi ... May take a while.
Sorry :-(
--
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
[GitHub] [camel] thuri commented on a diff in pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
thuri commented on code in PR #8785:
URL: https://github.com/apache/camel/pull/8785#discussion_r1033946639
##########
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:
Fair point and also a good idea. From what I understand I'd say that `BackgroundTask` is stateful because of the `budget` and the `elapsed` fields. Especially the `next()` method probably has side effects on the state of the budget.
--
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
[GitHub] [camel] thuri commented on a diff in pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
thuri commented on code in PR #8785:
URL: https://github.com/apache/camel/pull/8785#discussion_r1033935822
##########
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:
That's a good idea. Should I add this to the PR?
--
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
[GitHub] [camel] orpiske commented on pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
orpiske commented on PR #8785:
URL: https://github.com/apache/camel/pull/8785#issuecomment-1329282531
GH actions won't test all code affected by this, so I will test it manually.
--
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
[GitHub] [camel] thuri commented on a diff in pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
thuri commented on code in PR #8785:
URL: https://github.com/apache/camel/pull/8785#discussion_r1033919412
##########
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:
It's not necessary but i think it's easier to comprehend when it's explicitly defined
--
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
[GitHub] [camel] orpiske commented on pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
orpiske commented on PR #8785:
URL: https://github.com/apache/camel/pull/8785#issuecomment-1331894640
> Click on Details then Summary, you will then see the link to download the different log files at the bottom of the page. In your case the checkstyle error is:
>
> ```
> [camel-support] [INFO] Starting audit...
> [ERROR] /home/runner/work/camel/camel/core/camel-support/src/main/java/org/apache/camel/support/task/BlockingTask.java:40:5: File contains tab characters (this is the first instance). [FileTabCharacter]
> ```
>
> Always tries to launch the command `mvn clean install -Psourcecheck` against the modules in which you have updated files
Actually, only us committters have access to those files (due to Apache rules). So, in cases like these, we have to upload them in the comments.
--
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
[GitHub] [camel] orpiske commented on pull request #8785: CAMEL-18766 Fixes bug in camel-support
Posted by GitBox <gi...@apache.org>.
orpiske commented on PR #8785:
URL: https://github.com/apache/camel/pull/8785#issuecomment-1331897051
> >
>
> Thanks for the heads up. I'll retest tonight and I'll comment tomorrow morning about how it is going.
The tests passed on my own CI. We can merge it (and backport) once you fix the formatting problem.
--
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