You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "lsergio (via GitHub)" <gi...@apache.org> on 2024/08/09 15:13:09 UTC
[PR] feat(#5755): avoiding deadlock between builds with dependencies build [camel-k]
lsergio opened a new pull request, #5757:
URL: https://github.com/apache/camel-k/pull/5757
… order strategy when the builds share enough dependencies
<!-- Description -->
Closes #5755
<!--
Enter your extended release note in the below block. If the PR requires
additional action from users switching to the new release, include the string
"action required". If no release note is required, write "NONE".
You can (optionally) mark this PR with labels "kind/bug" or "kind/feature" to make sure
the text is added to the right section of the release notes.
-->
**Release Note**
```release-note
Fixed dependencies build order strategy to avoid deadlocks
```
--
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
Re: [PR] bugfix(#5755): avoiding deadlock between builds with dependencies build order strategy [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #5757:
URL: https://github.com/apache/camel-k/pull/5757#issuecomment-2288070863
> LGTM. Just a small question : wouldn't it be better to use `defaults.DefaultRuntimeVersion` instead of "3.8.1" in the tests ?
Yeah, @lsergio it would be good if you can change in order to maintain consistency with the default settings (and other tests using the same). Thanks!
--
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
Re: [PR] bugfix(#5755): avoiding deadlock between builds with dependencies build order strategy [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5757:
URL: https://github.com/apache/camel-k/pull/5757#discussion_r1713274840
##########
pkg/apis/camel/v1/build_types_support.go:
##########
@@ -311,10 +332,27 @@ func (bl BuildList) HasMatchingBuild(build *Build) (bool, *Build) {
// additionally check for the creation timestamp
if b.CreationTimestamp.Before(&build.CreationTimestamp) {
return true, &b
+ } else if b.CreationTimestamp.Equal(&build.CreationTimestamp) {
Review Comment:
I think this extra condition check is okey.
--
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
Re: [PR] bugfix(#5755): avoiding deadlock between builds with dependencies build order strategy [camel-k]
Posted by "lsergio (via GitHub)" <gi...@apache.org>.
lsergio commented on code in PR #5757:
URL: https://github.com/apache/camel-k/pull/5757#discussion_r1713588914
##########
pkg/apis/camel/v1/build_types_support.go:
##########
@@ -311,10 +332,27 @@ func (bl BuildList) HasMatchingBuild(build *Build) (bool, *Build) {
// additionally check for the creation timestamp
if b.CreationTimestamp.Before(&build.CreationTimestamp) {
return true, &b
+ } else if b.CreationTimestamp.Equal(&build.CreationTimestamp) {
Review Comment:
This condition is included in the block that handles the cases where all dependencies are matching.
However, while testing, I saw the issue happening where at least half, but not all of the dependencies match. Each build kept waiting for the other one.
This is the scenario in the test case `TestMatchingBuildsSchedulingSharedDependencies`.
I handle it in the first `else` block.
Before it would just return true, causing the deadlock:
```
} else if missing > 0 {
// found another suitable scheduled build with fewer dependencies that should build first in order to reuse the produced image
return true, &b
```
I also included a runtime check. If the builds have different runtime versions, they can't share images and thus there's no reason to wait.
--
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
Re: [PR] bugfix(#5755): avoiding deadlock between builds with dependencies build order strategy [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #5757:
URL: https://github.com/apache/camel-k/pull/5757#issuecomment-2286214541
@lsergio please rebase with `main`. I have fixed the CI.
--
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
Re: [PR] bugfix(#5755): avoiding deadlock between builds with dependencies build order strategy [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez merged PR #5757:
URL: https://github.com/apache/camel-k/pull/5757
--
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
Re: [PR] bugfix(#5755): avoiding deadlock between builds with dependencies build order strategy [camel-k]
Posted by "gansheer (via GitHub)" <gi...@apache.org>.
gansheer commented on PR #5757:
URL: https://github.com/apache/camel-k/pull/5757#issuecomment-2288559907
> > > LGTM. Just a small question : wouldn't it be better to use `defaults.DefaultRuntimeVersion` instead of "3.8.1" in the tests ?
> >
> >
> > Yeah, @lsergio it would be good if you can change in order to maintain consistency with the default settings (and other tests using the same). Thanks!
>
> Hi @squakez and @gansheer. When I do it, I get the following error while building:
>
> ```
> # github.com/apache/camel-k/v2/pkg/apis/camel/v1
> package github.com/apache/camel-k/v2/pkg/apis/camel/v1
> imports github.com/apache/camel-k/v2/pkg/util/defaults
> imports github.com/apache/camel-k/v2/pkg/util/log
> imports github.com/apache/camel-k/v2/pkg/apis/camel/v1: import cycle not allowed in test
> ```
>
> It is not really clear for me why this is happening. :( If someone can provide some hint, that would be great. I googled for the error and could not find anything helpful. For the test purpose, though, the runtime versions could be anything. I could have called them "runtime-1" and "runtime-2", for example.
If these does not need a real runtime version, then I would go with fake ones like you suggest.
--
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
Re: [PR] bugfix(#5755): avoiding deadlock between builds with dependencies build order strategy [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #5757:
URL: https://github.com/apache/camel-k/pull/5757#issuecomment-2288562650
It's a circular reference loop. Yeah, let's keep this way then. Merging, thanks!
--
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
Re: [PR] bugfix(#5755): avoiding deadlock between builds with dependencies build order strategy [camel-k]
Posted by "gansheer (via GitHub)" <gi...@apache.org>.
gansheer commented on PR #5757:
URL: https://github.com/apache/camel-k/pull/5757#issuecomment-2284276184
LGTM. Just a small question : wouldn't it be better to use `defaults.DefaultRuntimeVersion` instead of "3.8.1" in the tests ?
--
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
Re: [PR] bugfix(#5755): avoiding deadlock between builds with dependencies build order strategy [camel-k]
Posted by "lsergio (via GitHub)" <gi...@apache.org>.
lsergio commented on PR #5757:
URL: https://github.com/apache/camel-k/pull/5757#issuecomment-2288515467
> > LGTM. Just a small question : wouldn't it be better to use `defaults.DefaultRuntimeVersion` instead of "3.8.1" in the tests ?
>
> Yeah, @lsergio it would be good if you can change in order to maintain consistency with the default settings (and other tests using the same). Thanks!
Hi @squakez and @gansheer. When I do it, I get the following error while building:
```
# github.com/apache/camel-k/v2/pkg/apis/camel/v1
package github.com/apache/camel-k/v2/pkg/apis/camel/v1
imports github.com/apache/camel-k/v2/pkg/util/defaults
imports github.com/apache/camel-k/v2/pkg/util/log
imports github.com/apache/camel-k/v2/pkg/apis/camel/v1: import cycle not allowed in test
```
It is not really clear for me why this is happening. :(
If someone can provide some hint, that would be greate. I googled for the error and could not find anything helpful.
For the test purpose, though, the runtime versions could be anything. I could have called them "runtime-1" and "runtime-2", for example.
--
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