Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix(#5755): avoiding deadlock between builds with dependencies build order strategy #5757

Merged
merged 3 commits into from
Aug 14, 2024

Conversation

lsergio
Copy link
Contributor

@lsergio lsergio commented Aug 9, 2024

Closes #5755

Release Note

Fixed dependencies build order strategy to avoid deadlocks

@lsergio lsergio marked this pull request as draft August 9, 2024 15:13
@lsergio lsergio force-pushed the feat/5755-deps-strategy-fix branch from de00a60 to 2c68f13 Compare August 9, 2024 15:44
@lsergio lsergio changed the title feat(#5755): avoiding deadlock between builds with dependencies build bugfix(#5755): avoiding deadlock between builds with dependencies build order strategy Aug 9, 2024
@lsergio lsergio force-pushed the feat/5755-deps-strategy-fix branch from 2c68f13 to 6e5b851 Compare August 9, 2024 17:54
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I think the extra condition check you've added when the time is exactly equal should be enough to avoid the deadlock. I am not sure to understand why you're changing the rest of the algorithm. Also +1 on the unit tests.

pkg/apis/camel/v1/build_types_support.go Outdated Show resolved Hide resolved
@lsergio lsergio marked this pull request as ready for review August 12, 2024 11:27
@lsergio lsergio force-pushed the feat/5755-deps-strategy-fix branch from 6e5b851 to 58c814c Compare August 12, 2024 11:32
@gansheer
Copy link
Contributor

LGTM. Just a small question : wouldn't it be better to use defaults.DefaultRuntimeVersion instead of "3.8.1" in the tests ?

@squakez
Copy link
Contributor

squakez commented Aug 13, 2024

@lsergio please rebase with main. I have fixed the CI.

@lsergio lsergio force-pushed the feat/5755-deps-strategy-fix branch from c4ce7ff to 35b89dc Compare August 13, 2024 16:21
@squakez
Copy link
Contributor

squakez commented Aug 14, 2024

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!

@lsergio
Copy link
Contributor Author

lsergio commented Aug 14, 2024

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.

@gansheer
Copy link
Contributor

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.

@squakez
Copy link
Contributor

squakez commented Aug 14, 2024

It's a circular reference loop. Yeah, let's keep this way then. Merging, thanks!

@squakez squakez merged commit e77a314 into apache:main Aug 14, 2024
9 checks passed
@lsergio lsergio deleted the feat/5755-deps-strategy-fix branch August 14, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible deadlock between integration builds
3 participants